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

Multiple equivalent require blocks could be merged #2404

Closed
Waffl3x opened this issue Aug 13, 2024 · 5 comments
Closed

Multiple equivalent require blocks could be merged #2404

Waffl3x opened this issue Aug 13, 2024 · 5 comments
Assignees

Comments

@Waffl3x
Copy link
Contributor

Waffl3x commented Aug 13, 2024

I am fairly certain the former can be replaced with the latter as the require blocks are equivalent, only differing in their depends attribute. I can open a pull request for this later if this change is desirable.

        <extension name="VK_KHR_push_descriptor" number="81" type="device" author="KHR" depends="VK_KHR_get_physical_device_properties2,VK_VERSION_1_1" contact="Jeff Bolz @jeffbolznv" supported="vulkan" ratified="vulkan">
            <require>
                <enum value="2"                                             name="VK_KHR_PUSH_DESCRIPTOR_SPEC_VERSION"/>
                <enum value="&quot;VK_KHR_push_descriptor&quot;"            name="VK_KHR_PUSH_DESCRIPTOR_EXTENSION_NAME"/>
                <enum offset="0" extends="VkStructureType"                  name="VK_STRUCTURE_TYPE_PHYSICAL_DEVICE_PUSH_DESCRIPTOR_PROPERTIES_KHR"/>
                <enum bitpos="0" extends="VkDescriptorSetLayoutCreateFlagBits"   name="VK_DESCRIPTOR_SET_LAYOUT_CREATE_PUSH_DESCRIPTOR_BIT_KHR"  comment="Descriptors are pushed via flink:vkCmdPushDescriptorSetKHR"/>
                <command name="vkCmdPushDescriptorSetKHR"/>
                <type name="VkPhysicalDevicePushDescriptorPropertiesKHR"/>
            </require>
            <require depends="VK_VERSION_1_1">
                <command name="vkCmdPushDescriptorSetWithTemplateKHR"/>
                <enum value="1" extends="VkDescriptorUpdateTemplateType"    name="VK_DESCRIPTOR_UPDATE_TEMPLATE_TYPE_PUSH_DESCRIPTORS_KHR" comment="Create descriptor update template for pushed descriptor updates"/>
            </require>
            <require depends="VK_KHR_descriptor_update_template">
                <command name="vkCmdPushDescriptorSetWithTemplateKHR"/>
                <enum value="1" extends="VkDescriptorUpdateTemplateType"    name="VK_DESCRIPTOR_UPDATE_TEMPLATE_TYPE_PUSH_DESCRIPTORS_KHR" comment="Create descriptor update template for pushed descriptor updates"/>
            </require>
        </extension>
        <extension name="VK_KHR_push_descriptor" number="81" type="device" author="KHR" depends="VK_KHR_get_physical_device_properties2,VK_VERSION_1_1" contact="Jeff Bolz @jeffbolznv" supported="vulkan" ratified="vulkan">
            <require>
                <enum value="2"                                             name="VK_KHR_PUSH_DESCRIPTOR_SPEC_VERSION"/>
                <enum value="&quot;VK_KHR_push_descriptor&quot;"            name="VK_KHR_PUSH_DESCRIPTOR_EXTENSION_NAME"/>
                <enum offset="0" extends="VkStructureType"                  name="VK_STRUCTURE_TYPE_PHYSICAL_DEVICE_PUSH_DESCRIPTOR_PROPERTIES_KHR"/>
                <enum bitpos="0" extends="VkDescriptorSetLayoutCreateFlagBits"   name="VK_DESCRIPTOR_SET_LAYOUT_CREATE_PUSH_DESCRIPTOR_BIT_KHR"  comment="Descriptors are pushed via flink:vkCmdPushDescriptorSetKHR"/>
                <command name="vkCmdPushDescriptorSetKHR"/>
                <type name="VkPhysicalDevicePushDescriptorPropertiesKHR"/>
            </require>
            <require depends="VK_VERSION_1_1,VK_KHR_descriptor_update_template">
                <command name="vkCmdPushDescriptorSetWithTemplateKHR"/>
                <enum value="1" extends="VkDescriptorUpdateTemplateType"    name="VK_DESCRIPTOR_UPDATE_TEMPLATE_TYPE_PUSH_DESCRIPTORS_KHR" comment="Create descriptor update template for pushed descriptor updates"/>
            </require>
        </extension>
