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

src: Add vk_format_utils to UtilityHeaders #89

Merged
merged 1 commit into from
Sep 11, 2023

Conversation

charles-lunarg
Copy link
Collaborator

The library originates from Vulkan-ValidationLayers, but is being moved into this repo to make it easier for others to use it.

The library has also been modified to be header only and C compatible, which allows more developers to be able to use it. This does require some changes, but only affects the FormatElementSize and FormatTexelSize functions which used default parameters. Two new functions, FormatElementSizeWithAspect and FormatTexelSizeWithAspect have been added to handle the non-default image aspect case (the default was COLOR_BIT).

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.

I guess before we merge here, a good exercise would be go in the Validation Layers

  1. Remove the current format utils (so it doesn't generate)
  2. Replace the header with this file
  3. Make sure CI will pass and this didn't forget anything subtle a test there would have caught

(the goal not being to merge that PR, but to prove this new interface works)

@charles-lunarg
Copy link
Collaborator Author

I guess before we merge here, a good exercise would be go in the Validation Layers

I started to do that on Friday and did not have much trouble with the conversion but had a lot of trouble running tests (w and w/o mock icd). It felt like I setup the repo wrong but I was on a different machine than I normally develop on, so I’ll try again come the morn on that machine.

inline bool FormatIsUSCALED(VkFormat format);

// Returns whether a format type is any type from "Interpretation of Numeric Format" table (OpTypeInt)
inline bool FormatIsSampledInt(VkFormat format);

Choose a reason for hiding this comment

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

Is "Interpretation of Numeric Format" table about the formats that can be sampled? Not sure I understand semantics of this function and in which scenarios it can be used.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I wasn't super clear on that myself - but I'll look at the format table in question and double check that.

Copy link
Contributor

Choose a reason for hiding this comment

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

so for the formats, awhile ago I tried to unify the wording

https://registry.khronos.org/vulkan/specs/1.3-extensions/html/vkspec.html#formats-numericformat

internal MR

in short:

  • Numeric Format = [SFLOAT, SINT, SNORM, SRGB, SSCALED, UFLOAT, UINT, UNORM, USCALED]
  • Numeric Type = [float, signed int, unsigned int]
  • SPIR-V Type = [ [OpTypeFloat, OpTypeInt], width, sign ]

.... basically we should re-write this comment to whatever @charles-lunarg thinks "makes sense" as I am now to tainted with spec lawyer knowledge

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Let me know if the comments make more sense now.

@juan-lunarg
Copy link
Contributor

Did some minimal integration testing with VulkanTools. Looks good on that front 👍🏾
LunarG/VulkanTools#1883

include/vulkan/utility/vk_format_utils.h Outdated Show resolved Hide resolved
include/vulkan/utility/vk_format_utils.h Outdated Show resolved Hide resolved
tests/format_utils/test_formats.cpp Outdated Show resolved Hide resolved
@charles-lunarg charles-lunarg force-pushed the format_utils branch 2 times, most recently from bd36e90 to 659a524 Compare September 5, 2023 19:41
Copy link
Contributor

@juan-lunarg juan-lunarg left a comment

Choose a reason for hiding this comment

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

This will require updating BUILD.gn

@charles-lunarg charles-lunarg force-pushed the format_utils branch 4 times, most recently from a749af3 to 2376416 Compare September 11, 2023 17:38
@charles-lunarg
Copy link
Collaborator Author

Apologies for the force push spam - the C compatibility tests & add_subdirectory/find_package tests are easy to forget about since they aren't compiled normally.

The build.gn file should be correct now as well @juan-lunarg

The library originates from Vulkan-ValidationLayers, but is being moved
into this repo to make it easier for others to use it.

The library has also been modified to be header only and C compatible,
which allows more developers to be able to use it. This does require some
changes, but only affects the vkuFormatElementSize and vkuFormatTexelSize
functions which used default parameters. Two new functions,
vkuFormatElementSizeWithAspect and vkuFormatTexelSizeWithAspect have been
added to handle the non-default image aspect case (the default was COLOR_BIT).

Renaming was done using the following convention:
* public header files begin with `vk_`
* enums begin with VKU_FORMAT_
* functions begin with vku
Copy link
Contributor

@juan-lunarg juan-lunarg left a comment

Choose a reason for hiding this comment

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

LGTM

@juan-lunarg juan-lunarg merged commit c903525 into KhronosGroup:main Sep 11, 2023
4 of 10 checks passed
@charles-lunarg charles-lunarg deleted the format_utils branch October 17, 2023 19:20
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.

5 participants