BuildTools Vulkan
At the moment BuildTools creates a structure for each alias, even though identical to the root structure.
49 Replies
for example
PhysicalDeviceFeatures2KHR and PhysicalDeviceFeatures2 are both built, even though the first is identical to the second, and both use the same StructureType
That's not a problem until now, except when I add the logic to look up the start of the chain and mark it as IChainStart, of course we only find the root struct, not all of it's aliases
so the aliases don't get marked up as IChainable
I can 'fix' by doing a similar trick and marking the roots of all aliases with their aliases (using the Build Tools intrinsic attributes)- and then bounce on from there...
That's quite a lot of extra code, and there's a question as to whether we should be outputting the struct aliases really anyway as they're indistinguishable to the VulkanAPI and add bloat. However, I see the argument both ways.
Pros are the extra info marking structs as aliases, and saying what aliases exist for the current struct, is not unusefuluhhh
For example B extends A, and C is an alias of A.
oh yeah i understand
just
"uhhh"
I've finished so that B marks C as having an extension B (actually there is a list of all the extenders)
But C never gets marked up for obvious reasons
as it's just 'an alias'
Anything that extends
PhysicalDeviceFeatures2KHR cannot extend PhysicalDeviceFeatures2KHR
It's actually subtlely even worse
as not only do I have to say mark C as extended by A, but B also has be marked as being capable of exending C
So we get exponential growth of interfaces for each alias
which sucks
e.g. C->B->A and D is alias of B
currently we get
C->B
B->A
D->A
as well as
B<-C
A<-B
and we'd need to add
A<-D
D<-C
etc.
gets messy quick, but entirely possible to do
The alternative is to not support chaining on aliases at all
but that has downsides too
Or to not expose aliases
OK got to go, but it's actually not too bad as we only mark the reverse as IChainStart no matter how many extenders are present
ttyl8rcc @Deleted User above. do we want an exponential growth of interfaces for aliases, do we want to support the chaining on aliases of structs that have been promoted to core vulkan - could cause breaking changes if not, etc.
this is a vulkan-only change so i don't have too many concerns from my perspective with any case
actually
openxr is probably a massive concern
but we'll get there when we get there
Depends on how many we'd get
I can probably implement and see
As far as I can see there’s usually only one alias
So probably not going to be horrific
It doesn’t really impact performance at all
I also think we should just not export promoted structs
Yes alias structs are for backwards compatibility really
C# has typeforwarding
Oooooo
There’s something I didn’t consider 🤔
that's just moving a type to another assembly though
the type still has to exist in some form
Ah the full name must be identical
Well when promoted they do
yeah but we can't just forward Struct2KHR to Struct2 the types be magically fused together as one is my point
type forwarding has no place here.
none that i can see anyway
But we could eventually rename them all to Struct2
no way without breaking
And like, when stuff is promoted from KHR to Core, we just promote the type from the KHR package to the core package
all structs are in one package today
Ah right
Wups
and this doesn't change the fact that the KHR struct has... KHR at the end of it
and it also doesn't account for changes made during promotion, but those wouldn't be aliased
there's no way we can expose this in a way that's 1:1 with native without duplicating the struct [without source generators]
OK; I’ll implement as is
And see if there’s any issues
It might not be so bad
Yeah, we'll see
There are 116 aliases.
Should I add a
NativeName("Alias", "AliasName") attribute to the root of an alias?
of add a new Alias("AliasName") attribute?
or not add any attribute?
I have the info, so figure may as well mark it upNativeName("AliasOf", "...es2") sounds reasonableNativeName("Alias", "BKHR") on B indicates BKHR is an alias of B
To me AliasOf in that scenario feels backward?
Also there can be technically more than onewouldn't that result in multiple attributes on B
NativeName is already
AllowMultiple trueyeah i know, but that's intended for different native name types
I can dump a CSV into the value
NativesName("Aliases", "A,B,C")i would say do both, but we need to be more careful about assembly size...
I can do both no problem
NativeName("AliasOf", "D")
is always one way I believe
If I use the CSV for 'aliases' it's a max of two attributes, but in practice only one
e.g. A will have NativeName("Aliases", "B")idk again this only affects vulkan so i shall defer to kai
and B will have
NativeName("AliasOf","A")
I can stick it in the code, it's trivial to remove from StructWriter, as there's no harm the intrinsics being left against the structs
and not outputactually i think from a static analysis perspective AliasOf might be better
I put them both in, normally only one or the other will appear
we can always remove
they're not required
The intrinsics are useful to have around anyway 🤷♂️
And that's a complex one as it's both a chain start and a chain extension. Not particularly bloaty.
I just have to expand the extensions now with aliases
@Deleted User If you look
PhysicalDeviceFeatures2Khr has it's NativeName set to VkPhysicalDeviceFeatures2, shouldn't that be VkPhysicalDeviceFeatures2Khr?
That is existing functionality...
Wait nope
I borked something
Lemme look
(i.e. it isn't 'existing')
DOH fixed it
OK Maximum interfaces added (all of them) is 7, and that happened 5 times (all video ones!), e.g.:
Extending an alias doesn't happen too often, here's one:
And those aliases:
So nothing is particularly bloaty tbf
All 5 worst offenders ( VideoDecodeH264ProfileEXT, VideoDecodeH265ProfileEXT, VideoEncodeH264ProfileEXT, VideoEncodeH265ProfileEXT, VideoProfileKHR) are effectively that long because they legitimately extend 6 chains already:
- only one of which (FormatProperties2Khr) is an alias (of FormatProperties2)
So I think it's not worth stressing aboutI agree
The code is quite complex, but should be bombproof and isn't actually slower, mostly because I changed from building every alias individually to cloning structs to make aliases.
So there's an extra loop to build aliases, which is offset entirely by those aliases not being built in the first loop, and the new aliases being clones rather than built from scratch. And a second loop to build chains (which has lots of loops embedded sadly), but again is fast and bombproof. It will expand out aliases regardless of where you start them from.
Right, should be able to finish this PR, but not going to have power tomorrow as I'm replacing my consumer box.
Hmmm
MemoryRequirements2KHR has changed from MemoryRequirements2Khr, seem to be the only I can find though
I wonder if it changed in Vk.xml?
Ahhh
@Deleted User we have another problem
There's a few cases where the spec has changed case:
But the filesystem is not case sensitive
so VkMemoryRequirements2KHR.gen.cs gets overwritten with the class for VkMemoryRequirements2Khr as here
(similar for quite a few others)
That bug is already there
Because I add the aliases after I've built the structs, then the alias version overwrites the non-alias version...
Whereas before it was non-deterministic
I can, of course, ignore aliases that already exist (case-insensitive)
Actually, I can add a file disambiguator to the projectwriter...@Perksey is probably the person to ask here 😅
I'm trying to see if I can disambiguate (e.g. add a '2')
This is really weird VkMemoryRequirements2Khr appears nowhere in the spec.
Oooooo
I think there's a naming thing going on like KHR->Khr
OK, I think I'm there 🤔
The code in the Naming class is very… delicate…
check over your diff to ensure you’re using the same function
iirc vulkan struct names use TranslateLite
Yup, Its' corrected now
In fact it's fully working and compiling now
Going to do a draft PR, with just the code changes, then add a commit for the gen changes, to make it easier to review.
Hmmmm 🤔 there's actually lots of
IChainable structs, that is structs that are not a chain start and don't extend a chain (according to spec). e.g. AccelerationStructureGeometryInstancesDataKHR. These structs currently can't take advantage of the strongly typed chaining system and there are a lot of them.
IChainable is any type that has a StructureType SType in position one and T* PNext in position two (any pointer type but usually void*)...
There are 208 of them (out of 739)! That's almost 30%
😞Awesome (on the draft PR)! I’m in office tomorrow so won’t be as active as I usually am throughout the day, will give it a look in the evening though
I'm without power tomorrow too
Will have a think about what to do with all those 'orphaned'
IChainable's
OK, for unmanaged chains, I've implemented Any versions, (e.g. AddNextAny, etc.) These can substitute the equivalent non-Any method (e.g. AddNext) whenever you wish to 'loosen' the type checking, e.g. for situations when adding a struct that the specification has defined as an extension. That will handle the 208 other structs nicely.
There are still some types like PhysicalDeviceAccelerationStructureFeaturesKHR` with the capitalization, but that appears to be the case in the main branch too - https://github.com/dotnet/Silk.NET/blob/main/src/Vulkan/Silk.NET.Vulkan/Structs/PhysicalDeviceAccelerationStructureFeaturesKHR.gen.cshttps://github.com/dotnet/Silk.NET/pull/683 is ready for review
GitHub
Feature/vulkan struct chaining/2 unmanaged chains by thargy · Pull ...
Summary of the PR
Full implementation Proposal 2
Added IReadOnlyList<string> Extends property to StructureDefinition, to store the structextends values.
Added string Alias property ...
@Perksey did you get a chance to look at the
Any idea
As I move onto ManagedChain, I'm contemplating a similar concept, i.e. having a ManagedChainAny<T...>i don't particularly want to cause a type explosion, if there's a way to do this through methods only that would be good
but also no i haven't, feel free to link as i'm a bit behind
https://github.com/dotnet/Silk.NET/blob/5b96bfb78b1daee9c38d3fdab4fa27fc8310422c/documentation/proposals/Proposal%20-%20Vulkan%20Struct%20Chaining%20-%20%232%20Unmanaged%20Chaining.md#chain-extension-methods
That's the
Any implementation in unmanaged chains
The problem with ManagedChain is that the type constraints are on the type. However, one approach is to make ManagedChain<T..> itself only have the loose constraints (i.e. where T : struct, IChainable,... etc.) and to only put the tight constraints on the factory methods, e.g. ManagedChain.Create ... where TChain : struct, IChainStart, T1: struct, IExtendsChain<TChain> and ManagedChain.CreateAny ... where TChain : struct, IChainable, T1: struct, IChainable
That doesn't add any types, and I can possibly do something with adding constraints on the instance methods, or converting them to internal and adding static methods
static extension methods
I'll have a play
@Deleted User I'm not sure if you saw this? Or if it's your bag
OK, best approach is -
* Make ManagedChain<T...> have 'loose constraints
* Make all instance methods internal
* Expose all instance methods via static extension methods, default with tight constraints and Any version using loose constraints.
This adds no additional types.