Feature/vulkan struct chaining/3 managed chains #693

thargy · 2021-11-15T14:04:09Z

Summary of the PR

Full implementation of Proposal 3

Adds ManagedChain.gen.tt class, which adds ManagedChain, ManagedChainExtensions and 16 ManagedChain<TChain...> classes.

Related issues, Discord discussions, or proposals

Further Comments

This is currently functionally complete.

Lead Reviewer: @Perksey

* Created class library with 3 chainable structs * Added `IChainable` interface hierarchy * Added chaining methods `Chain`, `CreateNext`, `SetNext` and `End` * Added tests

* Added tests to check that invalid structs don't compile * Minimal additional code required per struct - just add an interface for each allowed `PNext` struct; and, - add a short static method `Chain` (technically optional), and a short instance method `SetNext`.

Retargeted project to ensure that it only targets the same includes, etc. as `Silk.Next.Core` and `Silk.Net.Vulkan`

* Added `IStructureType` for any struct that has a `StructureType SType` field. This is usually added automatically by one of the other interfaces; and allows the structure type to be automatically corrected and retrieved using the `StructureType IStructuredType.StructureType()` method. * Added `IChainStart` to indicate a type that can form the start of a chain. * `IChainable<...>` was replaced with `IExtendsChain<T>` which matches the Vulkan specification more readily. * The chaining methods now all accept, and return, a reference to an `IChainStart`, making it easier to scan a chain for existing items, and making use of the auto-generated `IExtendsChain<T>` to validate types at compile time. * `SetNext`, by default, will now replace existing chain entries if they match the supplied `SType`. This behaviour can be overriden with the optional `alwaysAdd` parameter. * `CreateNext` is now called `AddNext` to indicate it always adds to the end of a Chain.cs * `TryAddNext` added to only create a new structure if it is not already there. * `IndexOf` added to return the index of a structure in a chain. * Updated Readme.md.

Updated `Changes Required` section.

* ManagedChain holds structures in unmanaged memory, preventing their movement, and it can be safely held on the heap itself. * It allows for easy modification and manipulation of the chain.

`ManagedChain` now supports 2-16 chain items.

Can now easily append an item to an existing `ManagedChain` to efficiently create a new `ManagedChain`.

`ManagedChain` now has a constructor that will load an unmanaged chain allowing it to be safely stored on the heap.

Also improved loading to detect when the unmanaged chain is too long.

…al repository

Also added the new blank proposals.

Proposal - Vulkan Struct Chaining - #1 StructureType correction.md

* Restored targets for PrototypeStructChaining.csproj to match `Silk.Net.Vulkan` targets. * Upped the generated `ManageChain<T...` to 16.

* IChainable now exposes `Chain* PNext` property. * Added `ManagedChain<TChain>` as smallest chain (just the head) * All `IntPtr`s are replaced with `nint`s. * Pointer offsets are calculated once per chain type and stored in statics, meaning that a chain contains a single `nint` field internally, making it incredibly lightweight. Getting the sizes once may also improve the performance, and has simplified the code. * Various fixes to ensure tail always has `PNext` set to `null` (0). * Added `Truncate` instance methods.

Also cleaned up some tests.

* Added the `Extensions`, `Extenders`, `ClrTypes` and `StructureTypes` dictionaries to `Chain`. * Added the `ClrType`, `StructureType`, `IsChainStart`, `IsChainable`, `CanExtend` and `CanBeExtendedBy` extension methods to `ChainExtensions`. * Finished Proposal dotnet#4 on Chaining Metadata extension.

* Removed the manually generated code that is replaced by the auto-generated code now that it is implemented * Removed the solution. * Corrected namespace to `Silk.NET.Vulkan`. * Fixed issue clash of `System.Buffer` and `Silk.NET.Vulkan.Buffer`.

As ~30% of Vulkan Structures are purely `IChainable`, that is they are not included in one of the Vulkan Specification `structextends` chains, but do implement `SType` and `PNext` correctly, I have added `Any` versions of each of the unmanaged chain extension methods. These methods accept `IChainable` directly for the `chain` and `next` parameters, making them work with any `IChainable` structure. Indeed the stricter non-`Any` methods, call the looser `Any` methods directly and serve merely as more rigorous compile time checks. Improved the inline documentation of the extensions methods.

Added details about PRs and the new `Any` extension methods.

…ruct-chaining/1-istructuredtype

…feature/vulkan-struct-chaining/2-unmanaged-chains

* Removed instance methods and converted into static extension methods. * Added `Any` equivalents with looser constraints. * Various fixes to make prototype match more closely with current draft implementations, and `Silk.NET.Vulkan`. * Added tests. * Added efficient equality overloads. * Added ToString(). * Expose the `HeadPtr` and `Size` in the base class.

This is a proposed implementation that only checks the start of the payload. We may change this to check the full payload Also updated the proposals to match the new `Any` functionality.

…ruct-chaining/1-istructuredtype

