Skip to content
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

Buffer and textures nodes review #355

Open
azeno opened this issue Sep 18, 2020 · 8 comments
Open

Buffer and textures nodes review #355

azeno opened this issue Sep 18, 2020 · 8 comments
Assignees
Labels
VL.Stride Wrapper for 3d game engine Stride

Comments

@azeno
Copy link
Member

azeno commented Sep 18, 2020

Our buffer nodes need a review to be able to work with them easily.

Current state

  • DynamicBuffer taking IHasMemory<T>
  • DynamicArrayBuffer taking MutableArray<T>
  • StructuredBuffer taking size/count, unordered access and outputs the command list?!
  • Some Buffer.GetData and Buffer.SetData overloads, where only the ones with MutableArray<T> are somehow usable but still hard as they require the command list

Critic

We lack some needed ones

  • ArgumentBuffer
  • AppendBuffer taking element size / count
  • CounterBuffer - not sure?

Data access is inconsistent

  • A few use inputs of type IHasMemory<T> (implemented by Spread<T> and SpreadBuilder<T>) - an idea taken from Consider adding {ReadOnly}SpanMemoryStream types dotnet/runtime#22838 where it is still called IHasBuffer<T> as Buffer<T> was renamed to Memory<T> later I think. One however can't connect other common types like arrays - I guess therefor the need for DynamicArrayBuffer.
  • The GetData and SetData nodes use MutableArray<T>, ref T (makes no sense in VL) and DataPointer - a tuple of IntPtr and length as well as the unhandy command list input an end-user has no clue about.

