-
Notifications
You must be signed in to change notification settings - Fork 123
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
Move vulkan codegen generators #1851
base: dev
Are you sure you want to change the base?
Move vulkan codegen generators #1851
Conversation
CI gfxreconstruct build queued with queue ID 291417. |
CI gfxreconstruct build # 5243 running. |
CI gfxreconstruct build # 5243 passed. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These look good. Probably can squash the "Rename Base..."
framework/generated/khronos_generators/base_generators/base_decoder_body_generator.py
Outdated
Show resolved
Hide resolved
framework/generated/khronos_generators/khronos_base_generator.py
Outdated
Show resolved
Hide resolved
framework/generated/khronos_generators/khronos_base_generator.py
Outdated
Show resolved
Hide resolved
...rk/generated/khronos_generators/base_generators/base_struct_handle_mappers_body_generator.py
Outdated
Show resolved
Hide resolved
CI gfxreconstruct build queued with queue ID 300213. |
@locke-lunarg or @davidd-lunarg , can one of you review this because it's codegen? |
CI gfxreconstruct build # 5335 running. |
CI gfxreconstruct build # 5335 passed. |
1c1c5cd
to
9809efb
Compare
CI gfxreconstruct build queued with queue ID 300271. |
CI gfxreconstruct build # 5336 running. |
CI gfxreconstruct build # 5336 passed. |
Move the Vulkan generator scripts under a khronos_generators folder to prepare it for for sharing with other khronos APIs.
DX12 was still relying on one file in the base_generators folder. Make a copy in the DX12 folder, and remove all references to the Vulkan and base_generators folders in the generate_dx12.py script.
Move the reformat_code.py into the base of khronos_generators.
Move common items from the vulkan_generators/base_generator.py to the khronos_generators/khronos_base_generator.py
Move more common type parsing into the khronos_base_generator from the vulkan/base_generator.
base_generator code still has vulkan-specific items in it. This starts the process of pulling those out.
Remove more vulkan-specific items in the base_generator code path.
Modify the handling of 'pNext' extended struct variables in the base_generators to be more Khronos-generic instead of Vulkan-specific.
First phase moving of base_generator files to have a khronos identifier
Since the khronos_base files were referring back to the parent "base_generator" (which in this case was Vulkan) it still resulted in a tight coupling. Move the "write" method into the khronos_base_generator script to solve this.
Consolidate the structure type name generation into one location shared with all future Khronos APIs.
Put variable naming in a common location
Removed local variables for use in decoder body scripts and created a "all_cmd_params" that isn't cleared after every feature update.
Added raising an exception on Python methods that should be implemented by the Khronos API for the functionality to properly work.
9809efb
to
e28c3a3
Compare
CI gfxreconstruct build queued with queue ID 307679. |
CI gfxreconstruct build # 5366 running. |
CI gfxreconstruct build # 5366 passed. |
@@ -213,7 +192,7 @@ def make_decode_invocation(self, value, preamble, main_body, epilogue): | |||
is_class and value.pointer_count > 1 | |||
): | |||
if type_name in self.base_header_structs.keys(): | |||
base_type_name = self.gen_child_var_name(value.base_type) | |||
base_type_name = self.makeSimpleVarName(value.base_type) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wonder that should we unify the naming rule to pothole_case(make_simple_var_name)? or is it better to use dromedaryCase here? Plus, there are many other dromedaryCase functions. Should they be pothole_case? or dromedaryCase?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@bradgrantham @MarkY-LunarG @davidd-lunarg -- is there a standard all new code (including new functions for this refactor) should adhere to? Right now it's a mix.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't have a preference but we state PEP-0008 in CONTRIBUTING.md. PEP-8 appears to prefer pothole/snake case for functions and variables. Am I reading it right? https://peps.python.org/pep-0008/#function-and-variable-names
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was following the Khronos standard of naming which uses lower-camel-case for function names and lower-snake-case for variables. I honestly missed the whole PEP-0008 statement. I'm ok with reverting it back.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For example, beginFile
, endFile
, etc.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There are some functions with a specified naming from from the reg.py calls. We can PEP-0008 everything else in the new code.
Having function and variable naming match is somewhat odd until one considers how lambda heavy idiomatic/pythonic coding is.
I'll review that new def's match the standard.
|
||
Adds options used by FrameworkGenerator objects during C++ language | ||
code generation. | ||
|
||
Additional members | ||
blacklists - Path to JSON file listing apicalls and structs to ignore. | ||
platform_types - Path to JSON file listing platform (WIN32, X11, etc.) | ||
specific types that are defined outside of the Vulkan header. | ||
specific types that are defined outside of the OpenXR header. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should it be Khronos header?
I think we should rename |
These changes moves the current Vulkan and base generators into a better layout that can share code in a more understandable way.
Instead of:
After this change:
Also removed some duplication in the files and created utility functions to remove some additional Vulkan-specific items in the lower levels.
This is a big step towards building the appropriate structure for sharing files with OpenXR.