…feature/vulkan-struct-chaining/2-unmanaged-chains # Conflicts: # src/Lab/Experiments/PrototypeStructChaining/PrototypeStructChaining.Test/TestChainsAny.cs # src/Lab/Experiments/PrototypeStructChaining/PrototypeStructChaining.Test/TestManagedChains.cs # src/Lab/Experiments/PrototypeStructChaining/PrototypeStructChaining/BaseInStructure.cs # src/Lab/Experiments/PrototypeStructChaining/PrototypeStructChaining/ChainExtensions.cs # src/Lab/Experiments/PrototypeStructChaining/PrototypeStructChaining/DeviceCreateInfo.cs # src/Lab/Experiments/PrototypeStructChaining/PrototypeStructChaining/IStructuredType.cs # src/Lab/Experiments/PrototypeStructChaining/PrototypeStructChaining/ManagedChain.gen.cs # src/Lab/Experiments/PrototypeStructChaining/PrototypeStructChaining/ManagedChain.gen.tt # src/Lab/Experiments/PrototypeStructChaining/PrototypeStructChaining/PhysicalDeviceAccelerationStructureFeaturesKhr.cs # src/Lab/Experiments/PrototypeStructChaining/PrototypeStructChaining/PhysicalDeviceDescriptorIndexingFeatures.cs # src/Lab/Experiments/PrototypeStructChaining/PrototypeStructChaining/PhysicalDeviceFeatures.cs # src/Lab/Experiments/PrototypeStructChaining/PrototypeStructChaining/PhysicalDeviceFeatures2.cs # src/Lab/Experiments/PrototypeStructChaining/PrototypeStructChaining/StructureType.cs

Fixed broken pointer arithmetic. Also changed to hash entire structure for better separation.

…ruct-chaining/1-istructuredtype

…feature/vulkan-struct-chaining/2-unmanaged-chains

…ged-chains

thargy · 2021-11-30T08:41:45Z

COPIED OVER FROM #676 (comment)

All proposals provisionally approved, there are no significant comments and this won't need to come back to a meeting if these are addressed. Discussed in Working Group Meeting 29/11/2021.

StructureType correction

Must be explicit, must be implemented in a specific way.
- Invisible to users
Get-only property implies it's just getting, so it makes sense being a method.
There's a concern if we decided to make everything readonly in the future.
- Could probably use withers when we get to that place.

Consensus

Yes.

No discussion points, seems fine.

Unmanaged Chaining

OPEN QUESTION: OpenXR implications/applications?
- copy pasting is fine.
OPEN QUESTION: Should TChain be constrained to IChainStart?
- doesn't really matter, but good to check anyways.
"Chain interfaces (Optional)" no use case that we can think of, leave out [for now]. Can revisit if needs be.

Consensus

Happy.

Managed Chaining

Assuming ManagedCache refers to ManagedChain (this should be fixed before merge)
Ignoring actual implementation scrutiny for the purposes of proposal review
This would be the recommended strategy for chaining/the mainstay.
- unmanaged chain is specialized usage/"only if you know what you're doing"
Should the base class be able to modify the chain?
**OPEN QUESTION: ** Look at base append/truncate/duplicate. Not proposal critical, but would be nice to have.
Could possibly implement IList instead of IReadOnlyList (well, both...)
Append reminds us of StringBuilder/string things.

Consensus

Happy
Rename Append methods to Add
More optional comments, see above.

Metadata extensions.

Shouldn't use reflection in game engine libraries (could be a gotcha)
Pick n' mix of simple metadata additions which we can optionally add
Sort of already covered by other proposals
Proposal is basically "we already have this data, how do we/shall we expose this"
Could be an extension package?
Seems like the use cases are niche, unrealised, or otherwise not applicable or of concern to Silk.NET itself

Consensus

Revisit in the future if we have a use case
Merge as a "(Proposed) Proposal - " file and add a champion issue

thargy added 30 commits November 4, 2021 18:08

feat: Initial working sample

ea222f8

* Created class library with 3 chainable structs * Added `IChainable` interface hierarchy * Added chaining methods `Chain`, `CreateNext`, `SetNext` and `End` * Added tests

docs: Added Readme.md

babe08f

fix: Change __makeref to Unsafe.AsPointer

58865bf

chore: Retargeting

73763e6

Retargeted project to ensure that it only targets the same includes, etc. as `Silk.Next.Core` and `Silk.Net.Vulkan`

chore: Updated readme.md

aea15c9

Updated `Changes Required` section.

feat: Added ManagedChain

eca1dd2

* ManagedChain holds structures in unmanaged memory, preventing their movement, and it can be safely held on the heap itself. * It allows for easy modification and manipulation of the chain.

feat: Added static ManagedChain.Create example

389c1f9

feat: ManagedChain uses a single memory block.

e7f047d

feat: Added T4 template.

ff031f6

`ManagedChain` now supports 2-16 chain items.

feat: Added ManagedChain.Append

dd2c1a8

Can now easily append an item to an existing `ManagedChain` to efficiently create a new `ManagedChain`.