Inconsistent in- and outputs

  • DynamicBuffer has options for IsVertexBuffer and IsIndexBuffer while all the others don`t have that
  • Some return the command list - because of the SetData GetData nodes?

Proposal

  • Structure the nodes based on the buffer types, which is IndexBuffer, VertexBuffer, StructuredBuffer, AppendBuffer, ArgumentBuffer and CounterBuffer
  • Make versions based on the usage Default (read-write), Immutable (read-only), Dynamic (gpu: read-only, cpu: write-only) and Staging (copy from gpu to cpu) - as long as they make sense and are supported, need to check API documentation
  • In general all versions except for Staging have a data input of type ReadOnlyMemory<T> and we provide an adaptive node AsReadOnlyMemory taking IReadOnlyList<T> with implementions for Spread<T>, SpreadBuilder<T>, MutableArray<T> and ImmutableArray<T>
  • There's a corner case where one wants to load bytes (like Memory<Byte>) and interpret the data in the shader. Feeding that to the buffer constructors usually results in a not supported exception as the graphics APIs require an element size of at least 4 bytes. This could be tackled by re-interpreting the Memory<Byte> as Memory<Int> - see https://stackoverflow.com/questions/54511330/how-can-i-cast-memoryt-to-another for an implementation. This raises the question why not use Span<T>? See discussion below.
  • All nodes have exactly one output of type Buffer<T>

As long as VL doesn't support implict type conversions on links, a AsReadOnlyMemory node would need to be placed upstream. But this reliefs the node set to get blown up completely by all type combinations. The performance impact should be neglectable as ReadOnlyMemory<T> is a struct and therefor causes no allocation.

Based on the above ideas we'd end up with the following node set:

Name (Default) (Immutable) (Dynamic) (Staging) Comment
IndexBuffer x x x - Takes ReadOnlyMemory<int>
VertexBuffer x x x -
StructuredBuffer x x x x Has Unordered Access input
ArgumentBuffer x - - - Used for indirect dispatch
AppendBuffer x - - - No data input
CounterBuffer x - - - Not sure, I guess also no data input
  • (Staging) would never have data input but only element count. Element size would gets infered from T.
  • (Dynamic) has Apply pin to write incoming data
  • StructuredBuffer (Default) will have UnorderAccess set. If one wants read-only use 'Immutable' or 'Dynamic' versions.

And further we'd provide GetData and SetData process nodes (dealing with the command list internally) again with ReadOnlyMemory<T> as data type.

Discussion

  • We could also say we take ReadOnlySpan<T> as input - but we would first need to sort out the compiler exception in VL - because a ref read-only struct is not allowed to be used as a type argument. The tracing mechanism would need to filter them out - how costly would that be? Backend would need to check every hub it traces.
@azeno azeno changed the title Buffer nodes cleanup Buffer nodes review Sep 18, 2020
@tebjan
Copy link
Member

tebjan commented Sep 18, 2020

Some notes on top of my head:

  • Get/Set buffer nodes could be renderers and have Auto render and renderer output to control the execution
  • There are some more buffers, for example, ConsumeBuffer and ByteAddressBuffer
  • I think flag enums are easier to manage than 10+ nodes, so we could look into creating a buffer description and a buffer creation node, then provide a few often used defaults as nodes
  • Some combinations are missing, for example, a StructuredBuffer can also be a VertexBuffer to bind them differently to different shaders -> flags re more easy to manage then, if we don't want to make a StructuredVertexBuffer
  • We should also have to look into SRVs for example, one can re-interpret a StructuredBuffer as VertexBuffer or the other way around
  • more later...

@tebjan
Copy link
Member

tebjan commented Sep 19, 2020

For reference, these were the things we used in Xenko/Ocean of air. the CopeResource, CopyCounter, ... (should be renderers) and two different indirect args buffers one for DrawIndirect and one for DispatchIndirect:

image

I think that the Usage should stay as an enum and have defaults... also additional flags should be possible (Vertex/Index for compute buffers).

interesting articles:

Shader Model 5 Objects

Let’s Close the Buffer Zoo

Unity Doc: ComputeBufferType

@tebjan
Copy link
Member

tebjan commented Sep 19, 2020

  • We also need the StreamOut option for VertexBuffers

@antongit
Copy link
Member

I guess DynamicTexture nodes are also missing.

@tebjan tebjan changed the title Buffer nodes review Buffer and textures nodes review Nov 4, 2020
@antongit antongit mentioned this issue Mar 1, 2023
34 tasks
@azeno
Copy link
Member Author

azeno commented Feb 9, 2021

@tebjan suggests to first introduce one Buffer [Graphics] - Advanced and Texture [Graphics] - Advanced nodes to rule them all. Some of its input combinations won't work (as seen in the matrix above) - the node will throw an error (turn pink).

Based on the usage of it we can post pone the introduction of more easy to use nodes.

@tebjan
Copy link
Member

tebjan commented May 10, 2021

Low-level nodes:
SetData Texture+Buffer

  • Use default command list, if command list input is null
  • Input data is DataPointer/Box

Texture/Buffer (Advanced)

  • low-level node
  • only creates resource from descriptions
  • view description could be option/nullable input

Texture2D

  • high-level node, single surface initial data input

High-level nodes:
DynamicTexture2D

  • with SetData internally
  • Is Immutable flag?

DataPointer/Box

  • Array, Spread, Image

@gregsn
Copy link
Member

gregsn commented Jun 7, 2021

SetTextureData, SetData, BufferData, InitialData, TextureIntitalData shall all use the same data type.
We want one node of input T that outputs an instance of one central data type capturing the dimensionality and pinning ideas. The main responsibility is to provide graphics-API compatible datatypes (like IntPtr) from the input T.


We want to focus on a databox-like construct that doesn't treat the 1d case as special. We want to have a pinnable DataBox.

  • PinnableDatabox
    • Simpler Signature
    • has the mini-advantage that the pin- and unpin-methods are allocation-free (No ResourceHandle class instance)
  • Provider<DataBox>
    • has the advantage that it comes with operators for fine-grained timing of the pin and unpin moments. PROBABLY NOT NECESSARY. Pin & Unpin often!
    • No new invention
    • We would need a new region that allows dealing with Handles directly. It would allow disposing the handle (unpin), not the resource. This region would be similar to the New (BeforeDispose), in that it allows to call a custom delegate that unpins in our case.

tebjan referenced this issue in vvvv/VL.Stride Jun 9, 2021
tebjan referenced this issue in vvvv/VL.Stride Jun 14, 2021
tebjan referenced this issue in vvvv/VL.Stride Jun 17, 2021
tebjan referenced this issue in vvvv/VL.Stride Jun 26, 2021
tebjan referenced this issue in vvvv/VL.Stride Jul 1, 2021
tebjan referenced this issue in vvvv/VL.Stride Jul 1, 2021
@tebjan
Copy link
Member

tebjan commented Jul 2, 2021

We now got:

  • complete set of easy to use building blocks to create any buffer or texture supported by DX11, including views
  • simple nodes for most common use cases: immutable, dynamic and compute for common resource types
  • an easy, complete and generic way to pass Spread, (Mutable)Array, Image (and Strings) to graphics resources
  • an easy way to create vertex and index buffers in compute shaders

Not yet:

  • easy to use special interest buffers (Argument, Append/Consume, Counter)
  • Cube and Array textures

these would need some investigation into use cases (cube textures, append/consume/counter buffer, auto draw with copy counter) and make the nodes according to them.

@joreg joreg transferred this issue from vvvv/VL.Stride Mar 1, 2023
@joreg joreg added the VL.Stride Wrapper for 3d game engine Stride label Mar 1, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
VL.Stride Wrapper for 3d game engine Stride
Projects
None yet
Development

No branches or pull requests

5 participants