@Waffl3x
Copy link
Contributor Author

Waffl3x commented Aug 13, 2024

I found another example, I'll probably have to write a script to locate all cases.

        <extension name="VK_EXT_full_screen_exclusive" number="256" type="device" author="EXT" depends="(VK_KHR_get_physical_device_properties2,VK_VERSION_1_1)+VK_KHR_surface+VK_KHR_get_surface_capabilities2+VK_KHR_swapchain" platform="win32" contact="James Jones @cubanismo" supported="vulkan" ratified="vulkan">
            <require>
                <enum value="4"                                             name="VK_EXT_FULL_SCREEN_EXCLUSIVE_SPEC_VERSION"/>
                <enum value="&quot;VK_EXT_full_screen_exclusive&quot;"      name="VK_EXT_FULL_SCREEN_EXCLUSIVE_EXTENSION_NAME"/>
                <enum offset="0" extends="VkStructureType"                  name="VK_STRUCTURE_TYPE_SURFACE_FULL_SCREEN_EXCLUSIVE_INFO_EXT"/>
                <enum offset="2" extends="VkStructureType"                  name="VK_STRUCTURE_TYPE_SURFACE_CAPABILITIES_FULL_SCREEN_EXCLUSIVE_EXT"/>
                <enum offset="0" extends="VkResult" dir="-"                 name="VK_ERROR_FULL_SCREEN_EXCLUSIVE_MODE_LOST_EXT"/>
                <type name="VkFullScreenExclusiveEXT"/>
                <type name="VkSurfaceFullScreenExclusiveInfoEXT"/>
                <type name="VkSurfaceCapabilitiesFullScreenExclusiveEXT"/>
                <command name="vkGetPhysicalDeviceSurfacePresentModes2EXT"/>
                <command name="vkAcquireFullScreenExclusiveModeEXT"/>
                <command name="vkReleaseFullScreenExclusiveModeEXT"/>
            </require>
            <require depends="VK_KHR_win32_surface">
                <enum offset="1" extends="VkStructureType"                  name="VK_STRUCTURE_TYPE_SURFACE_FULL_SCREEN_EXCLUSIVE_WIN32_INFO_EXT"/>
                <type name="VkSurfaceFullScreenExclusiveWin32InfoEXT"/>
            </require>
            <require depends="VK_KHR_device_group">
                <command name="vkGetDeviceGroupSurfacePresentModes2EXT"/>
            </require>
            <require depends="VK_VERSION_1_1">
                <command name="vkGetDeviceGroupSurfacePresentModes2EXT"/>
            </require>
        </extension>

@oddhack
Copy link
Contributor

oddhack commented Aug 14, 2024

Agreed this is harmless in terms of the generated artifacts from the spec repo. We'll discuss as to whether there might be any downstream consequences.

Sometimes we refactor interactions like this from one extension to another, which is probably how this happened.

@Waffl3x
Copy link
Contributor Author

Waffl3x commented Aug 14, 2024

Sometimes we refactor interactions like this from one extension to another, which is probably how this happened.

The theory I heard was it happened during the transition from requires and requiresCore to depends. Sounds very plausible to me as this is how it would have been structured for those attributes.

@oddhack
Copy link
Contributor

oddhack commented Aug 14, 2024

We discussed and are OK with this - please go ahead and propose a PR to that effect. If you happen to write a detection tool we could run in CI, that seems useful as well (preferably as a Python script).

Waffl3x added a commit to Waffl3x/Vulkan-Docs that referenced this issue Aug 14, 2024
Addresses KhronosGroup#2404
Merges extension require blocks that have identical children,
extending the depends attribute with a logical or.
oddhack pushed a commit that referenced this issue Aug 16, 2024
* Merge equivalent require blocks
Addresses #2404
Merges extension require blocks that have identical children,
extending the depends attribute with a logical or.

* Add utility script
This script prints out require blocks that are eligible to be
merged together.
@oddhack
Copy link
Contributor

oddhack commented Aug 16, 2024

Closed with #2406 merged and published in the latest spec update.

@oddhack oddhack closed this as completed Aug 16, 2024
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

No branches or pull requests

2 participants