3.0 Vulkan Bindings

Alright, that should be plenty of information for me to work with. I'll keep reading through the codebase and see if I can't get the RSPs working. Thanks for taking the time to explain everything 😄
485 Replies
Exanite
ExaniteOP3mo ago
I now have some initial Vulkan bindings generated. It doesn't compile yet and there are a few issues that I've noticed. I'll go through them here and also ask a few additional questions. Current problems: 1) Most of the enums now have defined types, but some are still missing. I've posted the logs in a comment in the PR (https://github.com/dotnet/Silk.NET/pull/2457#issuecomment-2910293716). There are 3 cases that I've identified (not an enum, part of a different header, and does not exist). Silk 2 does generate code corresponding to case 3. Case 2 probably can be fixed by adding multiple source sets (similar to TerraFX) or just including+traversing those directly. How should I handle these cases? 2) There are free floating properties at the bottom of https://raw.githubusercontent.com/dotnet/Silk.NET/20ef4637fdf5910ede171aa70676bfe2f4cd059b/sources/Vulkan/Vulkan/Vulkan/Vulkan.gen.cs. I haven't looked into why this occurs yet. Multiple source sets? 3) Should I add a source set (if that's the right term) for each of the Vulkan headers, similar to TerraFX (https://github.com/terrafx/terrafx.interop.vulkan/tree/main/generation/Vulkan/vulkan)? 4) How should I handle Vulkan extensions? Would they be related to source sets or are they handled in a different way? 5) What is the naming scheme for the header files in the RSPs folder? I'm guessing this doesn't matter too much though. Unique names can help with debugging since it's easier to tell which source set the header comes from (the OpenGL ones all have the same name, even though there are 4 source sets). Configuration: 6) For MixKhronosData, should I use UseExtensionVendorTrimmings: None, All, or KhronosOnly? I currently don't specify the property at all. 7) What VTables should I generate (DllImport, StaticWrapper, ThisThread) and what settings? I currently only use DllImport and have set it to be default. Oh, one other problem that I forgot to mention. 7) Some enums don't have the correct file name. I explained this in the commit message of https://github.com/dotnet/Silk.NET/pull/2457/commits/f7c38b0e1498b9b0ec6f2bdd1007b042563458cb Before adding the traverse argument, some enums such as AccessFlags had the correct name and are in the Enums folder, but afterwards, they were moved to the Vulkan folder and have the incorrect/original name (VkAccessFlagBits). Everything else about the enums seems correct. Just the file name is wrong.
Perksey
Perksey3mo ago
I probably wouldn't bother with the multiple source sets, although there may be some cases where it's needed e.g. where different preprocessor directives are needed to wake up specific extensions without affecting others. tbh, looking at TerraFX.Interop.Vulkan, it looks like there is probably a case for generating (or at least semi-generating) the RSPs to ensure that we can handle the Flags/FlagBits properly. MixKhronosData's name trimmer will probably need to have some logic to strip off _T but otherwise it should all just work, so I don't think we need to explicitly declare the handles like they have. In 3.0 all of the extensions are going to be in one project, with us eventually adding a SupportedApiProfile attribute analyzer to ensure this isn't a loaded footgun. so from a vulkan perspective, treating the extension functions/APIs like any other API is A-OK. AccessFlagBits is technically correct unfortunately, as that's what the enum is declared as essentially, because C enums have no type, Khronos for flags (where stricter typing is required) decided that it will declare the possible values as a *FlagBits enum, but the parameters declare the *Flags typedef (which typedefs to VkFlags, which in turn typedefs to uint32_t) so they can guarantee the size in 2.X had a manual fixup for this, and we'll probably need logic in 3.0 to rename FlagBits to Flags as well
Exanite
ExaniteOP3mo ago
Yup, I get that part. I'm more talking about the inconsistency here:
No description
Exanite
ExaniteOP3mo ago
I guess it is related though
Perksey
Perksey3mo ago
interesting so it's probably helpful to look at the header: https://github.com/KhronosGroup/Vulkan-Headers/blob/main/include/vulkan/vulkan_core.h where it does indeed seem they are declared differently that is really fucking annoying why tf is it like that
Exanite
ExaniteOP3mo ago
AccessFlags2 and AccessFlags3KHR are also empty enums. VkAccessFlagBits seems to be fine
Perksey
Perksey3mo ago
I presume the Flags are generated by MixKhronosData, if that's still in the loop https://github.com/KhronosGroup/Vulkan-Docs/blob/main/scripts/generator.py#L628 looks like for some reason they only do typedef enum for 32-bit bitmasks
Exanite
ExaniteOP3mo ago
2 and 3 are from MixKhronos. VkAccessFlagBits was also empty (and named AccessFlags) until I added the RSPs.
Perksey
Perksey3mo ago
hmmmm, so on one hand the headers are very unhelpful but on the other hand MixKhronosData should already be used to that helpfulness from OpenGL, and for some reason isn't moving the constants to the enums itself
Exanite
ExaniteOP3mo ago
I didn't mention this earlier, but the current committed bindings were generated on Linux. The main difference is that on Linux some usages of uint are replaced with nuint and some enums are typed as uint instead of the default. I'll regenerate with Windows before I undraft the pull request.
Perksey
Perksey3mo ago
hmmmm, that's also very annoying ideally the outputs don't change per OS I mean I know how C's integers work and it's crap but I really hope there's a way we can make ClangSharp not do that I think typemapping should work okay found it https://github.com/dotnet/ClangSharp/issues/574
Exanite
ExaniteOP3mo ago
Interesting
Perksey
Perksey3mo ago
so yeah basically if you typemap the stdint typenames to the correct C# types, that issue should be solved as for the dangling constants, no clue, let's try updating our ClangSharp versions
6) For MixKhronosData, should I use UseExtensionVendorTrimmings: None, All, or KhronosOnly? I currently don't specify the property at all.
KhronosOnly, but double check what OpenGL is using.
Exanite
ExaniteOP3mo ago
This is what the config for OpenGL looks like
No description
Perksey
Perksey3mo ago
oh, weird, then I guess don't specify it also? lol. UseDataTypeTrimmings should be excluded from Vulkan though as that's not part of Vulkan's function naming conventions (that's the glFunctionNameui64v crap)
Exanite
ExaniteOP3mo ago
Should I use KhronosOnly still (and maybe open a PR for OpenGL)? Also, should these mappings be done globally or just for Vulkan?
Perksey
Perksey3mo ago
probably just for vulkan for now although it maybe wouldn't hurt to have them in a common rsp that is explicitly pulled in by vulkan
Exanite
ExaniteOP3mo ago
How about UseExtensionVendorTrimmings?
Perksey
Perksey3mo ago
honestly I find it so strange that OpenGL isn't using it that must be for a reason... will look through commit history
Exanite
ExaniteOP3mo ago
Updating ClangSharp doesn't seem to affect anything
No description
Exanite
ExaniteOP3mo ago
I can debug later and see where they are getting added from though
Perksey
Perksey3mo ago
never used it, so strange... I guess maybe leave it out, I have a sneaking suspicion I implemented the extension vendor trimming and then realised it was a terrible idea as loads of functions ended up conflicting
Exanite
ExaniteOP3mo ago
Yeah, that might be the case I can test with OpenGL real quick Fortunately this is something we can easily change later I know with Vulkan, this might be bad since there are functions that differ between the extension version and the core promoted version So likely safer to leave out
Perksey
Perksey3mo ago
yeah I think that's likely the conclusion I landed on as well so the dangling constants is definitely a ClangSharp bug (cc @tannergooding) but I'm not sure what's unique about our RSPs that could be causing it
tannergooding
tannergooding3mo ago
what do you mean by "dangling constants"?
Exanite
ExaniteOP3mo ago
2) There are free floating properties at the bottom of https://raw.githubusercontent.com/dotnet/Silk.NET/20ef4637fdf5910ede171aa70676bfe2f4cd059b/sources/Vulkan/Vulkan/Vulkan/Vulkan.gen.cs. I haven't looked into why this occurs yet.
tannergooding
tannergooding3mo ago
probably some quirk with single file generation the ez fix is to not do single file generation /troll
Perksey
Perksey3mo ago
oh is that what's happening lol wait no because where are the structs haha can you remove the other mods so we can make sure this is raw clangscraper output?
Perksey
Perksey3mo ago
yeah, that's what I'm wondering what are we doing differently... well, for starters I guess we're shoving it all in one generator call
tannergooding
tannergooding3mo ago
if this isn't single file mode, then I'd guess it's not ClangSharp definitely still could be some weird edge case with XML or whatever but most of the time it comes down to someone using something like single-file generation, which is extra messy
Perksey
Perksey3mo ago
worth noting we're not using XML in this iteration of 3.0 SilkTouch is basically just a Roslyn syntax modding machine @Exanite in theory you can also crank up the logging to get the command line arguments we're passing into ClangSharp, and then try to replicate it yourself using the ClangSharpPInvokeGenerator dotnet tool
tannergooding
tannergooding3mo ago
The rsp I use for TerraFX are here, if you want to compare settings as well: https://github.com/terrafx/terrafx.interop.vulkan/tree/main/generation, following my own standard setup for shared rsp for common settings and then 1 per header file
Perksey
Perksey3mo ago
tbh I'm fine if we just take TFX.I.Vulkan's RSPs, but given that there's a lot of typemapping in them I'd probably want us to contribute some scripts to update the RSPs so we can have assurance that updates are relatively easy to do. but in any case it'd be good to get to the bottom of this so that if it is a ClangSharp bug we can file it/investigate it
tannergooding
tannergooding3mo ago
right, more just giving it as a reference for in case some --config switch differs here, for example I've got skate here in a bit, but if there is more found out happy to push a fix through on ClangSharp if needed
Perksey
Perksey3mo ago
no worries, have fun! will keep you updated.
Exanite
ExaniteOP3mo ago
https://raw.githubusercontent.com/dotnet/Silk.NET/e97a9a7e15839f4071b4cbc44978dca35823aa5d/sources/Vulkan/Vulkan/Vulkan/Vulkan.gen.cs Generated on Windows this time, but Linux also has similar output The free floating properties are still there I can test with ClangSharpPInvokeGenerator directly
Perksey
Perksey3mo ago
damn yeah so does feel like an upstream bug. can you paste the command line args that are logged here for reference? but yeah feel free to give it a go with upstream directly, should be the same result but we should be very worried if it's not
tannergooding
tannergooding3mo ago
these don't look floating actually? it just looks like indentation is borked
Perksey
Perksey3mo ago
hmm yeah, maybe it is worth running it against upstream as SilkTouch does try to prettify the files (although I'd have expected it to explode)
Exanite
ExaniteOP3mo ago
Oh there is a } at the end
Perksey
Perksey3mo ago
VK_NULL_HANDLE is tripping it up I think
tannergooding
tannergooding3mo ago
note that it messes up on the static ref readonly null here
[NativeTypeName("#define VK_NULL_HANDLE nullptr")]
public static ref readonly null VK_NULL_HANDLE {
[MethodImpl(MethodImplOptions.AggressiveInlining)]
get { null
} }
[NativeTypeName("#define VK_API_VERSION_1_0 VK_MAKE_API_VERSION(0, 1, 0, 0)")]
public const uint VK_API_VERSION_1_0 = ((((uint)(0)) << 29U) | (((uint)(1)) << 22U) | (((uint)(0)) << 12U) | ((uint)(0)));
[NativeTypeName("#define VK_NULL_HANDLE nullptr")]
public static ref readonly null VK_NULL_HANDLE {
[MethodImpl(MethodImplOptions.AggressiveInlining)]
get { null
} }
[NativeTypeName("#define VK_API_VERSION_1_0 VK_MAKE_API_VERSION(0, 1, 0, 0)")]
public const uint VK_API_VERSION_1_0 = ((((uint)(0)) << 29U) | (((uint)(1)) << 22U) | (((uint)(0)) << 12U) | ((uint)(0)));
then the property gets all off
Perksey
Perksey3mo ago
yeah, maybe add an exclude for that as we have nullptr anyway once we add TransformHandles
tannergooding
tannergooding3mo ago
I'd guess that something about the constant handling for a null type is breaking it and I do have VK_NULL_HANDLE excluded in which case this would be an issue with ClangSharp and it needing to special case such a null macro definition
Perksey
Perksey3mo ago
tbh I'm not really sure what ClangSharp is meant to do there other than not generating that particular macro gracefully
tannergooding
tannergooding3mo ago
skip it and spit out a warning
Perksey
Perksey3mo ago
yeah but fair enough, I think there are other cases of that so it's not the end of the world
tannergooding
tannergooding3mo ago
could also reasonably emit const nuint ... = 0 or const void* ... = null; etc, depending
Perksey
Perksey3mo ago
it's just it became very non-obvious once we ran CSharpier over the file lol
Perksey
Perksey3mo ago
yay!
tannergooding
tannergooding3mo ago
ok, so definitely an issue with VK_NULL_HANDLE if you could log an issue, then I'll get a regression test added and ensure that type of entry is skipped
Exanite
ExaniteOP3mo ago
Alright, I finished writing up the issue. I included more information in case someone hits the same issue later.
Exanite
ExaniteOP3mo ago
I think I have everything working except for the enums. I looked through the remaining mods that were available and added them (TransformHandles, TransformProperties, and ExtractNestedTyping). The issue where 32-bit and 64-bit enums are defined differently in the Vulkan headers seems fairly fixable. The enum members are currently being output as constants into Vulkan.gen.cs so it's a matter of finding those and moving those back. Screenshot 3 shows the output after I manually remove a few enum constants from the vulkan_core.h file. I added code to replace _THandle with _Handle and FlagBits with Flags in MixKhronosData.Trim (screenshot 1). This works perfectly for handles. For enums, the difference between Flags and FlagBits seems to cause the wrong type/name to be used inside structs (and probably also functions; haven't checked). This can be seen in screenshot 2 where ShaderStageFlags is not used correctly. I'm not sure how to cleanly fix this. I'm guessing we want some additional type mappings?
No description
No description
No description
Exanite
ExaniteOP3mo ago
We probably also want to remove the MaxEnum enum members
No description
Perksey
Perksey3mo ago
so structs should be using Flags, not FlagBits, but I suppose there’ll be exceptions here and there really MixKhronosData should be able to work it out, but I’m not sure what that’d actually look like in code - it’s doesn’t do any renaming itself so this isn’t tracked anywhere. I’ll have a think about this through the week and review at some point
Exanite
ExaniteOP3mo ago
I'll go ahead and experiment with a few ways of handling this and the other issues I've noticed
Exanite
ExaniteOP3mo ago
The XML does seem to have mappings between the FlagBits and Flags types
No description
Exanite
ExaniteOP3mo ago
Some other things I just noticed: Type aliases also exist. From what I can tell, Silk 2 for the most part has duplicate types for any aliased types. In Silk 3, these duplicate types don't exist. I'm guessing this is because ClangSharp knows to deduplicate them. Some enums aren't used aren't generated at all. I haven't looked into why yet. Edit: VkPrivateDataSlotCreateFlags technically isn't even an enum. It's simply typedef VkFlags VkPrivateDataSlotCreateFlags;. We have to rely on the XML data to properly handle this or assume that anything typedef'd to VkFlags is an enum. I'm guessing we want the NativeTypeName attributes to exist for everything if possible. They don't currently exist for things like the handle structs. In any case, I found the native type names extremely useful when looking up documentation in Silk 2. Having the attributes be properly generated for everything would be very nice. This goes for the other attributes as well.
No description
Exanite
ExaniteOP3mo ago
I'll keep reporting anything that I notice about the generated bindings here, mainly so we have a record of what needs to be done/investigated. I'll experiment with potential solutions as well. I took a look at VulkanMemoryAllocator's and SPIRV Reflect's headers. VMA is similar to Vulkan: VMA only uses the 32 bit flags and uses it with the typedef enum VmaAllocatorCreateFlagBits + typedef VkFlags VmaAllocatorCreateFlags pattern. VMA also uses the Flags typedef for storage. SPIRV Reflect does not use the same pattern as Vulkan: Reflect does not use VkFlags nor the second typedef. It just has typedef enum SpvReflectShaderStageFlagBits. I'm guessing we want to have the different Vulkan-related bindings be as consistent as possible, so the other Vulkan related libraries should be taken into consideration. Most notable is that I don't think we have access to an XML file for these APIs so we might not want to rely on the XML file for this information, unless we want to handle each API separately.
Perksey
Perksey3mo ago
ok so I'm happy with this VkPrivateDataSlotCreateFlags behaviour.
I'm guessing we want the NativeTypeName attributes to exist for everything if possible. They don't currently exist for things like the handle structs. In any case, I found the native type names extremely useful when looking up documentation in Silk 2.
this should be a fairly low-hanging fruit in TransformHandles so if we do make use of this, it'd be good to do so in an automated way. this could be having a python script that we run ahead-of-time (similar to what we do for OpenGL) to generate the RSPs, or it could be making MixKhronosData/PrettifyNames smarter. if we go the mod route, I reckon PrettifyNames probably needs to be changed to a two-pass system so that first we use some ISymbolNameSource (name to be bikeshedded) which will have some string GetManagedName(string original, string current), MixKhronosData will implement that and return *Flags for *FlagBits (please read my later message!). this probably also has the problem of the original typedefs mapping to VkFlags which map to uint, for which I think MixKhronosData's function transformer implementation will map those parameters into a Constant<uint, EnumNameHere> for OpenGL, this could be extended for Vulkan as well. this obviously doesn't cover structures, for which a similar mod to TransformFunctions will eventually exist (heck, you could start on that if you wanted) ...or you can just use the python script 😛 but the above is probably the golden way of doing it
VMA is similar to Vulkan: VMA only uses the 32 bit flags and uses it with the typedef enum VmaAllocatorCreateFlagBits + typedef VkFlags VmaAllocatorCreateFlags pattern. VMA also uses the Flags typedef for storage.
hmmmm, so I guess this presents a problem if we lean in too heavily on MixKhronosData to do FlagBits -> Flags. Maybe we should add the ability to do regex replacements on names for PrettifyNames?
SPIRV Reflect does not use the same pattern as Vulkan: Reflect does not use VkFlags nor the second typedef. It just has typedef enum SpvReflectShaderStageFlagBits.
awesome, so this will just work then. I recall the SPIRV-Tools generator.json configurations for BuildTools being super basic, we basically just ran it through as-is without any special configs. come to think of it, given that VMA uses the same pattern we probably want a mod that doesn't rely on khronos metadata to do uint -> *Flags I'm not sure what the best form for that is. maybe we can build on the regex replacements... a RemapNativeNames mod, which may have configuration but can also have INativeNameRemappers sourced as an IJobDependency<T>, initially RemapNativeNames would inject the remappings into the RSPs and clear its remapping dictionary, but anything that is added between ClangScraper and RemapNativeNames running (e.g. from PrettifyNames which would have an option on its regex replacements to include it as a native name to be remapped and would implement INativeNameRemapper) it would look through the NativeTypeNames itself and do the remappings itself. if there are no remappings then it does nothing 😄 none of this necessarily forms a good idea btw, this is just live train of thought
Exanite
ExaniteOP3mo ago
Reading through your messages right now I tested with using --remap in the RSPs to map the FlagBits types onto the Flags types and it seems to work well: https://github.com/dotnet/Silk.NET/pull/2457/commits/e5f1edd8fd539e6accce79a7f7c0a23ca9d09289 The RSPs look like this:
--remap
VkFramebufferCreateFlagBits=VkFramebufferCreateFlags
VkRenderPassCreateFlagBits=VkRenderPassCreateFlags
VkSamplerCreateFlagBits=VkSamplerCreateFlags
...
--remap
VkFramebufferCreateFlagBits=VkFramebufferCreateFlags
VkRenderPassCreateFlagBits=VkRenderPassCreateFlags
VkSamplerCreateFlagBits=VkSamplerCreateFlags
...
And are generated by scraping the XML (currently by printing the mappings to the console output in MixKhronosData)
// ...
var mapFrom = typeElement.Attribute("requires")?.Value;
var mapTo = typeElement.Element("name")?.Value;
// ...
Console.WriteLine($"{mapFrom}={mapTo}");
// ...
var mapFrom = typeElement.Attribute("requires")?.Value;
var mapTo = typeElement.Element("name")?.Value;
// ...
Console.WriteLine($"{mapFrom}={mapTo}");
Of course, this has the problem of being dependent on the XML. Good thing is that I don't think VMA has too many VkFlags enums. I also tried using IResponseFileMod to inject the mappings into the RSPs, but modifying PInvokeGeneratorConfiguration was annoying since I can't use the with syntax to replace just one property. Should we add a intermediate record type that converts to and from PInvokeGeneratorConfiguration? To summarize your two approaches, the first is to map uint back to Flags, and the second is to have a Regex name replacer right?
Exanite
ExaniteOP3mo ago
The Regex name replacer is more generalized and is probably similar to what I was trying to do with IResponseFileMod. Yeah, I saw that and I was like, "ehhhh, I don't want to do that" It'll get messy if ClangSharp ever adds another property to that class as well
Perksey
Perksey3mo ago
but yeah my approach is essentially to have a dedicated mod for changing the types of symbols annotated with a particular NativeTypeName, either based on a configuration or as instructed by other mods. so: RemapNativeNames(IEnumerable<IJobDependency<INativeMappingSource>>>) (all names to be bikeshedded, as I've literally already changed one since I started talking) - the remappings would be determined by aggregating the mod config (if any) together with the job dependency outputs - first, it would try to inject it into the RSP as this is the cleanest way, by getting the remappings at the point IResponseFileMod is used - if the remappings used at the time of IResponseFileMod being used has changed by the time RemapNativeNames.ExecuteAsync is called (or vice versa, who knows... RemapNativeNames could be used without ClangScraper) then it will manually look at the source files and do the remappings and yeah PrettifyNames will have a regex rename feature with some flag to indicate whether the outputs should be fed into RemapNativeNames using INativeMappingSource that's currently my mental model for what this could look like anyway the uint back to *Flags thing only works if we rely on metadata or have a similar mod interaction, at which point we're better off doing the generic option above anyway
In Silk 3, these duplicate types don't exist. I'm guessing this is because ClangSharp knows to deduplicate them.
this probably will need to be MixKhronosData specific, essentially make that do the duplicating. ClangSharp isn't generating the duplicates because the duplicates don't exist. e.g. when VkPhysicalDeviceFeatures2KHR was promoted to VkPhysicalDeviceFeatures2, VkPhysicalDeviceFeatures2KHR just became typedef VkPhysicalDeviceFeatures2 VkPhysicalDeviceFeatures2KHR; this form of promotion mechanism isn't something I'd expect to see outside of a Khronos binding, and if it does happen then the author of the binding should write a dedicated mod for that API's promotion/aliasing mechanism also I'm concerned about having MixKhronosData knowing about _THandle, ideally it should be just trimming _T and not really know about the TransformHandles mod - this may involve reordering.
Exanite
ExaniteOP3mo ago
So in other words, the duplication is desired I'm guessing the thought process behind this is that not duplicating the types would lead to types disappearing as extensions get promoted, which would be a breaking change
Perksey
Perksey3mo ago
yeah exactly (although, we do have the consideration that if a struct contains a field that references an enum that then gets promoted, this is breaking still, but 2.X has this issue as well so I'm not that worried about it)
Exanite
ExaniteOP3mo ago
So something I just realized now is that I don't think I ever knew where the _T suffix comes from. I don't see it in the Vulkan headers or XML. From what I can tell, Handle comes from the TransformHandles mod.
Perksey
Perksey3mo ago
it comes from VK_DEFINE_DISPATCHABLE_HANDLE typedef struct VkInstance_T* VkInstance;
Exanite
ExaniteOP3mo ago
I see, so the macro adds it
Perksey
Perksey3mo ago
yep we love macros!!!!1111!!1!11!1😭😭😭😭
Exanite
ExaniteOP3mo ago
I'm not sure how to replace _T without looking for _THandle. The data that currently is processed by MixKhronosData already has the Handle added as a suffix, so looking for _T as a suffix won't work. Globally replacing _T also won't work. The format of the log messages here are original, current, [previous separated by commas].
No description
Perksey
Perksey3mo ago
this is presumably because TransformHandles precedes PrettifyNames if you move TransformHandles to after PrettifyNames it should work
Exanite
ExaniteOP3mo ago
Hmm, I see I feel like ideally PrettifyNames should happen at the very end. In other words, work strictly with native names where possible.
Perksey
Perksey3mo ago
the ordering of the mods is kind of arbitrary, but there are some assumptions that will be made by each mod regarding its inputs which will inevitably place constraints on the order of the mods. This is normal and expected, each mod represents one discrete logical step in the syntax transformation. I agree that PrettifyNames makes sense to be nearer the end so that the mods that need the original names can deal in them easily, but right now there aren’t many constraints that I know of (aside from MixKhronosData) there are on that
Exanite
ExaniteOP3mo ago
Having to understand these assumptions also makes maintenance more difficult If I move TransformHandles after, the first question someone will have is why TransformHandles isn't with the rest of the Transform mods
No description
Perksey
Perksey3mo ago
this is true, but this can be solved using docs (or moving transform handles to after for all of the mods, again I don’t think there are any issues here). Ultimately this comes back to there not being any docs on the way of working with SilkTouch, which is entirely my fault and is something we want to rectify (and I’m talking about actual user docs here, not just for-contributors docs) I’d imagine the biggest factor in your feeling here is probably aesthetics - it feels more right to have the transform mods together and it may not be obvious to a reader why that isn’t the case. but this is a case study im going to come back to whenever i do write docs
Exanite
ExaniteOP3mo ago
Yup, that would be part of it The other is that making the codebase work the way someone would intuitively expect also makes things easier to understand (even without docs) and makes mistakes harder to make The more exceptions there are, the easier mistakes are to make If the general guideline is to put Transform mods together and put PrettifyNames at the end, then it's easy to have everything be correct Additionally, it won't have the problem where we need to fiddle around with the mod order, which would potentially add more inconsistencies Of course, if making the config more consistent comes at the cost of making the codebase more inconsistent, then we have a tradeoff to make I'm guessing the difference between user docs and contributor docs is that users are the ones using the generator, while contributors are the ones modifying the generator source code and writing the actual mods
Perksey
Perksey3mo ago
my main gripe with _THandle trimming is that it is trading one assumption with another much more specific assumption _T assumes ClangScraper has run, whereas _THandle assumes that PrettifyNames has run (presumably with the output of ClangScraper) just one assumption is hardcoded, and the other can be worked around through mod reordering
Exanite
ExaniteOP3mo ago
I'm fine with changing that part My concern is more with how mods are designed in general
Perksey
Perksey3mo ago
yeah exactly, for-contributors is basically where you have the code in front of you, an assumption that isn’t typically made of users. It’s my hope that SilkTouch will eventually be consumed as a dotnet tool or similar
Exanite
ExaniteOP3mo ago
The underlying problem is that MixKhronosData isn't getting the native name It's getting the name after TransformHandles has modified it, which changes how the decision making needs to be done (I'm testing with reordering the mods right now)
Perksey
Perksey3mo ago
it’s worth noting MixKhronosData is simply just another rule that PrettifyNames uses to trim names it’s not actually about where MixKhronosData is named in the pipeline so really it’s PrettifyNames gets the name after the preceding mods has modified it, which seems correct it’s just a bit weird because you have this trimming rule that only works properly if you haven’t gone and renamed it to something else using another mod which… seems fine and can be reasoned for in docs I agree it’s a complicated interaction and originally mods weren’t supposed to have interactions like this, every mod was originally going to be isolated and communicate only through syntax, but this was very inefficient and there does have to be some sharing involved if we are to keep the step-by-step nature (otherwise some things will end up needing to be multi-pass, which would skyrocket generation times and worsen user experiences)
Exanite
ExaniteOP3mo ago
An issue I'm noticing with moving TransformHandles after PrettifyNames is that the handles won't exist at all from PrettifyName's perspective
Perksey
Perksey3mo ago
yep that is certainly a tradeoff but it’s one to be expected if every mod considers its own inputs, the input simply doesn’t exist at that point but on the plus side, the transform handles mod will have the prettified name waiting for it as its input, so it’ll still achieve the end result another way to look at it is that in no circumstances can any part of the pipeline see into the future
Exanite
ExaniteOP3mo ago
PrettifyNames gets its input by parsing the C# syntax tree right? After that, the names are then passed to each INameTrimmer to be modified. When PrettifyNames runs, the handles don't exist as C# code at all
Perksey
Perksey3mo ago
yes that’s correct the handles don’t exist, that’s something that happens in the future but the handles will be created from the names that have already been prettified I.e. from the output from previous mod in the pipeline it will see an empty struct called Instance and generate a handle called InstanceHandle correction: it might actually see just a struct VkInstance_T* NativeTypeName in which case we have problems
Exanite
ExaniteOP3mo ago
I think the problem is that PrettifyNames doesn't parse parameter type names (and I think struct member type names), which is the only place the handles exist prior to the TransformHandles mod. INameTrimmer doesn't see VkPhysicalDevice_T at all right now. This means we can fix this by parsing all places where a type name can exist.
Perksey
Perksey3mo ago
I forgot about that
Exanite
ExaniteOP3mo ago
No description
Perksey
Perksey3mo ago
yeah that literally just hit me lmao fuck maybe we need to add more stuff to NativeTypeName so that we can go back to the native type name, trim it, and then reapply any modifications to the name i.e. PrettifyNames can undo any name modification by looking at NativeTypeName, trim it, and then reapply the modifications e.g. a Suffix = “Handle” on NativeTypeName but probably something less dumb I completely forgot that the non-transformed structs literally don’t exist, my bad
Exanite
ExaniteOP3mo ago
You're all good I'm not sure how I feel about undoing previous mods though
This means we can fix this by parsing all places where a type name can exist.
I think this should be the solution If we can guarantee all names are prettified, then any modifications to those names should be fine after that Prettifying and modifying names aren't necessarily the same process In other words, the underlying issue is that modifying a non-prettified name gives a bad result
Perksey
Perksey3mo ago
this isn’t really undoing though, rather just tracking the changes it might be that the previous mod does a rename that we want to keep even if we prettify the name we don’t want prettify names to just discard that action by looking at the native name e.g. PrettifyNames can see that VkInstance_T is now VkInstance_THandle and that we probably want to keep that Handle suffix, so the NativeTypeName needs to somehow instruct the PrettifyNames mod what to do I think an alternative is likely turning on the ClangSharp option that generates empty structs and then the mod reordering would just work tbh I don’t remember why I made TransformHandles look at the NativeTypeName anyway, I think it was “””resilience””” in case that option wasn’t on headed to bed now
Exanite
ExaniteOP3mo ago
Have a good night! I'll keep thinking about this more My own game engine is designed around being highly moddable so I'm seeing a few parallels in design here I'm looking into this right now Any pointers on how to do this? I'm guessing it's related to
-wts, --with-transparent-struct <with-transparent-struct> A remapped type name to be treated as a transparent wrapper during binding generation. Supports wildcards. []
-wts, --with-transparent-struct <with-transparent-struct> A remapped type name to be treated as a transparent wrapper during binding generation. Supports wildcards. []
Perksey
Perksey3mo ago
removing this setting would likely do it
Exanite
ExaniteOP3mo ago
Ah
Perksey
Perksey3mo ago
I'm not sure how best to do it in a way that isn't terrible for all the other bindings maybe a common.rsp and a core.rsp, where the former includes the latter idk
Exanite
ExaniteOP3mo ago
Maybe a mod that removes it?
Perksey
Perksey3mo ago
nah seems excessive for a mod given that it's possible to achieve what we want with built-in functionality come to think of it the NativeTypeName recognition of TransformHandles... kinda feels like maybe it should be in ExtractNestedTyping instead? creating the empty types I mean (with a configuration option) that is another way around it
Exanite
ExaniteOP3mo ago
That might make sense I'll look into how those mods work
Perksey
Perksey3mo ago
btw all that commented code I think can be deleted since this commit not sure why it's still in there, and I just double checked and it certainly looks obsolete (the commit linked is replacing it with something else)
Exanite
ExaniteOP3mo ago
You're talking about VisitFunctionPointerType right?
Perksey
Perksey3mo ago
yep it was a bunch of stuff ported from 2.X wrt anonymous function pointer handling but it was the same code that generated type names like PfnVvVUi64V and I guess it finally got to me what I replaced it with in that commit is far more elegant and basically just infers a name from the usage
Exanite
ExaniteOP3mo ago
Still reading through the relevant code. I should understand TransformHandles enough to work with it now though. I also added a commit adding some comments to TransformHandles. From what I can tell, TransformHandles identifies an existing struct to be a handle struct by checking if the existing struct is empty AND if all references to it are pointers, right?
Perksey
Perksey3mo ago
correct
Exanite
ExaniteOP3mo ago
That means just having ExtractNestedTyping generate an empty struct with the correct name is enough for TransformHandles to pick it up and rename it/add the relevant members to it.
Perksey
Perksey3mo ago
yep that's the plan i.e. it's extracting the AssumeMissingTypesOpaque functionality
Exanite
ExaniteOP3mo ago
Alright, that sounds very doable
Perksey
Perksey3mo ago
nice work finding your way around the code btw 😄 it's reassuring that someone other than curin and I can do so
Exanite
ExaniteOP3mo ago
Mind if I add comments where applicable? A lot of it has been reading the comments, then answering remaining questions by using the debugger Any questions that I have to answer by using the debugger or looking elsewhere should have a comment lightly explaining it
Perksey
Perksey3mo ago
not at all, please comment away!
Exanite
ExaniteOP3mo ago
I got ExtractNestedTyping to generate the empty handle structs This is with the following mods:
"AddIncludes",
"ClangScraper",
"AddApiProfiles",
"MixKhronosData",
"ExtractNestedTyping",
"AddIncludes",
"ClangScraper",
"AddApiProfiles",
"MixKhronosData",
"ExtractNestedTyping",
No description
Exanite
ExaniteOP3mo ago
Issue is that TransformHandles no longer detects these handles in TransformHandles.Rewriter.VisitStructDeclaration()
No description
Exanite
ExaniteOP3mo ago
No description
Exanite
ExaniteOP3mo ago
The second condition in the if statement is now returning false
Exanite
ExaniteOP3mo ago
No description
Exanite
ExaniteOP3mo ago
The difference from before I made the change to ExtractNestedHandles is that the value of the KeyValuePair of scopes contained the namespace (Silk.NET.Vulkan). Now it is not. This is what I understand TransformHandles.TypeDiscover.GetHandleTypes() to be doing. The xml docs here were added by me.
/// <summary>
/// Gets all discovered handle types, and optionally, any missing handle types.
/// </summary>
/// <param name="missingFullyQualifiedTypeNamesToRootNodes">If not null, this dictionary will be populated with syntax nodes for each missing handle type.</param>
/// <returns>
/// Dictionary containing discovered handle types.
/// The keys are the names of the discovered handle types.
/// The values are the fully qualified names of the types that reference that handle type and the namespace of those types.
/// </returns>
public Dictionary<string, Dictionary<string, string>> GetHandleTypes(
Dictionary<string, SyntaxNode>? missingFullyQualifiedTypeNamesToRootNodes
)
/// <summary>
/// Gets all discovered handle types, and optionally, any missing handle types.
/// </summary>
/// <param name="missingFullyQualifiedTypeNamesToRootNodes">If not null, this dictionary will be populated with syntax nodes for each missing handle type.</param>
/// <returns>
/// Dictionary containing discovered handle types.
/// The keys are the names of the discovered handle types.
/// The values are the fully qualified names of the types that reference that handle type and the namespace of those types.
/// </returns>
public Dictionary<string, Dictionary<string, string>> GetHandleTypes(
Dictionary<string, SyntaxNode>? missingFullyQualifiedTypeNamesToRootNodes
)
The return value of GetHandleTypes() is this dictionary, which is the same as the handles variable in VisitStructDeclaration:
var typeNameToScopesAvailableToDeclaringScope =
new Dictionary<string, Dictionary<string, string>>();
var typeNameToScopesAvailableToDeclaringScope =
new Dictionary<string, Dictionary<string, string>>();
I'm probably misunderstanding what the nested dictionary (Dictionary<string, string>) in typeNameToScopesAvailableToDeclaringScope = new Dictionary<string, Dictionary<string, string>> does.
Perksey
Perksey3mo ago
so this is basically in cases where we have missing types we try to figure out a good namespace to put the type in so most of this should've moved to ExtractNestedTyping or equivalent functionality implemented at that point the walker just needs to visit all structs, and record the fully qualified names of the ones that are empty (across all parts, if we didn't care about partials, this would be single-pass and we wouldn't need the walker), for the rewriter to then act on the code to implement this is a bit bonkers as it stands
Exanite
ExaniteOP3mo ago
For context, I currently have ExtractNestedTyping and TransformHandles both using the TypeDiscoverer from TransformHandles. They each do their own separate passes with the TypeDiscoverer. This was to save time while experimenting. I'm also not sure if I want to merge the code of the two walkers together in ExtractNestedTyping. This means that the handle discovering logic is basically duplicated at this point.
Perksey
Perksey3mo ago
I would keep the walkers separate as they should be doing different things
Exanite
ExaniteOP3mo ago
That sounds good
Exanite
ExaniteOP3mo ago
Assuming ExtractNestedTyping outputs all relevant handle types as empty structs (as it does now in the screenshot below), how should TransformHandles identify handle types?
at that point the walker just needs to visit all structs, and record the fully qualified names of the ones that are empty (across all parts, if we didn't care about partials, this would be single-pass and we wouldn't need the walker), for the rewriter to then act on"
I assume this is all we need to do? Should we check if these empty structs are only referenced through pointers?
No description
Perksey
Perksey3mo ago
how should TransformHandles identify handle types?
- only used as a pointer of at least one dimension - has no fields or properties in any of the declaration parts ah right okay I think I know why this is so weird
Exanite
ExaniteOP3mo ago
So effectively the same as before? This would mean the walker logic is shared between ExtractNestedTyping and TransformHandles
Perksey
Perksey3mo ago
this is essentially trying to ensure the rewriter understands when a given type is available i.e. if Silk.NET.A contains SomeStructA that is empty but Silk.NET.B contains SomeStructA that is not empty and therefore not a handle, we only change the references to SomeStructA where we know it's Silk.NET.A.SomeStructA being referenced this was before SilkTouch was given access to the symbol model and should now probably just use the renamer
Exanite
ExaniteOP3mo ago
Does the renamer use symbols?
Perksey
Perksey3mo ago
yeah it does, it uses Roslyn's built-in search engine to find references to a given symbol the bad news is I think on develop it's still integrated into the bakery, on @Curin's branch we factored it out as it's very useful
Exanite
ExaniteOP3mo ago
Gotcha That definitely simplifies things
Curin
Curin3mo ago
Yeah, that isn't submitted yet. I'll be getting back to it probably starting tomorrow.
Perksey
Perksey3mo ago
I think if we're super confident in where we're taking the Win32 bindings, we should probably look to get that PR closed out soon and the rest of improvements done in separate PRs (and we can move Win32 bindings to be a blocker for Preview 1 to ensure we do actually do that) if we're confident that is. I think we are though. there are some experiments I want to try wrt alternative ways to generate stuff but nothing that should materially destructively influence how the bindings are output. actually tbh I didn't realise the renamer was so small https://github.com/dotnet/Silk.NET/pull/2406/files#diff-4d93e56d6d07a37ac3577a7fa2dfd0f0649e9ac07912045809c723351c9be56c you could just cherry-pick this
Curin
Curin3mo ago
yeah, it should be a few quick fixes, but things happen and I could end up down rabbit holes.
Perksey
Perksey3mo ago
what do you mean, that hasn't happened once since you started on the win32 bindings :when:
Exanite
ExaniteOP3mo ago
I'm guessing the hash in the url is the relevant commit hash (the diff currently shows all commits for me). I can take a look at that in a bit
Perksey
Perksey3mo ago
it's meant to jump to NameUtils.cs on the files changed tab but it is a huge PR I really hope that someone one day makes a git client using the same stuff that the Zed editor uses
Exanite
ExaniteOP3mo ago
Going back to VisitStructDeclaration, the code inside the if statement renames the Handle struct and generates the Handle struct members. I'm trying to better understand what the if statement condition fully does. Here's what I understand: 1. It does not check whether the struct is empty. 2. It checks if the struct's name is the name of a previously identified handle struct 3. It checks if the struct's namespace is the namespace of a previously identified handle struct 2 and 3 are required to fully verify that the struct is a handle struct.
No description
Exanite
ExaniteOP3mo ago
I'm not sure why scopes currently has the value it has though. From my understanding Value should be Silk.NET.Vulkan
No description
Perksey
Perksey3mo ago
so yeah that's currently all happening in the first pass, and the first pass logic just looks completely broken for anything but cases where the struct doesn't exist
Exanite
ExaniteOP3mo ago
Gotcha, so it is intended that Value is Silk.NET.Vulkan (for both cases: struct already exists and struct does not yet exist)? I can look into fixing it if this is the case (and if the renamer doesn't change anything significant here)
Perksey
Perksey3mo ago
ahhhh ok I see what this is doing it's saying "if we know a handle with this struct's name exists, and the namespace of this struct declaration we've encountered is one we've recorded as containing the declared handle-eligible struct, then we rewrite it" which is a bit wordy, but essentially Name of struct referenced 1-------* Scope in which the struct is referenced 1-------1 The declaring namespace of the type referenced but yeah, using the renamer negates the need for a lot (but not all) of this in ExtractedNestedNames I believe we will still need to do the manual reference tracking because the types won't exist but by the time we get to TransformHandles we'll have the types which means we can operate purely in symbol land
Exanite
ExaniteOP3mo ago
The renamer is for this line right? var self = $"{node.Identifier}Handle";
Perksey
Perksey3mo ago
yes GetHandleTypes: 1. Finds all references to all type names 2. Removes the references that we can resolve to a type naively. 3. Generates the empty structs for the references that couldn't be resolved I suppose as a little experiment you could maybe look at what the symbol model looks like for unresolved types if this is workable it will be easier and more robust if there is a way to get a compilation, and identify missing references, this will make your job a lot easier. then it's just a matter of making sure all of those missing references are pointers, and then generating the missing type. otherwise most of GetHandleTypes will need to stay in ExtractNestedTyping
Exanite
ExaniteOP3mo ago
Since ExtractNestedTyping now generates all of the empty handle structs, should TransformHandles assume that all of the handle structs exist? (Also, quick tangent: this does successfully allow MixKhronosData to trim the _T suffix without looking for _THandle. This is how we got here in the first place.)
Perksey
Perksey3mo ago
TransformHandles at this point should: 1. Get a compilation 2. Identify the struct symbols that have no fields (autogenerated or otherwise) 4. At this point, run the renamer to change usages of X* to XHandle for all of those identified symbols (the reason we do this here is so that we only create a compilation in this mod once, running the renamer after we've modified the files will result in two compilations: one for the symbol search, and one for the renamer) 5. Run the syntax rewriter to generate the additional members as they are today. Now okay, I take it back. The underlined portion introduces a LOT of complexity. I completely forgot that this mod isn't just renaming, it's removing one pointer dimension. If we want to do this properly using the symbol model, this does become challenging. Essentially, right now the renamer looks for any references of a symbol and changes those referencing tokens. What I'd like to do is make it so we can tell it to remove that pointer dimension as well as part of the rename operation, so there's only one pass for this and we don't need the rewriter to scan over the entire codebase to find references (this is what we're trying to avoid by using the symbol model in the first place!) to remove that indirection level itself. No idea what the API for this looks like though.
I suppose as a little experiment you could maybe look at what the symbol model looks like for unresolved types
oh btw for this I just came across IErrorTypeSymbol in the Roslyn documentation so ExtractNestedTyping should: 1. Get a compilation 2. Identify the IErrorTypeSymbols, and find references of those symbols. 3. If all references are in a pointer, then the namespaces of the code that references this non-existent type should be recorded. 4. Once recording the references, the empty struct is generated in the common prefix of those namespaces.
Exanite
ExaniteOP3mo ago
Gotcha, so it's effectively a rewrite of the existing code
Perksey
Perksey3mo ago
yep :/ FWIW this does tend to happen in this generator codebase quite a bit, but that's mainly because we want to get it right
Exanite
ExaniteOP3mo ago
Yeah, that sounds like a lot, but from my small amount of experience working with the symbol model from when I was working with source generators, the symbol model does help out a lot
Perksey
Perksey3mo ago
we could go the BuildTools approach and just keep bashing until it does what we want, but I don't think any of us are satisfied with that codebase for that reason and also I don't think it's necessarily a lot of code, again the symbol model is very helpful, but it's a lot to keep in your head when writing that code
Exanite
ExaniteOP3mo ago
And don't worry, I'm still willing to attempt this
Perksey
Perksey3mo ago
awesome, thank you, you are a star honestly SilkX started pretty much exactly two years ago and is the clearest we've ever been about a path forward for 3.0 but it has taken so long because we don't have enough people like you Silk.NET 1.0 was written when I was in high school (and therefore I lots and lots and lots of free time) and we also had lots of contributors. 2.0 was mostly written when I was in sixth form and therefore still had lots of free time. less contributors this time around but Kai was worth several people with how much they did. whereas I have been full-time employed for pretty much all of 3.0's development.
Exanite
ExaniteOP3mo ago
I'm glad I'm able to help It's also nice to see that there are other contributors working on Silk 3, like what we saw yesterday
Perksey
Perksey3mo ago
absolutely, it's great to see people starting to crawl out the woodwork. part of the reason not happening before is absolutely me historically being crap at splitting up workload but I'm trying to get better.
Exanite
ExaniteOP3mo ago
On this note, I'm also interested with improving/refactoring/reworking the codebase For some context, I don't have professional experience and all my programming experience is from working on my own projects (edit: and reading through a lot of other people's code). However, I usually work on projects designed for the long term (such as my personal game engine) and those projects tend to become quite large over time, which means that I spend quite a bit of time focusing on maintainability. I'm hoping that a more documented and easier to understand codebase will make it easier for new contributors to work on the Silk codebase
Perksey
Perksey3mo ago
100%, absolutely, my goal with 3.0 is to make it much more maintainable and contributable. I'm trying to reduce the project's reliance on my encyclopaedic knowledge of it, and this is why in the SDP we discussed having things like for-contributors documentation and other much more rigourous processes around how we write code (but as you've found, we haven't done a lot of it yet)
Exanite
ExaniteOP3mo ago
One thing I plan on doing is reading through all of the questions/comments/docs/notes that I've asked/written and seeing how those can be addressed through documentation or codebase refactorings I do have some knowledge from reading through Silk 2's codebase, but I more-or-less started with no context on Silk 3 so what I went through should roughly model what a new contributor would go through
Perksey
Perksey3mo ago
sounds like a plan. in terms of refactoring, I'm always open to it but always want to make sure it's for a good reason. there are a lot of parts of the current codebase that does generate questions, but I don't think they're unanswerable or have answers that don't make sense. some of the code is indeed bollocks like what we've encountered today, but we've got a good path forward for reworking that
Exanite
ExaniteOP3mo ago
Yeah, and that's something that differs with a real world project compared to my personal projects I'm used to refactoring something whenever I see it, provided I have a good solution to replace the existing code with I think the key thing to take note and design around are "boundaries". From what we discussed today, one such boundary is when the symbolic model can/should be used. Being able to set general guidelines without any exceptions makes it a lot easier to understand the codebase. That's why I was concerned about moving TransformHandles to be after PrettifyNames previously. This meant that not all of the "Transform" mods were together and raises the question of why they weren't. Sure this is answerable, but it's also a question that will likely be continually asked.
Perksey
Perksey3mo ago
but it's also a question that will likely be continually asked.
yep absolutely, and in these cases it's always good to get it documented, so I think you're absolutely on the right wavelength.
Exanite
ExaniteOP3mo ago
Another boundary is with API (eg: Vulkan) specific code. I'm personally fine with writing Vulkan specific code and sticking that in a TransformVulkan mod given that the TransformVulkan mod gets refactored once functionality from that mod is required for other APIs. I want to avoid config settings that get enabled for one API and disabled for another API without an obvious reason. Sticking these exceptions in a API-specific mod helps document these exceptions.
Perksey
Perksey3mo ago
yeah definitely, where there is code that has no conceivable application outside of a particular binding, then yes that should not be a core part of a core mod like no non-vulkan-specific mod should have a "vulkan setting"
Exanite
ExaniteOP3mo ago
MixKhronosData is weird when it comes to this though It makes sense for the most part though and can be refactored in the future Well, at least until the generator config itself becomes public API As for next steps, I think I'll start rewriting the handle part of ExtractNestedTyping to do what you wrote here. This should be doable without the new renamer. I'll leave the refactorings for later, and of course, I'll discuss them before going ahead with them. The ideas above are my key focus though. Any recommendations on how to find references to IErrorTypeSymbol? I'm currently looking into SymbolVisitor
Exanite
ExaniteOP3mo ago
It looks like I can do some parsing to look through each method parameter (for example), get their type, unwrap any IPointerTypeSymbols, and check if the PointedAtType is an IErrorTypeSymbol
No description
Perksey
Perksey3mo ago
SymbolFinder.FindReferencesAsync and then for each reference, check that the parent is a PointerTypeSyntax although, yeah maybe staying in symbol land is the way to go eeeeh idk try both, see which is faster
Exanite
ExaniteOP3mo ago
Both as in the SymbolVisitor approach and the SymbolFinder approach?
Perksey
Perksey3mo ago
SymbolFinder will be less code and hopefully shouldn't result in speed drops as in theory all of the syntax trees are realised already actually yeah just use SymbolFinder, my mistrust is misplaced
Exanite
ExaniteOP3mo ago
Alright, I'll see what I can do Oh it looks like FindReferencesAsync is useful for the next step. I'm trying to find the instances of IErrorTypeSymbol right now
Perksey
Perksey3mo ago
for even less code you could probably do FindReferencesAsync with the IErrorTypeSymbol and FindReferencesAsync with Compilation.CreatePointerTypeSymbol(errTypeSymbol) and just compare the number of references but that may be slower than doing the first FindReferencesAsync and then looking at the syntax (as you're looking over the entire compilation twice maybe, not sure how Roslyn implements this) lots of different ways to implement this with potential varying speeds! the sanest for now is probably FindReferencesAsync then look at syntax
Exanite
ExaniteOP3mo ago
I'm still confused on how to get the symbol itself. FindReferencesAsync assumes that I already have the symbol so I can find all other references to it. Currently, the issue is that I don't know which types are missing, which is why I need to find all instances of IErrorTypeSymbol. If I use SymbolVisitor to find the missing types, I should get enough information to not require SymbolFinder at all
Perksey
Perksey3mo ago
good point yeah just use SymbolVisitor then as you'll be able to check whether the parent is a IPointerTypeSymbol at that point RenameAllAsync should work with the IErrorTypeSymbol as that also uses SymbolFinder under-the-hood (obviously recall that we want to change RenameAllAsync to be able to change Name* to NewName i.e. NameHandle, I'd imagine that would be changing the token splicing logic to cover the full TextSpan as denoted by the ReferenceLocation)
Exanite
ExaniteOP3mo ago
I'm using this in the DefaultVisit to traverse down the tree, but this doesn't include method parameters.
foreach (var member in symbol.Members())
{
Visit(member);
}
foreach (var member in symbol.Members())
{
Visit(member);
}
I can handle the method parameter case or any other case I notice pretty easily, but I'm worried that I miss some cases where IErrorTypeSymbol might exist.
Perksey
Perksey3mo ago
yeah it certainly seems like you'll have to switch on member apparently the reason why they never made a SymbolWalker is because the symbol model is a graph structure, and you'd never know where to terminate
Exanite
ExaniteOP3mo ago
Ah, I was wondering why it didn't exist I guess the best we can do is try to cover all cases I find it weird that there's no way to just iterate through all existing symbols though
Perksey
Perksey3mo ago
i don't think you can guarantee the symbols exist until you do any actions although ALTHOUGH something more efficient here you could look at the compilation errors see which ones are for unknown type names and then look at the syntax location to which the error pertains and see whether the parent is a pointer syntax if it is, create a semantic model for the node and get the symbol for the node (which will be an IErrorTypeSymbol) and shove it in a dictionary I guess and if you later encounter an error where the parent is not a pointer syntax, set the dictionary value to null and then you just RenameAllAsync with the non-null values ok scratch that I can't for the life of me figure out how to get the errors lol oh GetDiagnostics nvm
Exanite
ExaniteOP3mo ago
No description
Exanite
ExaniteOP3mo ago
There is certainly more than I was expecting though
Exanite
ExaniteOP3mo ago
No description
Exanite
ExaniteOP3mo ago
These should be easy to filter out though
Perksey
Perksey3mo ago
oh uhhhh what have you referenced Silk.NET.Core in your csproj
Exanite
ExaniteOP3mo ago
No description
Perksey
Perksey3mo ago
wtf ummm oh I know it's becuase the file that does global using Silk.NET.Core is nuked as part of the mod context creation LOL this line maybe it should be changed to only include the files in the directory we'll be outputting to (same with line 123) the goal is for SilkTouch to have a valid but empty project i.e. contains all the references and other magic that you'd get in a blank csproj, so the compilation is accurate to the environment in which the code will ultimately be compiled
Exanite
ExaniteOP3mo ago
Okay I see what's going on. Do we want to do this? Doing this makes sense to me.
Perksey
Perksey3mo ago
yep agree heck maybe it should even be only removing .gen.cs files actually nah, that will be chicken and egg .............................................actually no it'll be fine i was worried about manual files that reference the bindings for a second there but i don't think that's an issue
Exanite
ExaniteOP3mo ago
From what I understand, by including manually written files in the project source folder, it means that those files are now inputs to the generator. This also means the generator can modify them.
Perksey
Perksey3mo ago
hmmmmmmmmmmmmmmm yeah we'd also have to change the output writer to only consider .gen.cs not sure whether to make that call
Exanite
ExaniteOP3mo ago
The generator should be able to output compilable code regardless of whether the manually written files are included right? The manually written files depend on the generated code, not the other way around. So I think ignoring those manually written files during generation is fine
Perksey
Perksey3mo ago
good point, you're right i guess my main concern was the other way around but tbh that screams of a deficiency in SilkTouch more than anything else
Exanite
ExaniteOP3mo ago
And if we, for some reason, do depend on those files as inputs, we can add a way to include them explicitly as generator dependencies, such as through the generator.json config.
Perksey
Perksey3mo ago
yep that sounds like a good approach
Exanite
ExaniteOP3mo ago
I'm going to take a break for now, but I think I have all of the information I need Thanks for the help!
Perksey
Perksey3mo ago
no problem, thank you for keeping at it 😄
Exanite
ExaniteOP3mo ago
So parsing the diagnostics and finding the relevant symbols is apparently harder than I thought it would be GetDeclaredSymbol generally only works on Declaration syntaxes Here I get the syntax node (syntaxTree.GetRoot().FindNode(typeError.Location.SourceSpan)), then I search up for a node that I can call GetDeclaredSymbol on.
No description
Exanite
ExaniteOP3mo ago
These symbols aren't the IErrorTypeSymbols we want, but do contain them
No description
Exanite
ExaniteOP3mo ago
So we could use the diagnostics as an optimization to reduce the search area of the SymbolVisitor
Exanite
ExaniteOP3mo ago
Got it to work Data is all as expected
No description
Exanite
ExaniteOP3mo ago
I'm currently using the diagnostic type errors to narrow the search (this provides an initial set of symbols to search), then use the SymbolVisitor to find where the IErrorTypeSymbols are.
Perksey
Perksey3mo ago
niiice, this looks awesome!
Exanite
ExaniteOP3mo ago
I got the empty handle structs to generate again and also cleaned up the code a bit. I'll start working on the TransformHandles side of things. One thing to note is that the new TransformHandles will be a breaking change. This is because TransformHandles will no longer generate the empty handle structs. I don't think breaking the public API is an issue considering we're not even at preview, but it does mean the other binding configurations should be updated and potentially regenerated.
Exanite
ExaniteOP3mo ago
--- One thing with TransformHandles is that we probably should check again to see if the empty structs are only referenced through a pointer. This would be more correct, but we could also just assume that any empty structs are handle types rather than verifying again.
No description
Perksey
Perksey3mo ago
One thing to note is that the new TransformHandles will be a breaking change.
yep that's fine, this hasn't even shipped in preview yet and SDL is the only other binding currently that uses TransformHandles, which also uses ExtractNestedTyping beforehand
One thing with TransformHandles is that we probably should check again to see if the empty structs are only referenced through a pointer.
yes I agree
Exanite
ExaniteOP3mo ago
I got the symbol-based handle type discovery to work in TransformHandles
No description
Exanite
ExaniteOP3mo ago
I'll work on renaming and pointer dimension reduction next I copied in the symbol-based renamer earlier (NameUtils.RenameAllAsync) so I should be able to use that already Pointer dimension reduction should be doable by just reusing the existing code
Perksey
Perksey3mo ago
nice! great progress
Exanite
ExaniteOP3mo ago
I attempted to do the implement the remaining functionality for TransformHandles yesterday but ran into an issue where NameUtils.RenameAllAsync was not working due to the symbols being invalidated by earlier modifications. Apparently // Rewrite handle struct to include handle members and NameUtils.RenameAllAsync work fine together so not every change invalidates the symbols. I'm not sure what the exact rules are or if this behavior is reliable. The most correct fix would be to gather all information before making any changes and store it in a way that doesn't get invalidated. I can do this for the pointer dimension reduction since it only relies on strings. I don't know if I can do this for the other 3 steps (add handle members, add -Handle suffix, rename references). These all currently rely on Symbols. I'm also not sure when other data types get invalidated, such as DocumentId and Location. On a side note, RenameAllAsync should be split into two methods: a gather step and a modify step, similar to how the mods are implemented. Currently RenameAllAsync does both, which means that it's hard to do group all gathers/modifies together and that it is hard to reuse information, such as the Locations found by RenameAllAsync (if the Locations don't become invalidated; I'm not sure how that works yet). This can help with implementing pointer dimension reduction in a general way, along with any other modifications that need to affect all references of a symbol.
No description
Exanite
ExaniteOP3mo ago
Converting the Symbols into strings (fully qualified names) and converting back later might work. Haven't fully looked into this yet.
Perksey
Perksey3mo ago
hmmm, is your code uploaded? there’s probably a hidden mutation somewhere Roslyn is fully immutable so any mutation creates a clone of the everything (with some reused data structures behind-the-scenes to make this not eat RAM and perf)
Exanite
ExaniteOP3mo ago
Not fully since I'm testing a few things. The screenshot covers all of the important parts that are not pushed though. What I mean by invalidated is that searching for the symbols obtained from the non-modified project in the modified project seems to not work.
Perksey
Perksey3mo ago
yep that’s expected
Exanite
ExaniteOP3mo ago
Here's the diff if it helps The rest is pushed.
No description
Perksey
Perksey3mo ago
so really the RenameAllAsync should be the very first thing you do on the modification front or at least the verify first thing after walking the symbols (and operating directly on the compilation which you walked with no other modifications in between)
Exanite
ExaniteOP3mo ago
Wouldn't that break the other transformation steps though, since they also rely on the symbols? I know the reasoning for it, but I'm not sure when things get invalidated.
Perksey
Perksey3mo ago
wdym by other transformation steps
Exanite
ExaniteOP3mo ago
I can do this for the pointer dimension reduction since it only relies on strings. I don't know if I can do this for the other 3 steps (add handle members, add -Handle suffix, rename references). These all currently rely on Symbols.
These 4 steps (the three I pointed out and pointer dimension reduction)
Perksey
Perksey3mo ago
hmmmmm I guess really we need a "queued change" mechanism then sorry this has really spiralled either that or we can take the hit of walking again (not ideal)
Exanite
ExaniteOP3mo ago
The good thing is that (add handle members, add -Handle suffix) and (pointer dimension reduction, rename references) effectively work on independent sets of data. It's possible to do each group independently then merge the changes.
Perksey
Perksey3mo ago
hang on why is the symbol model involved for TransformHandles for anything other than the rename?
Exanite
ExaniteOP3mo ago
It's needed for identifying which of the structs are handle structs (must be empty and only referenced through a pointer).
Perksey
Perksey3mo ago
hmmm ok. could we maybe do the rename and the struct transformation as one? actually nah that'll probably be hellish to manage I was thinking maybe we could pass the list of reference locations to a rewriter, but I remembered that there were a lot of very special things required to get the token splicing working that all fall apart when you're making significant changes HMMMMM
Exanite
ExaniteOP3mo ago
Yeah, that's one approach I was considering:
On a side note, RenameAllAsync should be split into two methods: a gather step and a modify step, similar to how the mods are implemented. Currently RenameAllAsync does both, which means that it's hard to do group all gathers/modifies together and that it is hard to reuse information, such as the Locations found by RenameAllAsync (if the Locations don't become invalidated; I'm not sure how that works yet). This can help with implementing pointer dimension reduction in a general way, along with any other modifications that need to affect all references of a symbol.
The other is just falling back to strings. Eg: Find the symbols and use the fully qualified names instead.
Perksey
Perksey3mo ago
wait I'm still confused, what is leading to the symbol model being modified by the time we get to renaming? what's happening between the identification and the renameallasync
Exanite
ExaniteOP3mo ago
The document rename specifically breaks it
Perksey
Perksey3mo ago
can we just do that after we've done everything else?
Exanite
ExaniteOP3mo ago
I can try that Would you like me to experiment with a few other possible solutions (if that doesn't work)?
Perksey
Perksey3mo ago
yeah that'd be grand thanks
Exanite
ExaniteOP3mo ago
Awesome, will do Before I do that, any opinions on the falling back to strings approach? This wouldn't affect the renamer, but would affect the rest of the steps.
Perksey
Perksey3mo ago
hmmm, I'd imagine you'll need to do some of that anyway given that renaming invalidates the symbols. so it'd be 1. Get a compilation 2. Identify the handleable structs 3. Get the files that have declaration parts for the handleable structs (you can do this using the symbol model easily iirc, this is an optimisation for the rewriter later), and record the fully qualified metadata-style (i.e. taking account for generics) type name 4. Rename the structs using the renamer 5. Rewrite using the data gathered in step 3 6. Rename files (can probably do this at the same time as part 5) it's either that or we do things "properly" by walking the symbol model again and I just don't think we get anything from that do DocumentIds persist across modifications? if so use that for the output from step 3 (first half)
Exanite
ExaniteOP3mo ago
No clue. I'll see when I attempt to use it 😅 Glad to hear that you aren't opposed to using strings. I think using strings in this case simplifies things since we know the exact rules of how strings work. Using the Roslyn data types between modifications is dangerous since we don't know the exact rules and don't know if the rules are reliable (eg: they might change between Roslyn versions). Any future contributors would need to know the rules as well.
Perksey
Perksey3mo ago
yeah I'm not opposed to using strings, I just don't want to fall into the trap of doing things that Roslyn already does ourselves. the symbol model should be used wherever we want proper type and/or usage information considering the codebase as one unit, but most mods are just concerned with doing specific transformations to syntax which don't require extensive symbol knowledge we originally moved to using a Roslyn Compilation when Curin and I were going down a dark path of trying to do usage detection, expression evaluation, and pointer indirection changes ourselves - we had managed without it up until that point but it just made sense to call it in the end
Exanite
ExaniteOP3mo ago
That makes sense In this case, we're gathering the data using symbols, then falling back to strings for use as longer term data storage If we do that in a way we can get the symbols back, it's not too much additional work Basically serialization now that I think about it (not that we want to implement a symbol serializer though, that's excessive)
Perksey
Perksey3mo ago
well it's more just "doing the complex analysis ahead of time, and then mapping the results of that analysis back to the syntax using information we can trivially regain from the syntax itself" so in our case it's using all the knowledge that the compiler has (i.e. the symbol model) at the current point in time (which isn't the cheapest operation in the world), and the mapping data in our case is just a full type metadata name, which we can easily "guess" from the syntax. it's not completely bulletproof which is why I'm mincing words, I don't want us to imply we're overly reliant on that mapping data as something like "long-term storage" implies, and it could be possible that we find some edge case later that we need to take account of that would result in us changing the way we map that data
Exanite
ExaniteOP3mo ago
it's not completely bulletproof
I agree as well
as something like "long-term storage" implies
"longer term" specifically, but I was referring more towards storing data between project modifications. This would also be limited to being within one Mod as well, since each Mod is expected to gather their own information.
Perksey
Perksey3mo ago
yep sounds good certainly good to be having these types of discussions to try and demystify the art of authoring a mod
Exanite
ExaniteOP3mo ago
Yup, I agree. It's definitely something that I've been slowly figuring out and realizing as I work on the codebase. I think a short document should be enough to explain most things though. I'll also keep these conversations in mind when I switch to writing the contributor documentation (I also said I was interested on working on contributor documentation when I first started working on the Vulkan bindings). Alright, I think I have enough information to finish rewriting TransformHandles. There doesn't seem to be a good way to get a fully qualified metadata name from a symbol. I'm currently concating the namespace and metadata name together. I have an alternative approach that doesn't need to do this though. I'll get my current approach working first, then try my alternative approach.
Exanite
ExaniteOP3mo ago
Got everything working as expected 😄
No description
Exanite
ExaniteOP3mo ago
The new pointer dimension reduction code is basically a copy of RenameAllAsync now. Combining the two was my alternative approach and I have a good idea on how to combine them.
Exanite
ExaniteOP3mo ago
I know you're working on reviewing Curin's COM PR, so this is a lower priority I finished combining the pointer dimension reduction and renaming steps The code for it looks like this (you can find the code in the new SilkTouch/Mods/LocationTransformation folder):
await LocationTransformationUtils.ModifyAllReferencesAsync(ctx, logger, handleTypes, [
new IdentifierRenamingTransformer(handleTypes.Select(t => ((ISymbol)t, $"{t.Name}Handle"))),
new PointerDimensionReductionTransformer()
], ct);
await LocationTransformationUtils.ModifyAllReferencesAsync(ctx, logger, handleTypes, [
new IdentifierRenamingTransformer(handleTypes.Select(t => ((ISymbol)t, $"{t.Name}Handle"))),
new PointerDimensionReductionTransformer()
], ct);
This new function replaces the original renamer entirely, but it uses the original renamer's overall design. There's a few other things I need to verify, but it should have equivalent functionality. I tested it by replacing the old RenameAllAsync method with it (see screenshot). With this, this means that TransformHandles is working exactly as expected. I should be able to move on to other tasks related to the Vulkan bindings now. I'm thinking of working on the enums next. I want to look into it a bit on my own before asking too many questions though (mainly because I don't have much context to work off of yet).
No description
Perksey
Perksey3mo ago
seems fine and probably what I had in mind thanks
Exanite
ExaniteOP3mo ago
I'll probably be back working on this by the end of this week I've been stuck doing web dev stuff 😅
Perksey
Perksey3mo ago
haha no worries
Exanite
ExaniteOP2mo ago
Alright, I'm back to working on Silk now 🎉 One thing I noticed when I was last working on the bindings was that a bunch of extra using statements were being added (see screenshot). This is due to the change I made in response to this: https://discord.com/channels/521092042781229087/1376331581198827520/1381026216466845716
oh I know it's becuase the file that does global using Silk.NET.Core is nuked as part of the mod context creation LOL
This means that the global using files are kept. Apparently ModCSharpSyntaxRewriter interacts with this (during TransformFunctions, but probably happens any time a mod inheriting from ModCSharpSyntaxRewriter is used). From what I can tell, ModCSharpSyntaxRewriter keeps track of all using statements and adds the using statements to each file. Because the global using files are now kept, ModCSharpSyntaxRewriter also picks those using statements up. This doesn't break anything, but does seem unintended. A fix could be to have ModCSharpSyntaxRewriter ignore global using statements. Relevant code in ModCSharpSyntaxRewriter:
public override SyntaxNode? VisitCompilationUnit(CompilationUnitSyntax node)
{
var ret = base.VisitCompilationUnit(node);
if (ret is not CompilationUnitSyntax comp)
{
return ret;
}

foreach (var use in comp.Usings)
{
AddUsing(use);
}

ret = comp.WithUsings(GetUsings(UsingsToAdd, comp))
// ...
public override SyntaxNode? VisitCompilationUnit(CompilationUnitSyntax node)
{
var ret = base.VisitCompilationUnit(node);
if (ret is not CompilationUnitSyntax comp)
{
return ret;
}

foreach (var use in comp.Usings)
{
AddUsing(use);
}

ret = comp.WithUsings(GetUsings(UsingsToAdd, comp))
// ...
No description
Perksey
Perksey2mo ago
oh wow yeah ok tbh ModCSharpSyntaxRewriter is something of a hack, it’d be great to have a better solution but I haven’t thought of anything clever yet
Exanite
ExaniteOP2mo ago
What is the class meant to do? My guess is to just make sure all of the usings are available, but that's about it.
Perksey
Perksey2mo ago
yeah that’s pretty much it, it was really for generating new files and making sure all the code therein could resolve all their references (e.g. the interface files) it was a hacky way to accomplish it the class itself is meant to contain common utilities for rewrites within mods, but that was the only common utility at that time
Exanite
ExaniteOP2mo ago
Usually I like global:: qualifying all of the type names for generated code, but I'm not sure how feasible that is for Silk I can't immediately think of a better way to handle the usings though Should I look into making ModCSharpSyntaxRewriter ignore the global using statements?
Perksey
Perksey2mo ago
given that for the most part SilkTouch tries to be a dumb syntax based generator, it’s useful to not think too much about the types which is why I put the hack that is ModCSharpSyntaxRewriter in place a little hack to make sure all the usings were there so that the rest of the generator doesn’t need to worry
Exanite
ExaniteOP2mo ago
I think that's a fair thing to do It's imprecise, but keeps things simple
Perksey
Perksey2mo ago
removing the global usings in ModCSharpSyntaxRewriter could be a change that works but really the problem is we’re pulling in unrelated usings
Exanite
ExaniteOP2mo ago
That is true
Perksey
Perksey2mo ago
but yeah ignoring global using statements is a good idea given they’re implicit, and also making sure we’re only using ModCSharpSyntaxRewriter when we’re generating new files (or at least only adding the usings when that’s happening, as I realise we still need to collect the usings)
Exanite
ExaniteOP2mo ago
What constitutes as generating a new file? Are you referring to all files being output in the SourceProject folder or only files added by mods (excluding ClangSharp/ClangScraper)? I'm guessing we never want to modify the global usings files. If this is the case, it might be worth making ModCSharpSyntaxRewriter ignore all files in the project that are outside of the output folder (basically the opposite of what we did with MSBuildModContext.InitializeAsync). Admittedly hacky, but we're already depending on the same assumption to filter out the global usings files anyway; why not ignore those files completely? The other way to approach this is to just keep all of the files consistent (add using statements to all project files, excluding global using statements), which I like better since it's one less condition to keep track of. The extra using statements don't really hurt anyway (I know they can cause name conflicts). They just add noise to the output.
Perksey
Perksey2mo ago
basically just look for AddDocument yeah we don’t want to modify those files, so making it only add the usings where we’re doing AddDocument is probably sensible true. tbh, it’d be interesting to see what happened if you just disabled this logic and tried to regenerate the projects.
Exanite
ExaniteOP2mo ago
tbh, it’d be interesting to see what happened if you just disabled this logic and tried to regenerate the projects.
As in just don't add the using statements at all?
Perksey
Perksey2mo ago
yep
Exanite
ExaniteOP2mo ago
Hmm, I'll test that once the bindings compile Currently I have a lot of other type errors from other missing types (from Vulkan video and the other headers I haven't included yet)
Perksey
Perksey2mo ago
it feels like the sensible solution here is getting rid of this type as it’s clearly a bit of a footgun and/or a heavy-handed approach. makes sense
Exanite
ExaniteOP2mo ago
Yeah, I was thinking of just manually specifying the using statements to add in the config or something
Perksey
Perksey2mo ago
that is probably the opposite end of the spectrum, I feel like it shouldn’t be too difficult to just add the usings a bit more surgically but yeah we’ll see what breaks without this
Exanite
ExaniteOP2mo ago
Ah gotcha As an update, I'm currently investigating the generation of enums. I'm not super sure on the specifics yet. Some context: 1. The Vulkan headers defines 32 bit and 64 bit enums differently. - 32 bit enums are actually defined as enums (eg: typedef enum VkAccessFlagBits). - 64 bit enums are defined as a typedef to VkFlags64 (eg: typedef VkFlags64 VkAccessFlags2;) and a bunch of constants (eg: static const VkAccessFlagBits2 VK_ACCESS_2_NONE = 0ULL;). 2. In the generated bindings, 32 bit enums are fine; however, 64 bit enums are empty with the enum values being defined as constants in Vulkan.gen.cs. What I know so far: 1. This functionality seems to be related primarily to the ClangScraper, MixKhronosData and ExtractNestedTyping mods. 2. ClangScraper handles the 32 bit enums just fine. 3. MixKhronosData somehow discovers the 64 bit enums and creates empty enums for them. - I haven't looked into this much yet, but I know where the code that does this is located. 4. It looks like ExtractNestedTyping is supposed to discover enum constants and move them from Vulkan.gen.cs to their corresponding enums. - This currently doesn't work because, for example, VkAccessFlags2's trimming name is not a prefix of VK_ACCESS_2_NONE's trimming name (we need to remove the Flags suffix from the name). - Also, putting in a hacky fix for this (eg: enumTrimmingName.Replace("_Flags", "_");) reveals more problems. This seems to be because ExtractNestedTyping.Walker._numericTypeNames doesn't contain the type information for the relevant enums. Some enums are missing and the value for some of the key value pairs are null. I haven't looked further than this when it comes to ExtractNestedTyping, but ExtractNestedTyping seems to be where I should focus my efforts. For reference, this is the declaration of ExtractNestedTyping.Walker._numericTypeNames:
private readonly Dictionary<
string,
(
SyntaxKind Type,
HashSet<string> ReferencingFileDirs,
HashSet<string> ReferencingNamespaces
)?
> _numericTypeNames = new();
private readonly Dictionary<
string,
(
SyntaxKind Type,
HashSet<string> ReferencingFileDirs,
HashSet<string> ReferencingNamespaces
)?
> _numericTypeNames = new();
If the information/direction above is correct, I think I should be able to figure the remaining information out and hack together a proof of concept.
Perksey
Perksey2mo ago
i thought we had discussed this already, but yeah basically this will need something similar to what we already do for OpenGL: pull those constants out into enums based on the XML ExtractNestedTyping is supposed to handle this pattern to some extent yeah, but it doesn't match every pattern. I do want to make it handle different prefixes at some point, and perhaps there can be a cross-mod interface so that MixKhronosData can tell ExtractNestedTyping what prefixes it knows about. a null value is meant to indicate "we've spotted a discrepancy and therefore we're not generating this enum"
Exanite
ExaniteOP2mo ago
Yeah, the context part was for completeness since it's been over a month since we last talked about it. I wanted to make sure we were on the same page. So far, it sounds like I'm on the right track so I'll keep investigating. Alright, so looks like ExtractNestedTyping is pretty much irrelevant when it comes to Vulkan enum extraction (it's related conceptually, but none of the code is used) That's something I got wrong in my initial investigation One problem I identified was that MixKhronosData.Rewriter.VisitFieldDeclaration was failing to identify the enum constants in Vulkan.gen.cs. This meant that the enum constants were not being moved to their corresponding enum types. This was because Vulkan enum constants have a [NativeTypeName] value in the form const Type, which is different from OpenGL's #define NAME VALUE. I added a case to handle this and it looks like the code now works as expected! 🎉 There still seems to be some empty enums (ExportMetalObjectTypeFlagsEXT, ImageConstraintsInfoFlagsFuchsia, SwapchainImageUsageFlagsAndroid, maybe more; note that these are C# type names). I'm guessing that these are because I haven't added all of the Vulkan headers yet, so probably not a big issue. Here's the commit I made: https://github.com/dotnet/Silk.NET/pull/2457/commits/78bbb4d6a384f732fcc5348bd823f37fcbb8273b It's been a while since I've looked at the bindings at a whole, but I think that's all the tasks that we've discussed so far. The bindings don't compile yet, so that's my next goal. From Github Actions, we have the following types of errors: https://github.com/dotnet/Silk.NET/actions/runs/16159168351/job/45607394446?pr=2457 1. /Vulkan/VideoDecodeH264ProfileInfoKHR.gen.cs(35,12): error CS0246: The type or namespace name 'StdVideoH264ProfileIdc' could not be found (are you missing a using directive or an assembly reference?) 2. /Vulkan/StructureType.gen.cs(8001,5): error CS0102: The type 'StructureType' already contains a definition for 'SurfaceCapabilities2EXT' 3. /Vulkan/StructureType.gen.cs(8001,31): error CS0229: Ambiguity between 'StructureType.SurfaceCapabilities2EXT' and 'StructureType.SurfaceCapabilities2EXT'
SurfaceCapabilities2EXT = SurfaceCapabilities2EXT,
SurfaceCapabilities2EXT = SurfaceCapabilities2EXT,
// VK_STRUCTURE_TYPE_SURFACE_CAPABILITIES2_EXT is a deprecated alias
VK_STRUCTURE_TYPE_SURFACE_CAPABILITIES2_EXT = VK_STRUCTURE_TYPE_SURFACE_CAPABILITIES_2_EXT,
// VK_STRUCTURE_TYPE_SURFACE_CAPABILITIES2_EXT is a deprecated alias
VK_STRUCTURE_TYPE_SURFACE_CAPABILITIES2_EXT = VK_STRUCTURE_TYPE_SURFACE_CAPABILITIES_2_EXT,
4. /Vulkan/Vulkan.gen.cs(26222,22): error CS0102: The type 'Vulkan' already contains a definition for 'KhrMaintenance1SpecVersion'
[NativeTypeName("#define VK_KHR_MAINTENANCE_1_SPEC_VERSION 2")]
public const int KhrMaintenance1SpecVersion = 2;

[NativeTypeName("#define VK_KHR_MAINTENANCE1_SPEC_VERSION VK_KHR_MAINTENANCE_1_SPEC_VERSION")]
public const int KhrMaintenance1SpecVersion = 2;
[NativeTypeName("#define VK_KHR_MAINTENANCE_1_SPEC_VERSION 2")]
public const int KhrMaintenance1SpecVersion = 2;

[NativeTypeName("#define VK_KHR_MAINTENANCE1_SPEC_VERSION VK_KHR_MAINTENANCE_1_SPEC_VERSION")]
public const int KhrMaintenance1SpecVersion = 2;
5. /Vulkan/Vulkan.gen.cs(25934,10): error CS0019: Operator '<<' cannot be applied to operands of type 'uint' and 'uint'
public const uint ApiVersion1X0 = (
(((uint)(0)) << 29U) | (((uint)(1)) << 22U) | (((uint)(0)) << 12U) | ((uint)(0))
);
public const uint ApiVersion1X0 = (
(((uint)(0)) << 29U) | (((uint)(1)) << 22U) | (((uint)(0)) << 12U) | ((uint)(0))
);
Here are my current thoughts on the errors: 1 - Related to the headers I haven't included, but bring up the question of how we want to handle them. I haven't looked into whether simply including the headers will work or if there are going to be additional dependencies that need to be pulled in. Also, what happens if those dependencies are platform specific? 2,3,4 - We should ignore VK_STRUCTURE_TYPE_SURFACE_CAPABILITIES2_EXT, VK_KHR_MAINTENANCE1_SPEC_VERSION, etc. They are deprecated aliases. Looks like the XML tells us which are deprecated. 5 - Not sure how to best handle this one. It looks like ClangSharp directly outputs this. Is this an issue we should fix here in Silk or in ClangSharp? (I'll update this message as I investigate further) For 5, it looks like TerraFX has the correct expression: https://raw.githubusercontent.com/terrafx/terrafx.interop.vulkan/refs/heads/main/sources/Interop/Vulkan/Vulkan/vulkan/vulkan_core/Vulkan.cs
[NativeTypeName("#define VK_API_VERSION_1_0 VK_MAKE_API_VERSION(0, 1, 0, 0)")]
public const uint VK_API_VERSION_1_0 = ((((uint)(0)) << 29) | (((uint)(1)) << 22) | (((uint)(0)) << 12) | ((uint)(0)));
[NativeTypeName("#define VK_API_VERSION_1_0 VK_MAKE_API_VERSION(0, 1, 0, 0)")]
public const uint VK_API_VERSION_1_0 = ((((uint)(0)) << 29) | (((uint)(1)) << 22) | (((uint)(0)) << 12) | ((uint)(0)));
Not sure how this was done.
Perksey
Perksey2mo ago
it is highly likely that it was done through manual edits, but you can prove that by running ClangSharp in that repo
Exanite
ExaniteOP2mo ago
Gotcha, I wasn't sure if there was a flag somewhere or if they edited the generated code. I'll take a look
Exanite
ExaniteOP2mo ago
That took a silly amount of time to figure out, but yeah, I'm also getting the unsigned integers when running ClangSharp in the TerraFx repo:
No description
Exanite
ExaniteOP2mo ago
Is this something we should handle here in Silk or in ClangSharp?
Perksey
Perksey2mo ago
so yeah this is a ClangSharp bug that I guess Tanner is manually working around for now we’ve tried to fix things upstream where possible and Tanner does tend to be good about releasing updates when we’ve sent PRs their way
Exanite
ExaniteOP2mo ago
I can take a look at contributing to ClangSharp and seeing if I can fix that. Alright, opened an issue: https://github.com/dotnet/ClangSharp/issues/611
Perksey
Perksey2mo ago
awesome, thanks. cc @tannergooding
tannergooding
tannergooding2mo ago
Contributions are welcome 😄 This is just one of those annoying edge cases with how shifts work in C# 😄
Exanite
ExaniteOP2mo ago
I submitted a PR with the fix: https://github.com/dotnet/ClangSharp/pull/612 In the meantime, I'll work on the other tasks related to the Vulkan bindings
Exanite
ExaniteOP2mo ago
@Perksey I'm looking into Vulkan Video right now since it's the last thing I need to figure out to get the bindings compiling. Note that I'm completely unfamiliar with Vulkan Video. Technically it already compiles if I disable PrettifyNames. With PrettifyNames, it crashes on STD_VIDEO_AV1_CHROMA_SAMPLE_POSITION_RESERVED since it thinks RESERVED is a vendor suffix. With STD_VIDEO_AV1_CHROMA_SAMPLE_POSITION being the enum name and RESERVED being treated as the vendor suffix, this means Prettify is passed a single underscore (_) as the string to prettify, which it stack overflows on. Should Vulkan Video be part of the Vulkan bindings or should it be separate? It looks like Vulkan Video in Silk 2 is part of the same project, but generated separately. How should I handle this in Silk 3?
No description
Exanite
ExaniteOP2mo ago
From what I can tell, Vulkan Video doesn't depend on anything so I'm going to see if I can get Vulkan Video to compile on its own first, then make Vulkan depend on Vulkan Video. It looks like Vulkan Video also has its own XML spec, which Silk 2 apparently doesn't use.
Perksey
Perksey2mo ago
keeping it in one generation should be possible, it was only separate because of the way BuildTools worked (ConvertConstruct vs Clang) RESERVED being a vendor suffix is interesting the stack overflow even more so never observed these behaviours. Ideally the vendor name trimming is just a “preferred” thing and that we can roll back to a less preferred (but still trimmed) name. I know there is a notion of primary vs secondary in there but not sure how smart it is (or if trimming generates multiple results) but the stack overflow is bonkers
Exanite
ExaniteOP2mo ago
It stack overflows on the stackalloc, which I assume means it's trying to allocate a crazy amount of memory
No description
Exanite
ExaniteOP2mo ago
If I remember, Length is 0 here
Perksey
Perksey2mo ago
hrmf, yeah that makes sense need to add a sanity check in there really it’s probably a stackalloc of 0 tbh if it is a single underscore but C# arithmetic operations are checked by default and id be really surprised if it’s got a massive parameter
Exanite
ExaniteOP2mo ago
Issue is that I don't think Prettify can return a meaningful value even if we add a sanity check. We gave it bad input so we get bad output.
Exanite
ExaniteOP2mo ago
It's coming from here:
.. xml.Element("registry")
?.Element("extensions")
?.Elements("extension")
.Attributes("name")
.Select(x => x.Value.Split('_')[1].ToUpper()) ?? Enumerable.Empty<string>()
.. xml.Element("registry")
?.Element("extensions")
?.Elements("extension")
.Attributes("name")
.Select(x => x.Value.Split('_')[1].ToUpper()) ?? Enumerable.Empty<string>()
No description
Exanite
ExaniteOP2mo ago
VK_RESERVED_do_not_use_94 and VK_RESERVED_do_not_use_146 specifically
Perksey
Perksey2mo ago
sigh In the case of vulkan we can probably get away by using the tags at the top of the file yeah I’d do that, if registry contains a tags element, use the name attribute of each tag element within tags, otherwise do things the current way this should hopefully stop OpenGL from breaking while catering for the silly RESERVED cases
Exanite
ExaniteOP2mo ago
Adding the extensions element only adds two additional "vendors". COREAVI seems to be a real vendor, but their extensions are disabled:
<extension name="VK_COREAVI_extension_442" number="442" author="COREAVI" contact="Aidan Fabius @afabius" supported="disabled">
<extension name="VK_COREAVI_extension_442" number="442" author="COREAVI" contact="Aidan Fabius @afabius" supported="disabled">
No description
Exanite
ExaniteOP2mo ago
They also don't show up much in the headers:
No description
Perksey
Perksey2mo ago
tbh another solution would be filtering on supported
Exanite
ExaniteOP2mo ago
Hmm That seems viable
Perksey
Perksey2mo ago
We know that vulkan and vulkansc are the only declared profiles already so unless an extension contains at least one of those in its supported value, it’s not valid
Exanite
ExaniteOP2mo ago
Do we have that information in MixKhronosData?
Perksey
Perksey2mo ago
should do its already being pulled out for the supported API attributes iirc
Exanite
ExaniteOP2mo ago
I think only AddApiProfiles knows about it, but I haven't fully verified
No description
Perksey
Perksey2mo ago
MixKhronosData gives the data to AddApiProfiles it’s an IApiMetadataProvider<IEnumerable<SupportedApiAttribute>> from memory
Exanite
ExaniteOP2mo ago
Gotcha Looks like you can pull out vulkan and vulkansc by doing this
No description
Exanite
ExaniteOP2mo ago
Not sure if this information is readily available elsewhere though. Still looking Do you know of any documentation for the XML spec format?
Perksey
Perksey2mo ago
there is a RelaxNG spec, but I make no comments on how valid it is: https://github.com/KhronosGroup/Vulkan-Docs/blob/main/xml/registry.rnc I usually just open the XML file and go from there
Exanite
ExaniteOP2mo ago
Yeah, that's what I've been doing, but I want to make sure I'm not making any incorrect assumptions
Perksey
Perksey2mo ago
there are a lot of different XML formats as well that are subtly different for which there is a lot of code in MixKhronosData to take account of
Exanite
ExaniteOP2mo ago
Yeah, that's the scary thing as well I don't know if doing this will work for the other APIs Good thing is that we have a few possible solutions so we can switch between them if needed
Perksey
Perksey2mo ago
yeah exactly the tags solution doesn't work for gl.xml for instance as they don't have that but it'll work for OpenXR and Vulkan FWIW we have unit tests that iirc use verify so if you broke something you'd know
Exanite
ExaniteOP2mo ago
As in Verify snapshot testing?
Perksey
Perksey2mo ago
yep
No description
Exanite
ExaniteOP2mo ago
Ah that's nice
Perksey
Perksey2mo ago
i do need to get better at testing in general though generally if there's tests it's because i'm not confident, but being confident isn't a good enough reason not to have test coverage
Perksey
Perksey2mo ago
e.g. this shit I was not confident about in the slightest
No description
Exanite
ExaniteOP2mo ago
The bindings compile now 🎉
No description
Perksey
Perksey2mo ago
🎉🎉🎉🎉🎉
Exanite
ExaniteOP2mo ago
I can finally do the thing you asked me to do at the very start 🤣, which was to work on the vtable
Perksey
Perksey2mo ago
oh yeahhhh haha
Exanite
ExaniteOP2mo ago
It's been a journey
Perksey
Perksey2mo ago
hopefully it hasn’t felt too terrible 😂 it’s certainly been good to get an outsiders perspective, but I’m afraid I think you’re an insider now
Exanite
ExaniteOP2mo ago
Yeah, not too terrible, especially due to your help You've answered a lot of my questions that would have otherwise taken me hours to figure out
Perksey
Perksey2mo ago
basically a lot of things I probably should’ve written docs for :/ but I’m glad to help 🙂
Exanite
ExaniteOP2mo ago
I attempted to update a copy of my Vulkan code to Silk 3 and found a bunch of other generation issues Fortunately, none of them prevent me from using the bindings, but definitely nice to get fixed. I made a list of these issues and other todos at the bottom of my PR: https://github.com/dotnet/Silk.NET/pull/2457 I marked the ones that require a discussion/decision with "(Discuss)". A simple reason/yes/no should be enough for most of the "Discuss" items though. The rest I can investigate on my own or aren't relevant yet. I plan to address the easier/high impact ones first, then use dfkeenan's Vulkan tutorial code to get Silk 3 running (my Vulkan code has dependencies on VMA and SPIRV Reflect which makes it hard to test Vulkan independently). At that point, I'll switch to working on the vtables. I looked into some of the issues and also fixed some of them. I have also reorganized the todos list at the bottom of my PR.
Exanite
ExaniteOP2mo ago
Well, this is annoying This happens in other cases as well (that I don't remember) This inconsistency by itself shouldn't break my code though. I need to check if MixKhronosData.ReadGroups skips over empty enum/groups or not
No description
No description
Exanite
ExaniteOP2mo ago
For context, I'm currently working on these issues
No description
Exanite
ExaniteOP2mo ago
And I actually got it mostly working
No description
Exanite
ExaniteOP2mo ago
The issue is that it does this transformation too eagerly and does it for types that don't actually exist (eg: VkBuildAccelerationStructureFlagsNV). I should be able to fix this though. I have two plans, one of which should be guaranteed to work (which is to just track all enums in the project). Decided to do the guaranteed one since it was guaranteed and easier. It works 🎉 The core issue with my previous approach was that not all of the Vulkan enums were in the JobData.Groups collection though. For example, VkMemoryUnmapFlagBits is not included (which I'm guessing is because the enum in the XML is empty; see above screenshots). I'm not sure if that is intended or not. I added a new TransformFlags mod that adds a None = 0 member to [Flags] enums if they do not have an equivalent already (member of same name or of value 0 when checked using semanticModel.GetConstantValue()). This was because Silk 2 seems to do the same (I only checked the generated bindings, not the Silk 2 BuildTools code; I probably should take a quick look now that I'm thinking of it). Btw, I know we didn't discuss these changes beforehand so I'm open to any changes. My next goal is to fix the trimming issues that I have listed in my PR:
Exanite
ExaniteOP2mo ago
No description
Exanite
ExaniteOP2mo ago
How does enum member name trimming work / where can I find the code for it? Some enums are not generating as intended:
public enum ComponentSwizzle : uint
{
ComponentSwizzleIdentity = 0,
ComponentSwizzleZero = 1,
ComponentSwizzleOne = 2,
ComponentSwizzleR = 3,
ComponentSwizzleG = 4,
ComponentSwizzleB = 5,
ComponentSwizzleA = 6,
}

public enum IndirectStateFlagsNV : uint
{
None = 0, // Note: None is a member I added to match Silk 2 behavior. I add the None member at the very end of the generation pipeline. In other words, trimming happens first.
FlagFrontfaceBitNV = 0x00000001,
}

public enum SurfaceCounterFlagsEXT : uint
{
None = 0,
SurfaceCounterVblankBitEXT = 0x00000001,
SurfaceCounterVblankEXT = SurfaceCounterVblankBitEXT,
}
public enum ComponentSwizzle : uint
{
ComponentSwizzleIdentity = 0,
ComponentSwizzleZero = 1,
ComponentSwizzleOne = 2,
ComponentSwizzleR = 3,
ComponentSwizzleG = 4,
ComponentSwizzleB = 5,
ComponentSwizzleA = 6,
}

public enum IndirectStateFlagsNV : uint
{
None = 0, // Note: None is a member I added to match Silk 2 behavior. I add the None member at the very end of the generation pipeline. In other words, trimming happens first.
FlagFrontfaceBitNV = 0x00000001,
}

public enum SurfaceCounterFlagsEXT : uint
{
None = 0,
SurfaceCounterVblankBitEXT = 0x00000001,
SurfaceCounterVblankEXT = SurfaceCounterVblankBitEXT,
}
Corresponding Vulkan header code for reference:
typedef enum VkComponentSwizzle {
VK_COMPONENT_SWIZZLE_IDENTITY = 0,
VK_COMPONENT_SWIZZLE_ZERO = 1,
VK_COMPONENT_SWIZZLE_ONE = 2,
VK_COMPONENT_SWIZZLE_R = 3,
VK_COMPONENT_SWIZZLE_G = 4,
VK_COMPONENT_SWIZZLE_B = 5,
VK_COMPONENT_SWIZZLE_A = 6,
VK_COMPONENT_SWIZZLE_MAX_ENUM = 0x7FFFFFFF
} VkComponentSwizzle;

typedef enum VkIndirectStateFlagBitsNV {
VK_INDIRECT_STATE_FLAG_FRONTFACE_BIT_NV = 0x00000001,
VK_INDIRECT_STATE_FLAG_BITS_MAX_ENUM_NV = 0x7FFFFFFF // Note: I remove MAX_ENUM members to match Silk 2 behavior.
} VkIndirectStateFlagBitsNV;
typedef VkFlags VkIndirectStateFlagsNV;

typedef enum VkSurfaceCounterFlagBitsEXT {
VK_SURFACE_COUNTER_VBLANK_BIT_EXT = 0x00000001,
// VK_SURFACE_COUNTER_VBLANK_EXT is a deprecated alias
VK_SURFACE_COUNTER_VBLANK_EXT = VK_SURFACE_COUNTER_VBLANK_BIT_EXT,
VK_SURFACE_COUNTER_FLAG_BITS_MAX_ENUM_EXT = 0x7FFFFFFF
} VkSurfaceCounterFlagBitsEXT;
typedef enum VkComponentSwizzle {
VK_COMPONENT_SWIZZLE_IDENTITY = 0,
VK_COMPONENT_SWIZZLE_ZERO = 1,
VK_COMPONENT_SWIZZLE_ONE = 2,
VK_COMPONENT_SWIZZLE_R = 3,
VK_COMPONENT_SWIZZLE_G = 4,
VK_COMPONENT_SWIZZLE_B = 5,
VK_COMPONENT_SWIZZLE_A = 6,
VK_COMPONENT_SWIZZLE_MAX_ENUM = 0x7FFFFFFF
} VkComponentSwizzle;

typedef enum VkIndirectStateFlagBitsNV {
VK_INDIRECT_STATE_FLAG_FRONTFACE_BIT_NV = 0x00000001,
VK_INDIRECT_STATE_FLAG_BITS_MAX_ENUM_NV = 0x7FFFFFFF // Note: I remove MAX_ENUM members to match Silk 2 behavior.
} VkIndirectStateFlagBitsNV;
typedef VkFlags VkIndirectStateFlagsNV;

typedef enum VkSurfaceCounterFlagBitsEXT {
VK_SURFACE_COUNTER_VBLANK_BIT_EXT = 0x00000001,
// VK_SURFACE_COUNTER_VBLANK_EXT is a deprecated alias
VK_SURFACE_COUNTER_VBLANK_EXT = VK_SURFACE_COUNTER_VBLANK_BIT_EXT,
VK_SURFACE_COUNTER_FLAG_BITS_MAX_ENUM_EXT = 0x7FFFFFFF
} VkSurfaceCounterFlagBitsEXT;
I'm guessing NameTrimmer.Trim and NameTrimmer.GetPrefix has the relevant code.
Perksey
Perksey2mo ago
yeah so it's likely NameTrimmer and to a lesser extent PrettifyNames. It's likely thinking "oh there is no common prefix between None and all the other members" and therefore doesn't trim a way around this could be to put [Transformed] on None (this is a magic attribute that is interpreted to mean "this is generated and shouldn't influence any code that wants to get facts about the native APIs") alternatively you can move TransformFlags to after PrettifyNames
Exanite
ExaniteOP2mo ago
None gets added at the very end, after trimming is done, so it shouldn't affect anything
Perksey
Perksey2mo ago
oh weird I'm really not sure why it's falling over here then
Exanite
ExaniteOP2mo ago
[Transformed] is good to know about though I looked around briefly at that earlier
Perksey
Perksey2mo ago
but yeah naming things is one of the hardest thing with silktouch, whenever I change something there I try to ensure we have unit tests to reflect the changes (and what we're expecting elsewhere)
Exanite
ExaniteOP2mo ago
Gotcha, I'll keep investigating then NameTests is the where I would add new tests right?
Perksey
Perksey2mo ago
yep, you can see I've added some ones previously prefixed with Regression where I encountered bugs with the naming logic probably will make debugging the problem easier as well
Exanite
ExaniteOP2mo ago
Also, any idea why I can't run the tests locally? Rider shows Platform 'Arm64' is not supported on architecture 'X64' on the right.
No description
Perksey
Perksey2mo ago
uhhh what
Exanite
ExaniteOP2mo ago
I'm on Linux x64 so I'm not sure where Arm64 is coming from
Perksey
Perksey2mo ago
I mean I have been devving and running them on my arm64 mac does dotnet test work?
Exanite
ExaniteOP2mo ago
No description
Perksey
Perksey2mo ago
ah ok different issue
Exanite
ExaniteOP2mo ago
I'm not familiar with this error either
Perksey
Perksey2mo ago
this one I believe is because you don't have the android workload installed you can use nuke disable-platforms --platforms android if you want to get rid of that without installing the workload
Exanite
ExaniteOP2mo ago
Looks like dotnet test works Failed tests are related to the Maths/Maths folder, which I'm assuming is unrelated to anything I'm doing
No description
Perksey
Perksey2mo ago
yeah there's a bunch of flaky tests in there that seem to fail on certain platforms/.NET versions not worried about those, that codebase will be replaced eventually
Exanite
ExaniteOP2mo ago
Alright, I'll keep investigating the trimming issues and see what I can do I should be good for now
Exanite
ExaniteOP2mo ago
So, turns out that only the XUnit tests work. NUnit just refuses to work for some reason. Things I've tested: - Through the CLI vs through Rider - On Windows vs Linux - On my branch vs develop/3.0 Also tried updating packages just in case that was the issue, but same result The weirder thing is that I use NUnit and XUnit in my own repos and they work fine Screenshots show the errors from CLI and Rider
No description
No description
Exanite
ExaniteOP2mo ago
Interesting, CI also shows the error
No description
Exanite
ExaniteOP2mo ago
Text file contains snippets from the logs, but it seems to only work on Mac. Windows and Linux are broken.
Exanite
ExaniteOP2mo ago
The logs above are from this Actions run: https://github.com/dotnet/Silk.NET/actions/runs/16252017737/job/45882986503?pr=2457 I'm seeing similar results on Curin's branch: https://github.com/dotnet/Silk.NET/actions/runs/16251734862/job/45882329908 Well, it's definitely something to do with BuildTools I commented out the BuildTools code (edit: mainly referring to the PackageReference) and the unit tests now work
Perksey
Perksey2mo ago
wtf
Exanite
ExaniteOP2mo ago
I'm guessing a .NET DLL or shared library isn't getting copied to the right place or something The good thing is I have a workaround now, so I think I'll take a break from debugging why the unit tests don't work and look at the trimming code instead The Arm64 issue also disappears if I comment out the the Silk.NET.BuildTools package reference
Exanite
ExaniteOP2mo ago
Hmm, definitely -1 here
No description
Exanite
ExaniteOP2mo ago
ComponentSwizzle is getting rewinded here (eg: "ComponentSwizzleOne"):
No description
Exanite
ExaniteOP2mo ago
current.Length <= 4 is true so the issue is that the trimmed name is considered too short I need to figure out why this condition was added and how to override/change it to work better Naively disabling rewinding altogether leads to the error here
Perksey
Perksey2mo ago
interesting. the trimming code has an extremely long history that is mostly rooted in various iterations of BuildTools. I think the smarter trimming stuff started coming in as of 2.16. this rewinding stuff feels recent though the comment has my mannerisms but isn’t particularly helpful nonetheless you can probably trace it back to a commit of mine.
Exanite
ExaniteOP2mo ago
Yeah, it's from a commit of yours
No description
Perksey
Perksey2mo ago
I don’t remember why the length check was introduced hopefully there’s a commit in that PR that incrementally adds this so we can see the before and after
Exanite
ExaniteOP2mo ago
That would be in Silk 2 right?
Perksey
Perksey2mo ago
nah that’s silk 3 #2020 that is
Exanite
ExaniteOP2mo ago
Oh I see, I'm guessing that's a squashed commit
Perksey
Perksey2mo ago
we like to preserve history but we squash PRs upon merge with the logic being that the PR still has the history of you want to dig deeper
Exanite
ExaniteOP2mo ago
I'll take a look there and see if I can find a reason If not, I can always just run the OpenGL bindings generation and see what breaks
Perksey
Perksey2mo ago
yeah that’s probably a good shout and the stack overflow thing is just Prettify never being tested against single character names it’s an edge case that really you’d hope I’d have thought about using something dangerous like stackalloc
Exanite
ExaniteOP2mo ago
I fixed the issue, but it also affects the OpenGL bindings: https://github.com/Exanite/Silk.NET/commit/2df6cef33a790116c9f4b8cee3f0672ae68565be I think you should take a look and make sure that everything looks right. I'm not sure why the casing for RGBA32F and similar members changed to Rgba32F (I could take a look, but I'm tired 😅). The rest seem acceptable or improved. Also, I figured out what the rewind was actually meant to do:
No description
Exanite
ExaniteOP2mo ago
One thing to note is that the rewind doesn't actually remove the prefix, it just removes whatever comes before the first underscore. Considering each of the members are prefixed in each of the APIs, I think this is fine. I only took a glance though The screenshot shows which API XMLs I checked.
No description
Exanite
ExaniteOP2mo ago
I guess I don't explain why only leaving the vendor name is bad in the comment. It's because the next block of code removes the vendor name, leaving only an empty string or underscore.
Perksey
Perksey2mo ago
looks good, although we need to decide what to do about SubmitFlags SubmitProtectedBit turning into just Bit is a bit naff lmao but yeah the acronyms turning lowercase is weird, we’re supposed to have a LongAcronymThreshold for those I mean I can live with it, it doesn’t look hideous, it’s just interesting that this change has had this effect
Exanite
ExaniteOP2mo ago
I was concerned about it until I saw that Silk 2 had the same issue Is this something we want to fix for Silk 3? If yes, I'm not sure how to best implement the fix. From what I can tell, the trimmer finds and removes a common prefix from the enum member names. The fix would either have to be a name override... huh, there's a prefix override option in the PrettifyNames config; I wonder if that's what I need ...or a way for mods to specify the prefix that should be used.
No description
Exanite
ExaniteOP2mo ago
If you don't know the reason, then I'll look into it. I don't like leaving weird behavior unexplained 😂 Tbh, I prefer strict PascalCasing (Rgba, Ui, etc) since the names are so much easier to work with when using automated tooling. Just split by capitals and that's most of the cases handled.
Perksey
Perksey2mo ago
WAIT REALLY LMAO for the avoidance of doubt this is undesirable haha but I understand how the code got there it is doing the right things
Exanite
ExaniteOP2mo ago
The core difference is this. I'm looking into why the underscore matters.
No description
Perksey
Perksey2mo ago
oh right it’ll be the allowAllCaps logic maybe idk
Exanite
ExaniteOP2mo ago
Both should give the capitalized result right? I'm adding test cases for these
Exanite
ExaniteOP2mo ago
Are there any names that should also be changed?
No description
Perksey
Perksey2mo ago
so RG32EXT should have a trimming name of RG_32_EXT (or something similar, maybe RG_32EXT) which is then prettified into RG32EXT because RG and EXT are acronyms lower than the long acronym threshold I’m not actually sure off the top of my head how this works from a trimming vs prettifying perspective as thankfully it’s been a while since I’ve been in that code (whereas it felt like every other day I was making micro adjustments to it before, and it shows)
Exanite
ExaniteOP2mo ago
Here's the code for Prettify for reference:
No description
Exanite
ExaniteOP2mo ago
It seems to mostly go into Humanizer code
Perksey
Perksey2mo ago
what’s the type of transformer? the concrete type, I mean I seem to remember implementing that interface ourselves
Exanite
ExaniteOP2mo ago
NameTransformer with longAcronymThreshold of 4
No description
Perksey
Perksey2mo ago
ah cool, that’s where that comes in so I would expect it to be RG32FEXT. There is a question on whether the FEXT is desirable or if we want FExt. I don’t remember the logic for adding allowAllCaps, I think it might’ve been an issue where GL_LESS was becoming LESS instead of Less to be clear I don’t think that’s what’s kicking in here I’d expect Rg32fext if that were the case
Exanite
ExaniteOP2mo ago
I have a bit more information on my side since I have the debugger open. I should be able to figure out why it's behaving as it is I'm more concerned about what we want the results to be
Perksey
Perksey2mo ago
yeah makes sense I think RG32FEXT for now
Exanite
ExaniteOP2mo ago
How about the enum members that weren't changed by my commit?
Exanite
ExaniteOP2mo ago
Such as these:
No description
Perksey
Perksey2mo ago
hmmmmm, the Ext vs EXT inconsistency is driving me a bit nuts I’m happy with these though errr, correction I am not happy with these not sure what is happening with the Rg/Rgb, feels like it should be caps
Exanite
ExaniteOP2mo ago
Yeah, that's what I'm thinking as well Feels inconsistent to have both Rgb and RGB in the same file
Perksey
Perksey2mo ago
yeah that’s so strange
Exanite
ExaniteOP2mo ago
How about Ui vs UI for the 2nd to last enum I'm guessing we are fine with Snorm and Unorm judging based off this issue: https://github.com/dotnet/Silk.NET/issues/427
Perksey
Perksey2mo ago
it probably makes sense logically to be UI yeah as long as it’s consistent I’m not bothered it certainly looks like the behaviour, whether intentional or not, is that names containing multiple acronyms or words do not have capitalised acronyms
Exanite
ExaniteOP2mo ago
I'm guessing that's to handle cases where multiple adjacent acronyms exist
No description
Exanite
ExaniteOP2mo ago
I believe EXT (vendor names specifically) is a special case in MixKhronosData so it doesn't follow the rules
Perksey
Perksey2mo ago
ah ok cool, that makes sense yeah so in that case I’d probably be all for this change i.e. SRGBAlpha looks a bit hard to read heh, maybe acronyms should just be lowercase
Exanite
ExaniteOP2mo ago
I'm still wondering why we want to bother keeping acronyms capitalized in the first case Oh lol you beat me to it
Perksey
Perksey2mo ago
BuildTools wasn’t this smart around acronyms and clearly being too smart is causing us problems
Exanite
ExaniteOP2mo ago
It's not even a BuildTools issue at the core really Like how do you represent consecutive acronyms properly in PascalCase? In screaming snake case, you get ABC_DEF, but that's not possible in PascalCase so we have to work around it somehow.
Perksey
Perksey2mo ago
yeah, it’s just impossible really I still think it’s sensible to have acronyms capitalised if the entire name is the acronym i.e. URL instead of Url
Exanite
ExaniteOP2mo ago
I don't see much benefit to that though And it complicates the logic slightly
Perksey
Perksey2mo ago
hmmmm true do we know what the framework design guidelines are around here
Exanite
ExaniteOP2mo ago
Like Microsoft's C# naming convention guidelines?
Perksey
Perksey2mo ago
yeah looks like their guidelines is that if it’s 2 letters it’s capitalised e.g. ID but yeah it’s probably not even worth it that’s such a fringe case I’m content with getting rid of the capitalisation Rg32FEXT (assuming that EXT is an outlier as per MixKhronosData as you said)
Exanite
ExaniteOP2mo ago
I think FExt is more readable, so even that can be changed Hard to immediately tell what the F means Eg: Does it mean "float" or is there a vendor name called "FEXT"?
Perksey
Perksey2mo ago
means float in this case it is probably more readable, but the capitalisation does help illustrate that this is a vendor name in the majority case where the last letter is not capitalised, that is
Exanite
ExaniteOP2mo ago
Hmm, perhaps It's usually more readable in other cases I wonder about how useful the capitalization is though Usually it's pretty obvious when something is from an extension or vendor in OpenGL or Vulkan Since it's already a special case in the code, it doesn't really hurt to leave it as is, so I'm fine with doing it either way
Perksey
Perksey2mo ago
yeah I think I do prefer the capitalisation here, this is the official convention with function names for instance
Exanite
ExaniteOP2mo ago
As for changing the naming convention, I believe I should do that in a separate PR
Perksey
Perksey2mo ago
probably 😅
Exanite
ExaniteOP2mo ago
Also concerning the Bit vs ProtectedBit issue for SubmitFlags, should I just special case these in the generator config. It looks like there is an option for that. This would also cover cases like IndirectStateFlagsNV where there is only one enum member, meaning we can't determine a common prefix.
No description
Perksey
Perksey2mo ago
ah yes, prefix overrides they’re perfect for this you’re right something I carried forward from BuildTools (originally added by Beyley) and just haven’t used yet
Exanite
ExaniteOP2mo ago
Alright, that sounds good I think I should be good for a bit regarding questions Thanks for the help and discussion 😄
Perksey
Perksey2mo ago
no worries, all very good points, thank you for keeping on with this!
Exanite
ExaniteOP2mo ago
Interesting Vulkan Video flags
No description
Exanite
ExaniteOP2mo ago
No description
Exanite
ExaniteOP2mo ago
I feel like these should just be converted to normal Flags enums I wonder if Clangsharp has a setting for this or if we have to write a transformer
Exanite
ExaniteOP2mo ago
I replaced the TransformFlags mod that I added with TransformEnums, which also handles the removal of the max enum members that Vulkan uses. Because the max enum value members are a C pattern rather than a Vulkan/Khronos specific pattern, I decided to handle it here instead:
No description
Exanite
ExaniteOP2mo ago
No description
Exanite
ExaniteOP2mo ago
MemberName matches using Regex (which means that things like IdcMaxEnum also gets matched). MemberValue parses the string and compares the integer values of the enum members. One gotcha is that this mod is at the end of the mod order so it checks against the prettified name (MaxEnum) instead of the native name (MAX_ENUM). Another reason I changed the removal of MaxEnum members to be at the end of the pipeline is that it also affects trimming. If I remove these MaxEnum members too early, then trimming can't use the names of the MaxEnum members to determine common prefixes, which leads to the SubmitFlags.Bit situation. From what I can tell, deferring the removal of these MaxEnum members makes it so that we don't need to declare prefix overrides, which improves maintainability.
Exanite
ExaniteOP2mo ago
I decided to completely remove any constants that are marked as deprecated="aliased" in the Vulkan XML. These all represent misnamed constants that have been renamed. Keeping these causes name collisions and makes the trimmer select a more pessimistic common prefix. For example, VK_STENCIL_FRONT_AND_BACK forces the trimmer to select VK_STENCIL as the prefix instead of VK_STENCIL_FACE:
No description
Exanite
ExaniteOP2mo ago
----- These need to be transformed to use the MaybeBool type right? I see that SDL3 already uses it so I'll see how to get the transformation to also work for Vulkan.
No description
Curin
Curin2mo ago
It is part of TransformFunctions. OpenGL does it for GLboolean. you can check the generator doc.
Exanite
ExaniteOP2mo ago
Awesome, thanks for the tip I'll check it out
Curin
Curin2mo ago
if it is coming out of clangsharp as a uint though you might need to add it as a remapped name
Exanite
ExaniteOP2mo ago
Hmm, that only applies the transformation to functions and not structs
Curin
Curin2mo ago
yeah, then probably add it to your type remapping in the rsp
Exanite
ExaniteOP2mo ago
Yup, that worked 😄
Exanite
ExaniteOP2mo ago
@Perksey Silk 2 had constructors defined for each of the generated structs. Silk 3 is currently missing these. Do we want to add them? I rarely ever used them, but they were nice in certain situations:
No description
Perksey
Perksey2mo ago
so the reason for this was to have defaults for the fields e.g. SType but given that default constructors for structs are a thing now we can just use that instead
Exanite
ExaniteOP2mo ago
Ah interesting I've been just setting the SType fields manually in my own code I'll add setting default values for each of the structs to my todo list. Probably will involve scraping the Vulkan XML for this data. @Perksey Also, how do we feel about the Vulkan Video bitfield structs? I'm not familiar with how they work in C/C++, but these look to be equivalent to C#'s Flags enums for the most part. Do we want to transform these from bitfield structs to Flags enums (where applicable)?
Perksey
Perksey2mo ago
it’s probably safer and more correct to keep them as bitfields if that’s what they are in native I think that’s what we did in 2.X as well
Exanite
ExaniteOP2mo ago
Yeah I'll keep them as is and look into fixing the property naming Also, there's one more trimming issue. This one is also present in Silk 2 PFNVkDebugUtilsMessengerCallbackEXT and other function pointer structs don't get the Vk prefix trimmed properly. Do we care about fixing this? It's similar to the situation with the handles structs --- I feel like we need a way for mods to apply things in two passes: once before trimming and once after. But more generally, mods need a way to execute both before and after their following mods. It's like how middleware work in ASP.NET. The middleware gets info about the request going in and again when the response goes out. In other words, the mods first get executed sequentially (as they are now), but then we do another pass where we execute them in reverse order. Ideally most mods are kept simple and just use the forward pass, but having the second reverse pass helps with overriding things applied by later mods.
Perksey
Perksey2mo ago
middleware pattern sounds good to me
Exanite
ExaniteOP2mo ago
Alright It should be pretty easy to implement as well The only downside is that it makes mods slightly less intuitive, but it likely makes the code simpler as well so there's that tradeoff I also want to split ExtractNestedTyping into multiple mods It's really weird how everything is getting thrown in there I'll look into it more in a separate PR though Okay, I'm starting to look into the vtable stuff for Vulkan. There's other stuff I still need to work on for the Vulkan bindings as a whole, but I decided to move them to a separate PR since they are non-essential and likely interacts with the other bindings. My initial questions are the following: 1. What's the process of loading Vulkan function pointers? - I've never done this before since I just relied on Silk 2 doing it for me 😁 - From what I can tell, there are 3 ways of loading the pointers: vkLoadDeviceProcAddr, vkLoadInstanceProcAddr, and DllImport - Eg: vkLoadDeviceProcAddr is the fastest because it directly loads it from the driver being used. DllImport is a fallback. - The way you described it in our initial conversation implies that we just try each of them in order and store the function pointer in "slots". - I assume this refers to private readonly unsafe void*[] _slots = new void*[696]; - Should my code write directly to this array? - Since vkLoadDeviceProcAddr requires the device, do I invalidate (clear) the slots and rebuild it when a device is created? Same for vkLoadInstanceProcAddr. - When should the loading happen? When the device/instance is created OR when a new function is called? 2. Where should the code go? - Would this go into the AddVTables mod or into a separate mod? - I'm thinking of adding a separate Vulkan-specific mod that goes before AddVTables - This of course depends on the exact requirements (how much information it needs, how deeply does it interact with AddVTables, etc). - I currently want a solution that works as a proof-of-concept, then I'll refactor it to fit in the codebase better (especially if there are any other APIs that require this pattern (OpenGL?)) - From our initial conversation, I think you were thinking of adding a new interface that AddVTables hooks into. 3. What areas of the codebase should I look into as I work on this? - Eg: Silk 2, Silk 3. - I've been looking at the Silk 2 bindings, but I'm not making much sense of it just yet. This is the link to your initial message for reference: https://discord.com/channels/521092042781229087/587346162802229298/1375558458484592801
Perksey
Perksey2mo ago
1: So in 2.X instead of using an INativeContext that loads the function pointer as DllImport would, it instead returns a specific native context that will try to get the DeviceProcAddr, InstanceProcAddr, or the native loaded proc addr (in that order). It does this using a CurrentDevice/CurrentInstance property that is implicitly set by some code injected into CreateDevice/CreateInstance. These properties then implicitly swap out a "VTable" (a type that maintains a list of function pointers i.e. _slots) based on the active device. You can read more about VTables in 2.X here. In 3.0 in essence I want it to be the same (Vk having properties that track the CurrentInstance/CurrentDevice), with the exception that it should not allow multiple device/instance combinations (as this I found is pretty inefficient and is kinda hell to manage, constantly switching out those property values to switch vtables, I'd much rather encourage our users to create multiple IVk instances). So basically _slots will always be the same array, but they will be populated differently throughout the IVk instance's lifecycle. The _slots values are automatically populated as part of the calls generated by AddVTables, they essentially call out to the INativeContext captured by the Vk instance. The best way therefore to do this is likely to have an implementation of INativeContext that will call vkGetDeviceProcAddr and vkGetInstanceProcAddr (in that order), with the exception of vkGetInstanceProcAddr which should be a function pointer to an UnmanagedCallersOnly method that calls a DllImport function. This indirection is to get around some odd compiler behaviour we've had in the path, while still allowing us to use DllImport's "magic" for e.g. static linking (DirectPInvoke). We will then have a manual Vk.Create() that will then forward to the generated Vk.Create(INativeContext). No codegen changes needed. Yay! This custom implementation will need to capture the Vk instance (recursive object models, I love GC languages) so that it can get access to the public Instance/Device properties. We should probably also have a Clone method on IVk so that you can have multiple IVk instances sharing the same InstanceHandle without having to reload all the function pointers. But again, all manual stuff. The only codegen change will likely be for injecting the Instance/Device assignment (probably wouldn't hurt to throw in a pre-execution check to ensure that those properties are nullptr prior to execution as well). For the rest of it, look to the OpenGL bindings that are already in 3.0.
Should my code write directly to this array? Since vkLoadDeviceProcAddr requires the device, do I invalidate (clear) the slots and rebuild it when a device is created? Same for vkLoadInstanceProcAddr.
You can generally assume that a function loaded without a specific handle will continue to work once you have a specific handle, but it is atypical that a function that is optimally sourced from vkGetDeviceProcAddr with a specific handle is loaded before you've created a device (and same for instances).
When should the loading happen? When the device/instance is created OR when a new function is called?
Loading is lazy as above and as generated today. 2: hopefully answered with the above but lmk if not. 3: The OpenGL bindings is the most obvious case where we need to load a function pointer instead of a simple DllImport, so it'll be the same story as there. Maybe instead of ThisThread we want a non-thread-dependent Global (mainly because Vulkan is not thread-dependent) that functions the same way but with a normal static property or something.
PFNVkDebugUtilsMessengerCallbackEXT and other function pointer structs don't get the Vk prefix trimmed properly. Do we care about fixing this?
this is a good point, maybe we should look through the entire trimming name to see if there's any matches to a majority prefix (or the global prefix hint), but this likely wouldn't be an easy change, given that the current prefix determination logic is "it is a prefix or it isn't" yeah, I suppose you're right. we can add configuration knobs for the various passes, but at that point why is it even a single mod and not lots of little mods. but yeah, another PR.
Exanite
ExaniteOP2mo ago
Thanks for the incredibly detailed response 😄 I think I got most of the info I need and the rest I can get from reading the code and the resources that you linked The "middleware" pattern I explained above actually is meant to solve this issue. The idea is that prefixes/suffixes are added in the second pass after prettification is done. The hard part is tracking which exact structs need the PFN added to their name. I'm thinking of adding a temporary attribute that marks the PFN structs.
Perksey
Perksey2mo ago
tbh Pfn structs should be Transformed anyway and therefore not matter for prefix determination
Exanite
ExaniteOP2mo ago
How should I identify the vkCreateInstance/Device methods? Both of these screenshots are with AddVTables disabled. CreateInstance is the method with prettification. The other is without. I can use the DllImport attribute or hardcode the method to identify it (eg: Look for CreateInstance), but those feel hacky and indirect. Maybe we want a NativeName attribute?
No description
No description
Perksey
Perksey2mo ago
GetNativeFunctionInfo is your friend
Exanite
ExaniteOP2mo ago
Thanks, that looks like it'll work 😄 I'll take a look at the rest of the file while I'm at it
Exanite
ExaniteOP2mo ago
Alright, update time. The first section here is mainly here as an update and to make sure everything looks right. The second part I'd like to discuss more though. --- The attached file shows what I currently have generating. This is before AddVTables is ran and with the non-relevant members removed (to make it easier to read). Note the non-static storage members: private InstanceHandle? _currentInstance;, etc. And the modified DllImport methods: private static extern Result CreateDeviceInternal(, etc. Notably, AddVTables doesn't like my DllImport methods since you can't implement an interface using a private member. Because of these issues, I think it's better to do the transformation after AddVTables is ran, instead of before. --- The other problem is I don't think any of the existing VTable types fit Vulkan (DllImport, ThisThread, StaticWrapper). We probably want an instanced vtable as the default and an instance wrapper that allows the API to be used statically. Out of the existing ones, ThisThread seems to fit the best, but Vulkan as an API isn't thread local/specific.
Exanite
ExaniteOP2mo ago
I should mention that I'm not entirely confident on my knowledge of the existing VTables yet. I'm still working on reading through the code and trying to understand how they exactly work.
Perksey
Perksey2mo ago
seems like it works I don't think we need to change the vtables at all though we could probably do the code injection as a function transformer ah, but that would've already run at this point hmmmm I guess really there needs to be a way to customise the generation of the top-level Vk/INativeContext-driven implementation perhaps to allow us to generate a partial method call instead of using a function pointer call
Exanite
ExaniteOP2mo ago
I guess really there needs to be a way to customise the generation of the top-level Vk/INativeContext-driven implementation perhaps to allow us to generate a partial method call instead of using a function pointer call
I'm not fully following your messages here However, I think I see a way forward. I'll do some experimenting and see if I can't get it to work. The good part is that I think writing the syntax node generation code was the hardest part of all of this and I already got that out of the way. It's kinda crazy writing 5-10 lines of code per actual line of code generated.
Perksey
Perksey2mo ago
basically instead of emitting InvocationExpression(FunctionPointerType, ...)) the trampoline implementation could emit an invocation of a method, generated as a partial method for that entrypoint-signature combination (which then has its implementation in manual code) actually wait that's not the problem actually no that would work basically the logic here is that we don't want to change the manual file everytime we add a new overloader (hopefully this is rare in 3.0), so we intercept the native call and do our stuff in there
Exanite
ExaniteOP2mo ago
When you say overloader, you're referring to the raw pointer vs Ptr<T> overloads (and whatever overloads we add in the future), right?
Perksey
Perksey2mo ago
yep sorry transformer is the new language
Exanite
ExaniteOP2mo ago
Ah, so instead of calling through _slots[63], etc, we call another method that calls _slots[63]. This other method is the one that we rewrite to include the Instance/Device setting.
No description
Perksey
Perksey2mo ago
yeah essentially (although if we're doing this we should probably generate consts for the slot indices so we're not hardcoding 63 :P)
Exanite
ExaniteOP2mo ago
Hmm, I need to regenerate the bindings again to see how the vtables generate when TransformFunctions is included I disabled most of the mods for testing purposes earlier
Perksey
Perksey2mo ago
the transformed functions are included in the vtable interface generation this is why this strategy would need to track the entry point/native signature combinations and deduplicate the methods
Exanite
ExaniteOP2mo ago
I'm currently generating with all mods, except my WIP TransformVulkan mod. I only have the ThisThread VTable generated. This is the only method that actually calls using the slots array. The other variants (static/instance, overload) call into this method, so rewriting this method should be enough.
No description
Exanite
ExaniteOP2mo ago
This is the CreateInstance method from the generated DllImport table. Since all other variants call into one of these two methods, we only need to rewrite these two methods. This should be general enough to work for any API. Also, instead of rewriting the body of the methods, it's probably easier to add a wrapper method, similar to what I'm already doing (private and rename the original, add a new method with the original signature that calls the original). My mental model for this currently assumes this is done after both TransformFunctions and AddVTables, mainly because I'm unfamiliar with the implementation details of the two mods.
No description
Perksey
Perksey2mo ago
hmmm ok yeah that should be fine
Exanite
ExaniteOP2mo ago
Also, I think doing the transformation after everything else makes more sense because we are wrapping the original method. Since the hard part of doing this transformation is identifying which methods we need to transform, something that would make this easier is if AddVTable/TransformFunctions marked which overload is considered a "root" overload (for lack of a better term). Root overloads are overloads that call directly into native code, similar to the two methods above.
Exanite
ExaniteOP2mo ago
It's hard to distinguish the raw pointer (which is a root overload) and the wrapped pointer (which isn't a root overload) overloads otherwise:
No description
Exanite
ExaniteOP2mo ago
I'd have to look at the parameter typing, but that assumes a lot about what function transformers exist.
Perksey
Perksey2mo ago
this is why i thought looking for the function pointer call would be slightly easier even if it feels more complex
Exanite
ExaniteOP2mo ago
Ah, as in looking through the method body?
Perksey
Perksey2mo ago
yep
Exanite
ExaniteOP2mo ago
That should work then DllImport / static extern is another case, but that one is easy
Perksey
Perksey2mo ago
1. visit method, check native function info 2. if createinstance/createdevice, look for function pointer type or the other way around, doesn't make a difference I only care about the Vk implementation
Exanite
ExaniteOP2mo ago
Yeah, that's fair I was considering this from how generalizable this is to other APIs
Perksey
Perksey2mo ago
if you're using the DllImport vtable we're not loading the function pointers anyway so the user is on their own to optimise the function pointer loads (in reality, the vulkan loader will just act as a trampoline, so less efficient per call but gets there in the end. this is why I prefer us loading the funciton pointers ourselves as we get a direct driver function pointer that way)
Exanite
ExaniteOP2mo ago
Alright, I should be good for a while on this Thanks again for the help and discussion
Perksey
Perksey2mo ago
no worries, you're doing us a solid by keeping up with this really quite non-trivial work, so I massively appreciate it and happy to help in any way I can
Exanite
ExaniteOP2mo ago
The instance methods are missing the SupportedApiProfile and NativeFunction attributes. Is this intended?
No description
Exanite
ExaniteOP2mo ago
I can still identify the method by looking at LoadFunction, but without the NativeFunction attribute, I can't use GetNativeFunctionInfo.
Exanite
ExaniteOP2mo ago
These are explicitly getting dropped in AddVTable.Rewriter: I'm guessing the reason is because these are explicit interface implementations and don't need the attributes when we add the API profile analyzers.
No description
Exanite
ExaniteOP2mo ago
I'm going to undrop them since I need them 😅
Exanite
ExaniteOP2mo ago
The bindings compile and everything looks to be as expected! I think the code generation is done now 🎉 🎉 🎉
No description
Perksey
Perksey2mo ago
I don’t think it’s intended no awesome news! time to get an example or two up and running I guess! well, after the manual bits are done ofc off the top of my head: - the custom INativeContext as discussed previously - a manual Create() to instantiate Vk with the custom native context I think that’s it?
Exanite
ExaniteOP5w ago
Yup, I implemented those along with Clone() yesterday. I'm thinking of using dfkeenan's tutorial repo and converting that over to Silk 3. However, I need to fix how my location transformer / renaming code works on Curin's branch, so I'm going to do that first (https://discord.com/channels/521092042781229087/587346162802229298/1397337296684060703)
Perksey
Perksey5w ago
ah fair enough, sounds good this link contains our plan for in-tree tutorials for vulkan btw it doesn't directly map into any existing tutorial set but we think it's a good set of step-by-step examples that we want to create at some point
Exanite
ExaniteOP5w ago
Hmm. I feel like I'm doing something wrong here lol
SilkMarshal.NativeToString(ref new Ptr<byte>((byte*)&extension.ExtensionName.E0).Handle)
SilkMarshal.NativeToString(ref new Ptr<byte>((byte*)&extension.ExtensionName.E0).Handle)
No description
Exanite
ExaniteOP5w ago
Ayy, it works! Still really unfamiliar with how the Ref/Ptr types work, but I got dfkeenan's example 15 working Everything is very hacky since I just did whatever I needed to in order to get it running
No description
Exanite
ExaniteOP5w ago
Really surprised that line actually worked
Curin
Curin5w ago
I'm pretty sure we put string conversions in Ptr<byte>, etc. so I think that you could even just cast and it will do something similar behind the scenes
Exanite
ExaniteOP5w ago
I saw the ReadToString extension, but I haven't looked too much farther. It's 3 AM for me so I'll continue looking tomorrow.
Curin
Curin5w ago
same I'm also making choices
Exanite
ExaniteOP5w ago
I just wanted to speedrun the Vulkan triangle at 3 in the morning 🤣
Curin
Curin5w ago
The factory must grow (I need to go to bed)
Exanite
ExaniteOP5w ago
Yup (on both)
Exanite
ExaniteOP4w ago
Hmm, looks like I do have to do the enum remappings through ClangSharp My way is missing a single edge case and there's no way to get that information solely from the XML or generated code Doing the remappings through ClangSharp does fix it though I'll have to see how to automatically update the RSPs with the right remappings since I can get that from the XML
No description
Exanite
ExaniteOP4w ago
Got that to work Apparently I need both of my workarounds in place for correct generation There's two very similar edge cases when it comes to Flags/FlagBits enums Really hope that Vulkan doesn't combine both edge cases together. That would be fun to fix.
Exanite
ExaniteOP4w ago
Ahhh, dang it
No description
Exanite
ExaniteOP4w ago
This is how this property looked in Silk 2
No description
Exanite
ExaniteOP4w ago
Note that the property name not being prettified is a known bug
Exanite
ExaniteOP4w ago
This code needs two casts to be inserted in order to work
No description
Exanite
ExaniteOP4w ago
I feel like I need to open another ClangSharp PR Looks like ClangSharp already handles this to some extent It's probably my type remapping throwing it off I opened an issue with more information and a repro case: https://github.com/dotnet/ClangSharp/issues/618
Exanite
ExaniteOP4w ago
Vulkan has different shared library names on different platforms. How do we handle this in Silk 3? The bindings currently work fine on Linux, but fails on Windows
No description
Perksey
Perksey4w ago
see the LoaderInterface class
Exanite
ExaniteOP4w ago
I got a proof of concept working where I try to load the libraryNameHint library first, then fallback to a platform specific default. This involves writing a custom INativeContext though. I've been writing the native context here (https://github.com/Exanite/SilkVulkanTutorial/blob/3808a380282978816db7fa9e79b3f6221e298839/Source/15_HelloTriangle/Program.cs#L34) to avoid needing to recompile the bindings library. Do we want to have a more generalized way to do this (similar to Silk 2 where we provide a library name per platform) or do we want to implement this behavior per library? I also had to switch the Item1 and Item2 usages around in the screenshot below. They seem to be reversed.
No description
Perksey
Perksey4w ago
I don't think it needs to be that complicated if you LoaderInterface.RegisterAlternativeName (or whatever it's called) in a static ctor in Silk.NET.Vulkan, DllImport should be able to handle the rest and at that point you're just loading functions into _slots in the obvious way as previously discussed
Exanite
ExaniteOP4w ago
Ah, gotcha
Perksey
Perksey4w ago
I believe LoaderInterface.RegisterHook is called in the static ctor DllImport vtable, which would probably need to happen in the top-level Vk static ctor as seldom will the DllImport vtable be used (there are use cases for it, e.g. if you were static linking MoltenVk or some other driver without using the Vulkan Loader so we should probably still generate it, but it'll likely remain unused for 99.9999999999999% of use cases)
Exanite
ExaniteOP4w ago
Should I make it register a different alternative name per platform, or just register all of them?
Perksey
Perksey4w ago
I would register vulkan-1 and MoltenVK. the loader should be able to figure out the rest i.e. adding the lib prefix although I don't think it's smart enough to handle versioning, so there may be some cases where we have to bring some smart things forward from 2.X we'll see!
Exanite
ExaniteOP4w ago
LoaderInterface.RegisterAlternativeName works! 😄
Perksey
Perksey4w ago
awesome!

Did you find this page helpful?