Skip to content

Proposal/vulkan struct chaining #676

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
wants to merge 36 commits into from

Conversation

thargy
Copy link
Contributor

@thargy thargy commented Nov 6, 2021

Summary of the PR

This PR presents a set of proposals to improve the experience of chaining structures in Vulkan.

Related issues, Discord discussions, or proposals

Further Comments

The Initial commits import the history of the 'working' prototype from an external repo into /src/Lab/Experiments/PrototypeStructChaining. The prototype validated the design could be implemented and tested basic functionality, to make it easier to understand the proposals. It doesn't implement the BuildTool autogeneration or test passing the structures to/from the Vulkan API.

Lead Reviewer: @Perksey

thargy added 29 commits November 4, 2021 18:08
* 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.
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.
* 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.
@thargy thargy marked this pull request as ready for review November 8, 2021 13:06
* `Chain` was replaced by `BaseInStructure`, which is supplied by the
  Vulkan Specification already.
* `ChainExtensions` was renamed to `Chain`.
Added details about PRs and the new `Any` extension methods.
* 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.
Fixed broken pointer arithmetic.  Also changed to hash entire structure
for better separation.
@Perksey Perksey added this to the Next Working Group Meeting milestone Nov 29, 2021
@Perksey
Copy link
Member

Perksey commented Nov 29, 2021

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
Copy link
Contributor Author

thargy commented Nov 30, 2021

Closing this PR, as included in #693.

@thargy thargy closed this Nov 30, 2021
@thargy thargy deleted the proposal/vulkan-struct-chaining branch November 30, 2021 08:45
@thargy thargy mentioned this pull request Nov 30, 2021
14 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

2 participants