-
-
Notifications
You must be signed in to change notification settings - Fork 435
[3.0] Add Vulkan bindings for Silk 3.0 #2457
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
Draft
Exanite
wants to merge
114
commits into
dotnet:develop/3.0
Choose a base branch
from
Exanite:feature/vulkan-bindings-3.0
base: develop/3.0
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Draft
+253,728
−1,206
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This code seems to look for "<feature api=" elements in vk.xml. However, one of the six total elements don't have a "depends" attribute. You can see this by opening https://raw.githubusercontent.com/KhronosGroup/Vulkan-Docs/refs/heads/main/xml/vk.xml and searching for "<feature api=". This is the line that does not have the depends attribute, which causes the assert to fail: <feature api="vulkan,vulkansc" name="VK_VERSION_1_0" number="1.0" comment="Vulkan core API interface definitions"> The other relevant lines do have a depends attribute: <feature api="vulkan,vulkansc" name="VK_VERSION_1_1" number="1.1" depends="VK_VERSION_1_0" comment="Vulkan 1.1 core API interface definitions.">
This commit fixes the allowLeadingDigits overload. The previous similarly named commit fixes the maxLen overload.
This currently doesn't generate anything for some reason (only the enums generate, and that's because of a previous mod)
I'm guessing we don't need this since we don't care about vulkansc. Let me know if we want to keep this defined.
Nothing generates yet. Not sure why.
There are some file names that are incorrect (the enum name inside the file is fine), such as sources/Vulkan/Vulkan/Vulkan/VkBufferCreateFlagBits.gen.cs. They all seem to be enums. I did some light checking and it seems that these incorrectly named enums don't have equivalents in the Enums folder. Some enums have multiple versions, which is expected, but the newer versions tend to be in the Enums folder while the older versions are in the Vulkan folder: For example: sources/Vulkan/Vulkan/Vulkan/VkAccessFlagBits.gen.cs sources/Vulkan/Vulkan/Enums/AccessFlags2.gen.cs sources/Vulkan/Vulkan/Enums/AccessFlags3KHR.gen.cs Interestingly, as I was writing this commit message, I noticed that AccessFlags.gen.cs was named correctly before I added the traverse argument in my previous commits.
Will regenerate using Windows or whichever platform is preferred later for consistency.
a117cb2
to
20ef463
Compare
Putting this here for reference. These are the logs related to the generation of enums during the MixKhronosData mod after the latest commit. The enums specified in these errors either are:
|
20ef463
to
e97a9a7
Compare
Only uint64_t=ulong affects the generation, but the others are included to be slightly more complete. This does not fix the enum backing types though, which also differ between Linux/Windows. See this conversation in the Silk.NET Discord for more information: https://discord.com/channels/521092042781229087/1376331581198827520/1376628539536969949 Relevant ClangSharp issue: dotnet/ClangSharp#574
This is related to dotnet/ClangSharp#601
54c6350
to
57bde07
Compare
…immer This also prepares the Rewriter class for a few other changes.
Bit invasive, but since all FlagBits enums are already treated the same way, this ensures that everything is processed consistently. This also avoids the need to tell ClangSharp to remap these enums.
…int/ulongs with types that don't exist in the project
This adds a "None = 0" member to Flags enums if they do not have an equivalent already
Because the MaxEnum member got removed, some enum members had their trimming adjusted as a side effect. Case 1: Matches Silk 2: - SamplerYcbcrRange - SemaphoreImportFlags - DeviceDiagnosticsConfigFlagsNV - FenceImportFlags - VideoDecodeCapabilityFlagsKHR - VideoEncodeFeedbackFlagsKHR - ClusterAccelerationStructureAddressResolutionFlagsNV (Doesn't exist in Silk 2) Case 2: Unintended changes (need to fix): - IndirectStateFlagsNV - SemaphoreImportFlags - SemaphoreWaitFlags - SurfaceCounterFlagsEXT
Doesn't seem to affect OpenGL bindings generation This is to ensure all trimming code happens after rewinding, which is useful because I'm thinking of refactoring this code and want to ensure that I can treat all of the trimming code equally.
This is to avoid trimming things like GL_T from OpenGL
This fixes a few cases where the trimmer fails to trim enum member names. Note that this affects both the Vulkan bindings and the OpenGL bindings. (cherry picked from commit 2df6cef)
(cherry picked from commit 832c95b)
… trimmed too much of the enum member names
…val logic to TransformEnums
…that I'm getting confused
These only seem to exist for Vulkan currently. For Vulkan: These represent misnamed constants that have now been renamed to be consistent with the rest of the Vulkan spec. For Silk: These misnamed constants and their corrected variants cause name collisions when names are prettified (thus leading to compile errors) and cause common prefix determination for enum member names to be more pessimistic (thus leading to longer names). For users: Removing these aliases can induce more API breaks, but because these are aliases and always have an alternative, API breaks are simple to fix. Removing these aliases also makes it clearer which variant is the correct variant to use.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Current progress
(Will update this as I work)
Bindings now compile!!!
See bottom of PR for current progress since I'm now actively tracking todos here.
Summary of the PR
This PR primarily adds Vulkan bindings.
Summary of important decisions/changes:
remap-stdint.rsp
file specific to Vulkan bindings generation to fixuint64_t
mapped tonuint
on Linux (butulong
on Windows) ClangSharp#574Silk.NET.SilkTouch.Mods.LocationTransformation
namespace. This contains code that allows a set of transformations to be applied to the locations returned by SymbolFinder.FindReferencesAsync. This is currently used for renaming and for pointer dimension reduction (eg: replacingType*
withType
).Related issues, Discord discussions, or proposals
Initial Discord discussion: https://discord.com/channels/521092042781229087/587346162802229298/1375224188129906778
Discord thread: https://discord.com/channels/521092042781229087/1376331581198827520/1376623544875880589
Further Comments
Things to know
The change to ExtractNestedTyping and TransformHandles is a breaking change.
Existing binding generator configurations need to have their configurations updated.
Todos
Discuss
StdVideoEncodeH264SliceHeaderFlags
Attributes
These are not important for the bindings to be used, but are useful.
I might do these in a separate PR.
Trimming
Vk
inPFNVkDebugUtilsMessengerCallbackEXT
is not trimmed properlyBefore undrafting PR
Future PRs
In progress
Done
Bools
SamplerCreateInfo.UnnormalizedCoordinates
,SamplerCreateInfo.CompareEnable
, etc usesuint
instead of abool
-like type.Deprecated aliases
VK_STRUCTURE_TYPE_SURFACE_CAPABILITIES2_EXT
(alias ofVK_STRUCTURE_TYPE_SURFACE_CAPABILITIES_2_EXT
),VK_KHR_MAINTENANCE1_SPEC_VERSION
, etc are deprecated aliases that cause compilation issues due to name conflicts. These are currently manually excluded.deprecated="aliased"
attribute in the XML specMaxEnum members
INVALID
members (Vulkan Video)INVALID
members when the name contains the stringin the Vulkan XML spec (the XML spec is out of date/out of sync)
[Flags]
attributes.I ended up making it so that the
TransformEnums
mod handlesMaxEnum
removal instead ofMixKhronosData
. This is because the max enum value pattern applies to C as a whole, rather than just Vulkan or Khronos.data.Groups[group] = data.Groups.TryGetValue(group, out var groupInfo)
in MixKhronosData and read the surrounding code for more context)I also made it so that MixKhronosData does not skip over empty enum groups, which fixes the generation of
[Flags]
attributes and leads to a few more enums being generated.Trimming
ComponentSwizzle.ComponentSwizzleIdentity
, etc is not trimmed properly. The enum name isn't getting trimmed here for some reason.ShaderStageFlags.ShaderStageVertexBit
, etc is not trimmed properlyGuess: ShaderStageFlags is not a prefix of ShaderStageVertexBitIncorrect. This was the same issue as above.PipelineCompilerControlFlagsAMD.FlagBitsMaxEnumAMD
, etc trimmingMaxEnum
members. We don't want these do we?Fix trimming first, then remove these members.TheMaxEnum
members are the only ones that are named like this in the headers so we can just remove them. This isn't a trimming issue.MaxEnum
members breaksIndirectStateFlagsNV.FlagFrontfaceBitNV
due to no prefixes being shared by member namesIndirectStateFlagsNV
,SemaphoreImportFlags
,SemaphoreWaitFlags
,SurfaceCounterFlagsEXT
Linux
eng/silktouch/vulkan/settings.rsp
/usr/lib/clang/20/include
as an include directory.20
is the Clang version.UnixStdIncludeResolver
/usr/lib/clang/*/include
folder to UnixStdIncludeResolver. It finds all matching folder patterns and takes the latest version.These are related to the handling of Flags enums.
DebugUtilsMessageSeverityFlagsEXT
is not marked with the[Flags]
attribute.[Flags]
. Does this functionality exist in Silk 3 yet?KnownBitmask
inMixKhronosData.Rewriter
.DependencyFlags.None
does not exist, but does in Silk 2. None does not exist in the Vulkan headers nor XML spec, so I'm assuming None was added by Silk itself.ShaderStageFlags.None
I attempted to fix this, but led to some unwanted outputs, such as names not being trimmed properly (does the trimmer check to see if all enum members share a prefix?). Also, knowing when to add in theNone = 0
member requires evaluating the value of the existing enum members. This is somewhat difficult because they look likeunchecked((ulong)0UL)
, which isn't trivially comparable to 0.This might fit better in a TransformFlags mod so we can have this behavior in all Silk bindings.It looks like we can useIFieldSymbol.ConstantValue
for thissemanticModel.GetConstantValue
in a newTransformFlags
mod.Original comment:
These two issues are Flags/FlagBits specific issues. This disconnect causes ClangSharp to just output ulongs/uints. This is likely something we should fix on our side. We can fix this by looking at the NativeTypeName attribute.Original comment:
Some are trivial, but some NativeTypeNames look likeconst VkPipelineStageFlags *
, such as in SubmitInfo.MemoryBarrier2.SrcStageMask
does not use theAccessFlags2
enum and uses ulongs instead.DependencyInfo.DependencyFlags
does not use theDependencyFlags
enum and uses uint instead.ImageSubresourceRange.AspectMask
CommandPoolCreateInfo.Flags
FenceCreateInfo.Flags
ClearAttachment.AspectMask
Original comment:
I recently found code that handles the FlagBits -> Flags mapping in MixKhronosData. I think the issue is that this code doesn't fully work yet so I'm looking into seeing how that code works and fixing that code.sources/SilkTouch/SilkTouch/Mods/MixKhronosData.cs
.FlagBits
withFlags
in identifier names globally.VK_LOD_CLAMP_NONE
and other constants went.