feat: Loading of unmanaged chains

ea96e2a

`ManagedChain` now has a constructor that will load an unmanaged chain allowing it to be safely stored on the heap.

feat: Added IReadOnlyList support to ManagedChain

ec40730

feat: Added deconstructor to ManageChain<...>

5254b9d

feat: Added static ManagedChain.Load

8d3f124

Also improved loading to detect when the unmanaged chain is too long.

chore: Readme.md updated

80dac45

Imported PrototypeStructChaining into src/Lab/Experiments from extern…

fe2baf7

…al repository

chore: Added Lab projects to Solution

e74c977

Also added the new blank proposals.

chore: Fixed issues with previous commit

3cae5cc

docs: Finished initial draft #1

ef57171

Proposal - Vulkan Struct Chaining - #1 StructureType correction.md

docs: Embedded code sample directly into Proposal #1

089f2e5

docs: Updated the Unmanaged Chaining Proposal.

d5552b6

chore: Minor fixes to lab

bd9f0eb

* Restored targets for PrototypeStructChaining.csproj to match `Silk.Net.Vulkan` targets. * Upped the generated `ManageChain<T...` to 16.

docs: Initial work on Managed Chaining Proposal.

1c8dbda

docs: Finished draft proposal for Managed Chains

af6c5a8

docs: Updated Chaining proposals to match new functionality.

d1ced73

Also cleaned up some tests.

docs: Added suggestion for specific chain interfaces

374d85e

thargy added 14 commits November 9, 2021 23:42

chore: Committing structure changes

055a809

docs: Updated the first two proposals

8152c47

Added details about PRs and the new `Any` extension methods.

Merge branch 'proposal/vulkan-struct-chaining' into feature/vulkan-st…

bc26afc

…ruct-chaining/1-istructuredtype

Merge branch 'feature/vulkan-struct-chaining/1-istructuredtype' into …

5b96bfb

…feature/vulkan-struct-chaining/2-unmanaged-chains

feat: Added GetHashCode implementation

aebe0d2

This is a proposed implementation that only checks the start of the payload. We may change this to check the full payload Also updated the proposals to match the new `Any` functionality.

Merge branch 'proposal/vulkan-struct-chaining' into feature/vulkan-st…

35cf0f5

…ruct-chaining/1-istructuredtype

fix: Fixed GetHashCode

930a2fc

Fixed broken pointer arithmetic. Also changed to hash entire structure for better separation.

Merge branch 'proposal/vulkan-struct-chaining' into feature/vulkan-st…

7efd6fa

…ruct-chaining/1-istructuredtype

Merge branch 'feature/vulkan-struct-chaining/1-istructuredtype' into …

294bc77

…feature/vulkan-struct-chaining/2-unmanaged-chains

feat: Moved ManagedChain.gen.tt to Silk.NET.Vulkan

8a2eb80

This was referenced Nov 15, 2021

Feature/vulkan struct chaining/2 unmanaged chains #683

Closed

Feature/Vulkan Struct Chaining/1 IStructuredType #680

Closed

Proposal/vulkan struct chaining #676

Closed

thargy marked this pull request as ready for review November 15, 2021 14:17

thargy requested review from HurricanKai, Perksey and a team as code owners November 15, 2021 14:17

Merge branch 'dotnet:main' into feature/vulkan-struct-chaining/3-mana…

4af9ffd

…ged-chains

thargy changed the title ~~Feature/vulkan struct chaining/3 managed chains~~ [Feature] Vulkan struct chaining Nov 30, 2021

thargy closed this Nov 30, 2021

thargy deleted the feature/vulkan-struct-chaining/3-managed-chains branch November 30, 2021 08:47

thargy restored the feature/vulkan-struct-chaining/3-managed-chains branch November 30, 2021 08:49

thargy deleted the feature/vulkan-struct-chaining/3-managed-chains branch November 30, 2021 08:50

thargy changed the title ~~[Feature] Vulkan struct chaining~~ Feature/vulkan struct chaining/3 managed chains Nov 30, 2021

thargy mentioned this pull request Nov 30, 2021

Vulkan struct chaining #706

Merged

14 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Uh oh!

Feature/vulkan struct chaining/3 managed chains #693

Feature/vulkan struct chaining/3 managed chains #693

Uh oh!

thargy commented Nov 15, 2021 •

edited

Loading

Uh oh!

thargy commented Nov 30, 2021

Uh oh!

Uh oh!

Uh oh!

Feature/vulkan struct chaining/3 managed chains #693

Feature/vulkan struct chaining/3 managed chains #693

Uh oh!

Conversation

thargy commented Nov 15, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary of the PR

Related issues, Discord discussions, or proposals

Further Comments

Uh oh!

thargy commented Nov 30, 2021

StructureType correction

Unmanaged Chaining

Managed Chaining

Metadata extensions.

Uh oh!

Uh oh!

thargy commented Nov 15, 2021 •

edited

Loading