Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Refactor codegen scripts to use vulkan_object.py #1665

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

charles-lunarg
Copy link
Collaborator

Requires hardcoding a few things to make it work, most notably:

  • Missing Extension Requires Depends information
  • Missing aliases for handles

On the whole, the codegen outputs almost identical files from before, making the refactor not able to introduce issues into the output. The changes that do occur to remove duplicated if statements from aliased handles or for comments and whitespace.

Requires hardcoding a few things to make it work, most notably:
* Missing Extension Requires Depends information
* Missing aliases for handles

On the whole, the codegen outputs *almost* identical files from before,
making the refactor not able to introduce issues into the output. The
changes that do occur to remove duplicated if statements from aliased
handles or for comments and whitespace.
@ci-tester-lunarg
Copy link

CI Vulkan-Loader build queued with queue ID 379810.

@ci-tester-lunarg
Copy link

CI Vulkan-Loader build queued with queue ID 379827.

@charles-lunarg
Copy link
Collaborator Author

Oh foey I didn't mark this as a draft. The incremental & verify options are broken because the codegen creates files in different directories and neither of those options match.

Very much wanting to get feedback on the codegen itself. I know the clang-format pass that VVL has is missing, but I didn't feel that it was all that necessary to copy/paste all that stuff over.

@charles-lunarg charles-lunarg marked this pull request as draft February 24, 2025 23:36
@ci-tester-lunarg
Copy link

CI Vulkan-Loader build # 2927 running.

@ci-tester-lunarg
Copy link

CI Vulkan-Loader build # 2927 passed.

def OutputDispatchTableHelper(self, table_type):
entries = []
table = ''
out.append('#pragma once\n')
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

any reason to not do a ''' multi-block here so it is only a single append (like https://github.com/KhronosGroup/Vulkan-ValidationLayers/blob/main/scripts/generators/api_version_generator.py#L89)

table = ''
out.append('#pragma once\n')
out.append('// *** THIS FILE IS GENERATED - DO NOT EDIT ***\n')
out.append('// See dispatch_table_helper_generator.py for modifications\n')
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
out.append('// See dispatch_table_helper_generator.py for modifications\n')
out.append(f'// See {os.path.basename(__file__)} for modifications\n')



def convert_to_VK_OBJECT_TYPE(self, str_to_convert):
return '_'.join(['VK', 'OBJECT', 'TYPE'] + self.split_handle_by_capital_case(str_to_convert)).upper()
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What is this join for? why not just go

Suggested change
return '_'.join(['VK', 'OBJECT', 'TYPE'] + self.split_handle_by_capital_case(str_to_convert)).upper()
return 'VK_OBJECT_TYPE' + self.split_handle_by_capital_case(str_to_convert)).upper()

(same for VK_DEBUG_REPORT_OBJECT_TYPE)

protos += ' return VK_ERROR_EXTENSION_NOT_PRESENT;\n'
protos += '}\n\n'
return protos
for command in self.vk.commands.values():
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

if you want to level up your python

Suggested change
for command in self.vk.commands.values():
for command in [x for x in self.vk.commands.values() if len(x.extensions) > 0]:

Copy link
Contributor

@spencer-lunarg spencer-lunarg left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thats a lot of python removed

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants