Fix voxel GI on 4.4: SDSL mixer, D3D12 root signature, Vulkan MSAA - #3382
Fix voxel GI on 4.4: SDSL mixer, D3D12 root signature, Vulkan MSAA#3382Nicogo1705 wants to merge 20 commits into
Conversation
Ethereal77
left a comment
There was a problem hiding this comment.
Lets wait for xen, the new SPIR-V shader compiler is his child.
In the meantime, I've left some comments. The ones about docs are my personal opinion, to aim for a better, more clear and readable and less intimidating documentation for end users vs. information specific for mantainers.
Also, I like to reinforce my point that (from what I saw when it was being implemented) Stride SPIR-V is a Stride-specific dialect not specific to Vulkan, but cross-compiled to be used by DX APIs also. Do not assume when the word SPIR-V appears it belongs automatically to the Vulkan API.
| // Direct3D11 allows at most 65535 thread groups per dispatch dimension, so a purely | ||
| // one-dimensional dispatch cannot address more than 65535 * 1024 elements. A 256^3 | ||
| // anisotropic clipmap needs three times that, so the groups are spread over X and Y and | ||
| // the linear index is recomposed here. rowLength is how many elements one row of groups | ||
| // covers, and count bounds the last row, which generally overshoots. |
There was a problem hiding this comment.
As this is a limitation specific to Direct3D 11, maybe it would be better to enclose this in a
#if STRIDE_GRAPHICS_API_DIRECT3D11
...
#endifso as to not damage the performance of other APIs with extra data transfer or unnecessary computation.
Shaders also get those defines set when compiling. See here.
There was a problem hiding this comment.
The 65535 groups-per-dimension cap is not specific to Direct3D 11: D3D12's Dispatch documents the same D3D11_CS_DISPATCH_MAX_THREAD_GROUPS_PER_DIMENSION (65535) limit, and Vulkan's required minimum for maxComputeWorkGroupCount is 65535 per dimension too. So the 2D dispatch is needed on every backend, and it costs one multiply-add per thread. I've reworded the comment to say so rather than suggest a D3D11 branch.
Refs: https://learn.microsoft.com/en-us/windows/win32/api/d3d12/nf-d3d12-id3d12graphicscommandlist-dispatch and https://docs.vulkan.org/refpages/latest/refpages/source/Required_Limits.html
| /// <summary> | ||
| /// Thread group counts for clearing <paramref name="elementCount"/> buffer elements, spread | ||
| /// over two dimensions. | ||
| /// </summary> | ||
| /// <remarks> | ||
| /// Direct3D11 rejects a dispatch with more than 65535 groups in any one dimension, which | ||
| /// caps a one-dimensional clear at about 67 million elements. A 256^3 clipmap storing six | ||
| /// directions needs 201 million, so it failed outright. ClearBuffer recomposes the linear | ||
| /// index from X and Y, using <paramref name="rowLength"/> as the width of one row of groups. | ||
| /// </remarks> |
There was a problem hiding this comment.
This is a personal preference of mine, but I usually see XMLdocs as what I want to see in Intellisense or when hovering over some symbol. So, I'd keep just the bare minimum description about what the method does or what it is for, but those comments about D3D11-specific problems I'd move inside the method as regular comments. It does not add much value to a caller of the method that the implementation of a specific API has some drawbacks, imho.
There was a problem hiding this comment.
Done: one-line summary with param/returns, and the specifics moved inside the method as a comment.
| else if (declaration is UsingShaderNamespace) | ||
| { | ||
| // Ignore: shader classes are resolved by name across every loaded namespace, so a | ||
| // `using` at the top of an .sdsl file carries no information the compiler needs. | ||
| // Erroring on it took down every shader that has one — Stride.Voxels' | ||
| // LightVoxelShader among them, which made voxel GI unusable. | ||
| } |
There was a problem hiding this comment.
Excuse me if I'm misunderstanding this one.
using and namespaces don't work? Is this a regression or a problem with the new compiler?
The point of having namespaces in the SDSL language is the same as in C#: they serve as a means to avoid name clashes.
What's the point of using namespaces if they are not respected by the compiler?
There was a problem hiding this comment.
I don't know whether it is a regression - I haven't tried this on 4.3. What I can say is the current behaviour, which I've pinned down in tests (NamespaceTests): a shader class is resolved by its name alone (ShaderSourceManager.LoadShaderSource(type) loads {type}.sdsl), so a using takes no part in resolution; a using of a namespace no shader lives in is not an error; and two classes of one name in two namespaces of one file are not told apart by a using - the last declared one is mixed in. Rejecting the directive failed every shader that has one (Stride.Voxels' LightVoxelShader), so it is accepted and skipped. If namespaces are meant to take part in resolution, that is a separate change, and the tests will have to be updated knowingly.
| /// <summary> | ||
| /// The first unordered access register a Direct3D11 pixel shader may use. | ||
| /// <para> | ||
| /// D3D11 puts UAVs and render targets in one register space, so a pixel shader's UAVs have to | ||
| /// start past its render targets - FXC otherwise rejects the shader with X4509. The engine | ||
| /// already assumes this: CommandList.OMSetSingleUnorderedAccessView binds with | ||
| /// <c>UAVStartSlot: currentRenderTargetViewsActiveCount</c> and indexes | ||
| /// <c>slot - currentRenderTargetViewsActiveCount</c>, which is negative if a UAV took u0. | ||
| /// </para> | ||
| /// <para> | ||
| /// Counted as the highest output Location plus one, so multiple render targets are handled, | ||
| /// and left at 0 when the module has no pixel shader - a compute shader keeps u0. | ||
| /// </para> | ||
| /// </summary> |
There was a problem hiding this comment.
Similarly to my other comment, I think this <summary> contains too much too specific to D3D11 information.
I'd prefer to describe what the method does and a brief explanation of why in the remarks, and explain the D3D11-specific shenanigans in a comment inside for mantainers.
Oh, and as it is a method, I tend to prefer the verb-form both for the method name and the description (so GetFirst... and Retrieves the.... This is a personal take, however.
There was a problem hiding this comment.
Done: renamed to GetFirstUnorderedAccessSlot, short summary with remarks and returns, and the D3D11 register-space detail moved inside as a comment.
| // An image - texture or typed buffer alike - cannot deliver 16-bit values: Vulkan requires | ||
| // its sampled type to be a 32-bit int, 64-bit int or 32-bit float | ||
| // (VUID-StandaloneSpirv-OpTypeImage-04656), and there is no extension that lifts it. | ||
| // | ||
| // Nothing is lost. The 16 bits live in the resource's pixel format, not in the shader's | ||
| // declaration: the texture unit decodes the stored halfs and delivers 32-bit floats to the | ||
| // registers, for free. `half` is already an alias of `float` in shader model 5 anyway, and | ||
| // code that wants native 16-bit registers converts after the read. | ||
| static SymbolType WidenImageElementType(SymbolType elementType) => elementType switch |
There was a problem hiding this comment.
Does this not have any drawback? I mean, the old way was compiled succesfully by the old Stride / FXC shader compiler. Did it also widen under the hood?
I mean, even if when fetching a half it gets auto-converted to float for free by the HW, modern GPUs still have half-precission ALUs with better issue rate, if I'm not mistaken. I've seen shaders specifically optimized to use halfs because of this. Is this not valid anymore?
Note also that Stride SPIR-V is a Stride-specific dialect not specific to Vulkan, but cross-compiled to be used by DX APIs also.
There was a problem hiding this comment.
No drawback, and it is what FXC did: per the HLSL docs half is "provided only for language compatibility" and "Direct3D 10 shader targets map all half data types to float data types", so Texture3D<half4> already declared a 32-bit register under the old compiler. The 16 bits live in the resource's pixel format (R16G16B16A16_Float), not in the shader declaration - the texture unit decodes them and hands floats to the ALU. Native 16-bit arithmetic is min16float (or float16_t with 16-bit types enabled), a separate type that is unaffected. On the SPIR-V side, OpTypeImage's sampled type must be 32-bit for the module to validate, whichever backend it is cross-compiled to. The comment now states both.
Ref: https://learn.microsoft.com/en-us/windows/win32/direct3dhlsl/dx-graphics-hlsl-scalar
| /// <summary> | ||
| /// Whether an argument is handed to the callee as the caller's own pointer instead of being | ||
| /// copied into a function-local temporary. | ||
| /// <list type="bullet"> | ||
| /// <item>ref: atomic intrinsics (InterlockedAdd, etc.) need the actual memory pointer | ||
| /// (Workgroup, StorageBuffer, ...).</item> | ||
| /// <item>Opaque resources (see <see cref="SymbolTypeExtensions.IsOpaqueResource"/>): Vulkan | ||
| /// forbids OpStore to them, so they cannot live in a Function variable.</item> | ||
| /// <item>Geometry streams: appending goes through OpEmitVertexSDSL and the stage's output | ||
| /// variables, so the object holds nothing to copy - and the copy outlived the parameter, which | ||
| /// the interface processor removes from the signature, leaving SPIR-V reading an id that no | ||
| /// longer exists.</item> | ||
| /// </list> | ||
| /// The input and output sides both consult this: copying a result back out of something that | ||
| /// was never copied in would write through a pointer the callee already holds. | ||
| /// </summary> |
There was a problem hiding this comment.
Same thing I wrote above about comments.
This is mixing the description of the method with a "commit-like" explanation.
There was a problem hiding this comment.
Done: summary, remarks and returns split.
| /// <summary> | ||
| /// Compiles a trailing <c>buffer[i]</c> into an <c>OpImageTexelPointer</c> - a pointer to that | ||
| /// one texel - rather than the image read indexing normally produces. Returns false when the | ||
| /// chain is not an atomic-capable buffer index, leaving the caller to compile it normally. | ||
| /// <para> | ||
| /// A typed buffer is not memory the shader can point into: it is a storage image, so SDSL | ||
| /// compiles <c>buffer[i]</c> to an OpImageRead and <c>buffer[i] = x</c> to an OpImageWrite, | ||
| /// both of which deal in values. An atomic needs the memory itself, and OpImageTexelPointer is | ||
| /// the only instruction that hands it over. Its result may only be consumed by atomics, which | ||
| /// is why this is offered to the `ref` argument path instead of being how every index compiles. | ||
| /// </para> | ||
| /// </summary> |
There was a problem hiding this comment.
Again, too dense of a summary.
Better structure as a quick description in <summary>, a more thorough explanation in <remarks>, and what it returns in <returns>, instead of shoving all in a giant summary.
There was a problem hiding this comment.
Done: short summary, the explanation in remarks, and a returns.
| /// A composition that happens to inherit the same base as the shader it is composed into | ||
| /// inherits that base's entry point too, and lands in the same method group. Picking the | ||
| /// group's last member then picks the composition's copy: Stride.Voxels' Voxel2x2x2Mipmap | ||
| /// composes a Voxel2x2x2Mipmapper and both derive from ComputeShaderBase, so CSMain | ||
| /// resolved to the composition's, which calls the empty base Compute(). The real body - | ||
| /// and, once dead code was removed, the mipmap textures with it - disappeared, and voxel | ||
| /// GI silently contributed nothing. |
There was a problem hiding this comment.
Better explain what it does and why it is needed, but without pointing specifically at Voxel2x2x2Mipmap (that is an implementation detail). That part is better inside as a comment for mantainers explaining why this was added, not as the thing that shows when hovering over the symbol.
There was a problem hiding this comment.
Done: the summary describes the mechanism; the Voxel2x2x2Mipmap case is now an inline note for maintainers.
| /// <summary> | ||
| /// Drops geometry stream output parameters from every method that still has one, and the | ||
| /// matching argument from every call to them. | ||
| /// <para> | ||
| /// A <c>TriangleStream<Output></c> parameter carries no data - appending goes through | ||
| /// OpEmitVertexSDSL and the stage's output variables - but it cannot be dropped earlier: | ||
| /// EntryPointWrapperGenerator reads the output topology off it to emit the OutputPoints / | ||
| /// OutputLineStrip / OutputTriangleStrip execution mode. So it survives until here, where | ||
| /// the entry point has already been stripped of it and everything else still has to be. | ||
| /// </para> | ||
| /// <para> | ||
| /// The function type is rewritten through GetOrRegister rather than in place: a method and | ||
| /// the entry point calling it share one OpTypeFunction when their signatures match, and | ||
| /// mutating it for one silently rewrites the other - which is how the entry point's own | ||
| /// removal left such a method with more parameters than its type declared. | ||
| /// </para> | ||
| /// </summary> |
There was a problem hiding this comment.
Same as the last ones. Giant <summary>, too much too specific detail in there.
There was a problem hiding this comment.
Done: summary and remarks split, the shared OpTypeFunction detail moved inside as a comment.
|
|
||
| // Validate SPIR-V | ||
| var validationResult = Spv.ValidateFile($"{outputName}.spv"); | ||
| var validationResult = Spv.ValidateFile($"{outputName}.spv", targetVulkan: true); |
There was a problem hiding this comment.
Even though the new shader backend uses SPIR-V as an intermediary format, it is a Stride-specific dialect used for cross-compilation to different backends.
Is this fixed targetVulkan: true desirable?
There was a problem hiding this comment.
Agreed, it is a dialect. The tests now validate against Vulkan's rules only when Vulkan is the backend under test, which is what EffectCompiler does (targetVulkan: effectParameters.Platform is GraphicsPlatform.Vulkan).
|
Thanks for the review. Docs reworked throughout as you describe: short summaries for users, the specifics as inline comments for maintainers. Point taken on the dialect: the "Vulkan requires" phrasing is gone where the constraint is SPIR-V's, and the rendering tests validate against Vulkan's rules only for the Vulkan backend. On namespaces I've added tests that pin down the current behaviour rather than claim what 4.3 did. Happy to wait for xen on the compiler side. |
The SPIR-V compiler compiles ShaderClass, ignores ShaderEffect/EffectParameters and errors on everything else. An .sdsl file that opens with `using Foo.Bar;` therefore failed to compile at all: Error: Compiling declaration [Stride.Shaders.Parsing.UsingShaderNamespace] is not implemented Shader classes are resolved by name across every loaded namespace, so the declaration carries nothing the compiler needs - ignoring it is the correct handling, not a stopgap. Stride.Voxels' LightVoxelShader.sdsl is one such file, which took the whole game down the moment a voxel GI light was shaded. Necessary but not sufficient for voxel GI: past this point Stride.Voxels hits NotImplementedException in AccessorChainExpression.CompileHelper, the "array indexer for shader compositions" case (`compose IVoxelSampler Samplers[]` then `Samplers[i].Method()`), which the voxel marching and storage shaders use throughout. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
`compose IVoxelSampler Samplers[]` followed by `Samplers[0].Sample(...)` threw NotImplementedException from AccessorChainExpression.CompileHelper. The mixer half of this was already written: ProcessMemberAccessAndForeach (ShaderMixer.cs) watches for an OpAccessChain whose base id is a composition array, reads the constant index, maps the result id to compositions[index] and NOPs the chain out - and OpMemberAccessSDSL already consults that map before the single-composition one. Only the front-end never emitted the instruction. So this emits it, which is all the case has to do: push the constant index onto the pending access chain and type the accessor as a pointer to the composition's ShaderSymbol, so the following MethodCall resolves against the composition and emits the chain. The IntegerLiteral guard stays and now reports why a dynamic index cannot work: compositions are resolved at mix time, not at runtime. Stride.Voxels' marching, storage and layout shaders all index compositions this way, so voxel GI could not compile a single shader without it. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…NotFoundException Merging a mixin node looks every composition variable up in the node's supplied compositions with a bare dictionary indexer, so a missing one surfaced as The given key 'AttributeSamplers' was not present in the dictionary. from inside Dictionary`2.get_Item - no variable, no node, no shader, nothing to act on. Every one of those is already in scope at that point, so report them, along with the keys the node does have and the cause that produces this in practice: a `stage compose` whose declaring shader was promoted to this node while its value stayed at a nested composition path. Stride.Voxels' `stage compose IVoxelSampler AttributeSamplers[]` hits exactly that, and the old message gave no way to tell. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…es it `stage compose T Foo[]` worked only when its value was supplied by the root effect. Supplied by a nested effect, the merge threw: promoteToParent hoists the declaring shader to the root, but the value stayed in the nested instantiation's composition dictionary, so the root had a declaration it could not resolve. That asymmetry is the bug - the mixer already assumes stage compositions live at the root, resolving them through `this ?? Stage` in both ProcessMemberAccessAndForeach and ExpandForeach, and Stage is always the root. So this adds the twin of the existing promoteToParent callback for the value. Only a supplied value travels. Every shader inheriting the declaring one passes through ProcessCompositions too and defaults to an empty array; promoting those would overwrite the real value with whichever default was evaluated last. Two different supplied values for one stage slot now fail with an explicit message rather than silently picking one - a stage slot is shared by the whole stage, and code wanting one value per user should drop `stage`. Stride.Voxels is the case in the wild: MarchAttributes declares `stage compose IVoxelSampler AttributeSamplers[]` and LightVoxelEffect supplies it while being itself the root's environmentLights[0]. Test: CompositionArrayStageFromNested covers stage array + nested supply + constant index. CompositionArray1 covered stage arrays supplied at the root and CompositionArrayNested covered nested supply of a non-stage array; the combination had no coverage. Verified failing without this change and passing with it, on both D3D11 and Vulkan; full suite 599/599. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
SDSL has no implicit numeric conversion - Builder.Expressions only emits OpConvertFToS on the explicit cast path, and there is no SDSL#### diagnostic for an implicit one. So `perMapOffsetScale[mipBase]` with `float mipBase` compiled and then failed SPIR-V validation: VoxelStorageTextureClipmapShader.sdsl:66: Indexes passed to OpAccessChain must be of type integer. HLSL truncated it silently; the new compiler does not, and it should not - every shader that already runs through it uses integer indices. mipBase stays a float for the arithmetic and the SampleLevel calls around it, so the cast goes at the three indexing sites. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
`InterlockedMax(buffer[i], ...)` on an RWBuffer failed with 'ref' parameter at index 0 requires an l-value (pointer), but got uint A typed buffer is a storage image, not memory the shader can point into: indexing one compiles to an image read and assigning to one to an image write, both dealing in values. An atomic needs the memory itself, and OpImageTexelPointer is the only instruction that produces a pointer to a texel. The compiler never emitted it - it appeared nowhere in the tree - so atomics on a typed buffer could not be expressed at all. Two linked pieces: - The ref-argument path asks a trailing buffer index for a texel pointer before falling back to compiling it normally. It is offered there rather than made the way every index compiles, because the SPIR-V spec allows the result to be consumed by atomic instructions only. - OpImageTexelPointer is invalid on an image whose format is Unknown, which is what every RWBuffer declared. Image atomics are in turn only defined on 32-bit integer texels, so a concrete format (R32ui/R32i) is declared exactly there and Unknown is kept everywhere else - float buffers are unchanged. This is also the encoding DXC produces for the same HLSL, so the SPIR-V now matches what the rest of the ecosystem emits, and SPIRV-Cross reverses it back to `InterlockedMax(Output[0], ...)` for the D3D11 path - verified in the generated HLSL. Test: CSBufferAtomics covers InterlockedMax with an out parameter, a 2-argument InterlockedAdd, and a plain indexed write alongside them, on D3D11 and Vulkan. Full suite 601/601. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…he entry point A geometry shader that hands its output stream to another method produced SPIR-V that failed validation with "ID has not been defined". EntryPointWrapperGenerator strips the `inout TriangleStream<Output>` parameter from the entry point, because the stream object carries no data - appending goes through OpEmitVertexSDSL and the stage's output variables. It cannot be stripped any earlier: that parameter is where the output topology is read from, to emit the OutputPoints / OutputLineStrip / OutputTriangleStrip execution mode. But nothing stripped it anywhere else, so any other method keeping one was left referring to types and ids the stripping had removed. Three parts: - A pass after the entry point wrappers removes the parameter from every remaining method and the matching argument from every call to them. An argument cannot be dropped in place - the instruction is shorter - so those calls are rebuilt. - Each function's type is rewritten through GetOrRegister rather than in place. A method and the entry point calling it share one OpTypeFunction when their signatures match, and mutating it for one silently rewrote the other: that is how the entry point's own removal left such a method with more parameters than its type declared, which is the failure above. - Passing a stream as an argument no longer copies it into a function-local temporary. Streams join textures and samplers as types passed by pointer, and the copy-back in ProcessOutputArguments now consults the same predicate as the copy-in, so the two cannot disagree. Copying a stream was meaningless anyway, and the copy outlived the parameter it read from. Tests, both failing before and passing after on D3D11 and Vulkan: - StreamGSMethodCall: stream passed to a method sharing the entry point's signature. - StreamGSStreamOnlyParam: stream as the only parameter, so removal empties the signature and no shared type rewrites it on the method's behalf - the shape Stride.Voxels has in VoxelizationMethod.RestartStrip. Full suite 603/603. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…er refused a module Passing an RWBuffer<uint> to a method produced SPIR-V that failed validation with [VUID-StandaloneSpirv-OpTypeImage-06924] Cannot store to OpTypeImage ... objects and, before that, an OpFunctionCall whose argument storage class did not match the parameter's. Both rules were already implemented for textures and samplers, each spelled out as its own `is TextureType or SamplerType` list - one deciding that opaque parameters live in UniformConstant storage, the other that they are passed as the caller's pointer instead of copied into a Function temporary. Both lists had forgotten typed buffers, which are an OpTypeImage exactly like a texture. They are now one named predicate, SymbolTypeExtensions.IsOpaqueResource, so they cannot drift apart again. The failure surfaced as "spvOptimizerRun failed: InternalError" and nothing else: spirv-opt will not load a module that does not validate, and says only that. It now runs the validator on failure and reports what it found, with the source location - finding this one took dumping the module and bisecting the pass list by hand. Full suite 603/603. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…cution modes with their function Two Direct3D11 failures on the way to getting Stride.Voxels to render. FXC rejected the voxelization pixel shader with error X4509: UAV registers live in the same name space as outputs, so they must be bound to at least u1, manual bind to slot u0 failed D3D11 numbers UAVs and render targets in one space, so a pixel shader's UAVs start past its render targets. The engine already assumed that - CommandList binds with UAVStartSlot: currentRenderTargetViewsActiveCount and indexes slot - currentRenderTargetViewsActiveCount, which is negative if a UAV took u0 - only the compiler numbered them from zero. The UAV counter now starts at the fragment entry point's highest output location plus one, so multiple render targets work and a module without a pixel shader still gets u0. It is guarded on ResourcesRegisterSeparate, which is set for Direct3D11 alone: Vulkan and D3D12 share one counter across every resource class and have no such rule. One counter already fed both the SPIR-V Binding decoration and the reflection's SlotStart, so the shader's register and the engine's binding move together. Then a compute module failed to validate on a forward reference: an OpExecutionMode left pointing at a function the dead code remover had removed. An execution mode belongs to its entry point - [numthreads] here, the topology on a geometry shader - so it now goes when the function does, along with any OpEntryPoint naming it. The remover already collected exactly those ids to clean up names and decorations. Tests. Also fills the gaps left by earlier commits in this branch: - UsingNamespace: a shader file opening with a `using` declaration. - CSBufferAsParameter: a typed buffer handed to another method. - PixelShaderUav: a pixel shader writing a UAV and a render target. - CSExecModeOverride: an inherited CSMain overridden, killing the base's function. And the harness now validates against the Vulkan environment. It did not, so VUID-StandaloneSpirv-* never fired and CSBufferAsParameter passed even with its fix reverted - the very class of bug that cost the most to find here was invisible to the suite. Turning it on breaks nothing: 611/611. Every test above was checked failing with its fix reverted and passing with it. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
`Texture3D<half4>` emitted `OpTypeImage %half`, which fails validation: [VUID-StandaloneSpirv-OpTypeImage-04656] Expected Sampled Type to be a 32-bit int, 64-bit int or 32-bit float scalar type for Vulkan environment and left spirv-opt unable to load the module at all. There is no extension that lifts the rule, for images or for typed buffers, so a 16-bit element type in that position could never describe anything real. Nothing is lost by widening it. The 16 bits live in the resource's pixel format, not in the shader's declaration: the texture unit decodes the stored halfs and delivers 32-bit floats to the registers, for free. `half` is an alias of `float` in shader model 5 regardless, and code wanting native 16-bit registers converts after the read - which is the only place packed math can happen anyway. Done where the element type is resolved rather than at every read. The sinks are many - Load, Sample, SampleBias, SampleLevel, SampleGrad, four SampleCmp variants, Gather, and the write path - and missing one would resurface the same VUID elsewhere; the source is one function, which already carries the sibling rule about what element types a typed buffer may declare. Test: CSHalfTexture reads and writes a half texture. Verified failing with the change reverted, on both backends - it could not have been, before this branch made the harness validate against the Vulkan environment. Full suite 613/613. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Selecting the highest quality crashed inside D3D11: ID3D11DeviceContext::Dispatch: There can be at most 65535 Thread Groups in each dimension of a Dispatch call. ThreadGroupCountX (196608) ClearBuffer dispatched one group per 1024 elements along X alone, which tops out at 65535 * 1024, about 67 million elements. A 256^3 clipmap storing six directions is 201 million - three times over - so the largest volumes could not be cleared at all, whatever the caller did. The groups are now spread over X and Y and the shader recomposes the linear index from both, bounded by an element count because the last row overshoots. The one- dimensional case is unchanged: below the limit, groupsY is 1 and the index reduces to DispatchThreadId.x as before. Found by driving a voxel GI volume up to 256^3 anisotropic at runtime. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
ResolveEntryPoint takes the last member of the CSMain/PSMain method group, so that a shader's override beats the base it overrides. But a composition whose shader derives from the same base as the shader it is composed into contributes that base's entry point to the very same group - and, being added last, wins. Stride.Voxels hits this: Voxel2x2x2Mipmap composes a Voxel2x2x2Mipmapper and both derive from ComputeShaderBase, so CSMain resolved to the composition's copy, which calls the empty base Compute(). The mipmap shader compiled to a bare `ret` (1 instruction against 43 on 4.3), dead code removal then took ReadTex and WriteTex with it, and the six anisotropic axes all produced identical bytecode. Voxel GI contributed exactly nothing, and no error was reported anywhere. A composition's functions are only ever reachable through their composition variable, so none of them can be the entry point. They are now collected from the OpCompositionSDSL/OpCompositionEndSDSL brackets - with a depth counter, since compositions nest - and excluded from the candidates. Test: CompositionSharingABaseDoesNotOverrideTheRootsOverride, with CompositionWithoutASharedBaseKeepsTheRootsOverride as the control that has always passed, pinning the difference to the shared base. Verified failing without this change; full suite 615/615. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
A `stage compose` slot belongs to the whole stage, so promoteToParent hoists the shader that declares it to the root, and f5b8701 hoists its value along. But the value arrived stripped of where it came from: MergeMixinNode then merged it with currentCompositionPath == null and named everything underneath as if the root had supplied it. The engine composes its parameter keys with the path it actually supplied the value at, so the two no longer met. Stride.Voxels reads its clipmaps through MarchAttributes' `stage compose IVoxelSampler AttributeSamplers[]`, supplied by LightVoxelEffect from environmentLights[2]: reflection VoxelStorageTextureClipmapShader.clipMaps.storage.AttributeSamplers[0] runtime key VoxelStorageTextureClipmapShader.clipMaps.storage.AttributeSamplers[0].environmentLights[2] Nothing matched, so clipMaps, mipMaps and perMapOffsetScale were never bound: the lighting pass sampled a null texture on all 36 draws and voxel GI lit nothing. Compositions that do not go through the stage hoist were unaffected, even three levels deep (VoxelMarchConePerMipmap.coneRatioInv.Marcher.diffuseMarcher.environmentLights[2]). So the supply path now travels with the value, in StageCompositionPaths, and MergeMixinNode puts it back as the base path when there is no current one. Test: StageCompositionSuppliedFromNestedKeepsItsSupplyPath. CompositionArrayStageFromNested already covered the hoist resolving at all; this covers what it is named. Verified failing without this change; full suite 616/616. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…s not carry
`streams = input[i]` overwrites the stream members the geometry stage input
carries. StreamAccessPatcher built the whole struct though, defaulting every other
member to zero, so anything the shader had computed into a stream before its emit
loop was wiped once per vertex.
Stride.Voxels' dominant-axis voxelization is built on exactly that: it picks a
projection axis in InitializeFromTriangle, and the emit loop does
`streams = input[i]` before calling Append, which reads the axis back. The axis
reset to 0 on every vertex, the geometry shader constant-folded down to
if (true) { v1.xyz = v1.yzx; } // the X projection, always
else if (false) { ... } // dead
and 44 instructions on 4.3 became 26 with no integer op left. Every triangle was
projected along one axis, so only surfaces already facing it voxelized: floors and
ceilings disappeared from the voxel volume and everything else came out striped.
streams.clipIndex was zeroed the same way, writing every clipmap at index 0.
Those members are now read back from the current streams instead of defaulted.
Test: GeometryStreamsAssignKeepsMembersTheInputDoesNotCarry, asserting after
LegalizeForHlsl - what EffectCompiler hands to SPIRV-Cross - that the branch on the
carried value is still a branch and not a folded constant. Verified failing without
this change; full suite 617/617.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
… root parameters PrepareRootSignatureDescription allocated the immutable sampler buffer as `rootSignatureParameters.Count * sizeof(StaticSamplerDesc)`. It has to be `immutableSamplers.Count`: the two lists are unrelated, and as soon as an effect declares more immutable samplers than root parameters the copy runs off the end and throws "Destination is too short. (Parameter 'destination')". Found running a Stride.Voxels scene on Direct3D12, which crashed on the first frame in RootEffectRenderFeature.Prepare, but nothing about it is voxel-specific. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
GraphicsDeviceFeatures never queried them: every format was filled in with
MultisampleCount.None and the real query sat commented out next to it. So
Texture.New rejected any multisampled render target with
Cannot create a Texture with format R8G8B8A8_UNorm and multi-sample level X8.
The maximum supported level is None
which made MSAA unusable on Vulkan altogether. A Stride.Voxels scene dies on the
first frame, because dominant-axis voxelization allocates an 8x target, but any
multisampled render target hits it.
The count now comes from VkPhysicalDeviceLimits: the intersection of
framebufferColorSampleCounts and framebufferDepthSampleCounts, reported for every
format. Telling colour and depth formats apart here would need a predicate Stride
does not have on this side, and the two masks are identical on every driver worth
supporting; erring low only costs a sample count, where erring high would hand out
a count the device cannot attach.
Verified by running the Cornell box demo on Vulkan: it renders with GI and no
validation messages, where it previously threw before the first frame.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…eredAccessSlot, validation against Vulkan only for Vulkan The dispatch limit of 65535 groups per dimension is every API's, not Direct3D 11's, and the comments say so instead of suggesting a D3D11 branch; a using directive is explained for what it is; the half-in-images note says what FXC did; summaries are short, with remarks and returns of their own, and the implementation details moved into the methods. The rendering tests validate the SPIR-V against Vulkan's rules only when Vulkan is the backend, as the effect compiler does. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
A using of a base's namespace compiles and the base is found by name; a using of a namespace no shader lives in is not an error; two classes of one name in two namespaces of one file are not told apart by a using - the last declared is the one mixed in. Pinned down so that a compiler that honours namespaces changes them knowingly. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
…PIR-V, not Vulkan, where the rule is the dialect's Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
f999022 to
7b3499b
Compare
|
Rebased on master and comments trimmed as on the other PRs (1 to 2 lines, no history). No code change since the last review round. Note: this shares three files with #3389; whichever merges second will need a rebase. |
|
|
||
| for (int i = 0; i < mapFeaturesPerFormat.Length; i++) | ||
| mapFeaturesPerFormat[i] = new FeaturesPerFormat((PixelFormat) i, MultisampleCount.None, ComputeShaderFormatSupport.None, FormatSupport.None); | ||
| mapFeaturesPerFormat[i] = new FeaturesPerFormat((PixelFormat) i, maximumMultisampleCount, ComputeShaderFormatSupport.None, FormatSupport.None); |
There was a problem hiding this comment.
Doesn't this setup MSAA on every PixelFormat? (incl BC ones, etc.).
I think this won't work in vkCreateImage later.
Look into vkGetPhysicalDeviceImageFormatProperties (return sample count per format)
| // Function types are rewritten through GetOrRegister, not in place: functions with the same | ||
| // signature share one OpTypeFunction, and mutating it for one would rewrite the others. | ||
| // Which parameter index each function loses. | ||
| var removedParameters = new Dictionary<int, int>(); |
There was a problem hiding this comment.
Probably rare, but this won't work for a method which has 2 stream, i.e. void Emit(inout PointStream<Output> a, inout PointStream<Output> b)
|
|
||
| // --- Step 2b: Build the promote-composition-to-parent callback --- | ||
| // Same as above, for the value of a `stage compose` rather than the shader declaring it. | ||
| // The root's callback writes into its compositions; nested levels pass the parent's along. |
There was a problem hiding this comment.
I need to dig a bit more about that one, I will review that part later.
There was a problem hiding this comment.
I think we need to change a bit what is allowed in ShaderMixinSource to completely avoid this kind of issue.
I didn't understand the ReferenceEquals at first because it would almost always trigger due to how we generate those arrays, then I tried to review the use cases.
Also, I couldn't figure out if we needed to fail, or concat them, etc.
So, what about those new rules:
- a
stage composecan be declared anywhere (as today) - stage compose variables can only be declared on the root (where the compose end up being)
If we do that, we would need to do this:
Keep from the PR:
- eafee2e: the named error replacing the raw mixinSource.Compositions[variable.Key] indexer in MergeMixinNode. Valuable on its own.
Drop from the PR:
promoteCompositionToParentand its closure- StageCompositionPaths on ShaderMixinInstantiation
- the compositionPath / Nest() threading through EvaluateInheritanceAndCompositions → ProcessClasses → ProcessCompositions
- the basePath restore in MergeMixinNode
- the duplicate-value throw
Add in ProcessCompositions, check would be:
if (isSupplied && (variable.Flags & VariableFlagsMask.Stage) != 0 && !isRoot)
log.Error($"'{variableName}' is a `stage compose` declared by '{shaderName}', so it is one slot "
+ $"for the whole effect and must be supplied at the root. It was supplied at '{compositionPath}'.");
Then some existing effects need adjustments, like Voxel GI can drop the stage from AttributeSamplers and adjust C# code that set this collection. Need to check other usage (Light shafts, etc.)
Maybe the best approach here is to drop those specific controversial commits from PR (4df2f3d and 6a90900).
That way we can merge the current PR more easily, and then we can deal with that in another discussion/PR?
PR Details
Stride.Voxelsrenders correctly on 4.3 and crashes on 4.4: its shaders do not get through the new SDSL compiler. Past the crashes they compile and dispatch, but global illumination contributes exactly zero, with nothing logged. On Direct3D12 and Vulkan both backends throw on the first frame.Every fix here is a general engine issue that
Stride.Voxelshappens to uncover, not a workaround inside it; the two backend fixes affect any scene. Verified on Direct3D11, Direct3D12 and Vulkan.Reproduction: https://github.com/Nicogo1705/StrideVoxelGI, a Cornell box using
Stride.Voxels(dotnet run --project Demo). It targets4.4.0-beta5, so it reproduces every failure below as published; pointed at this branch it renders.Gtoggles the indirect light,Vcycles the voxel debug views.Commits
76c532eusingnamespace declarations instead of erroring80eff8deafee2eKeyNotFoundException4df2f3dstage composevalue up with the shader that declares ite51ceceperMapOffsetScalewith an int, not a floatd9717bcOpImageTexelPointerfd74e12e89f05bfc7e3825dd3223dd01d3a6c6c7bd6a90900stage composekeeps the path it was supplied at386baedstreams = input[i]keeps the members the stage input does not carryd3e578cbc51b7f2556d28usingdo in SDSL todayThree of these made GI render zero rather than crash, each by silently deleting code at mix time:
6c6c7bd: a composition sharing its base with the shader it is composed into won theCSMaingroup, so the mipmap shader compiled to a bareretwith no resources.6a90900: a hoistedstage composewas named as if the root had supplied it, soclipMaps/mipMapsnever matched their parameter keys and the lighting pass sampled a null texture.386baed:streams = input[i]zeroed everything the input did not carry, wiping the dominant axis each vertex; only surfaces already facing that axis voxelized.The two backend fixes are not specific to voxels:
d3e578c: the immutable sampler buffer was sized fromrootSignatureParameters.Countinstead ofimmutableSamplers.Count; an effect with more samplers than root parameters overran it (Destination is too short).bc51b7f:GraphicsDeviceFeaturesnever queried multisample support on Vulkan, so every format reportedMultisampleCount.NoneandTexture.Newrejected every multisampled render target. The count now comes fromVkPhysicalDeviceLimits.Related Issue
#3374
Types of changes
Checklist
Every shader fix has a regression test, verified failing with the fix reverted. The two backend fixes need a device and were verified by running the demo on the backend concerned. Game Studio was not run against these changes.
Known limitation left as is: on D3D12, mesa's
spirv_to_dxilreportsSampledBuffer/ImageBuffer(Buffer<T>/RWBuffer<T>) as unsupported; the scene renders, but that path is a backend limitation none of this touches.