stride3d / stride3d/stride

[Shaders] `stage compose` supplied from a nested composition: which rule for the mixer?

Open
#3,423 1 comment 0 reactions 0 assignees View on GitHub

Nobody has claimed this yet.

Dominant language
C#
Stars
7.8k
Forks
1.2k
Avg merge
2d 17h
Merged PRs (30d)
49

Description

Is your feature request related to a problem? Please describe.

The new mixer promotes any shader declaring stage members to the root, stage compose included. The value supplied for that composition does not follow: it stays in the Compositions dictionary of the node that supplied it. Supplied at the root it works (CompositionArray1). Supplied by a nested effect, the merge fails with No composition was supplied for 'AttributeSamplers' ... while merging the mixin node '<root>' (the named error added in #3382).

Case in the engine: Stride.Voxels. MarchAttributes declares stage compose IVoxelSampler AttributeSamplers[]. LightVoxelEffect supplies it, and is itself composed by the forward effect at environmentLights[i]. The marchers that sample through it inherit MarchAttributes two levels further down (Marcher.diffuseMarcher.environmentLights[i], specularMarcher.environmentLights[i]). A light renderer never controls the root effect, so "supply it at the root" is not something a light can do.

#3382 carried two commits that made this work (4df2f3d: hoist the value with the declaring shader; 6a90900: keep the supply path so resource keys get the names the engine composes). They were removed from the PR at @xen2's request so the rest can merge; this issue is the follow-up.

Describe the solution you'd like

Rules proposed by @xen2 in the #3382 review:

  • a stage compose can be declared anywhere (as today);
  • its value can only be supplied at the root. Supplying it from a nested node is an error, in ProcessCompositions:
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}'.");

Consequences:

  • Stride.Voxels drops stage on AttributeSamplers and supplies the sampler to every marcher composition that uses it (each gets its own resource bindings, so the C# applies the sampler parameters once per marcher path).
  • Other stage compose users to check: LightShaftsShader.lightGroup, DirectLightGroupArray.directLightGroups[], EnvironmentLightArray.environmentLights[]. Those are supplied by the root effect today.

Describe alternatives you've considered

Hoist the value with the declaring shader (the two dropped commits). It needs a rule for two different values supplied for one stage slot (error, or first wins), and the supply path has to travel with the value so the resource keys keep the names the engine composes (...AttributeSamplers[0].environmentLights[2], not ...AttributeSamplers[0]). It is more state in the mixer, and the open points in review were the ReferenceEquals check and whether to fail or concatenate.

Additional context

  • Review thread: #3382, ShaderMixer.ShaderSourceEvaluator.cs.
  • Tests that covered the hoist and were removed with the commits: CompositionArrayStageFromNested, StageCompositionSuppliedFromNestedKeepsItsSupplyPath (both in the PR history).
  • Question for the rule: is a shared composition below a light, which is always nested, meant to be impossible with stage compose? If so, what is the intended way for a light's nested compositions to share one sampler?

Contributor guide

Open the contributing guide

First steps

  1. Read the whole issue, then the project's contributing guide.
  2. Comment on the issue to say you are picking it up — it saves two people doing the same work.
  3. Fork the repository and make your change on a branch.
  4. Open a pull request that references the issue number.

Research direction

Start in ShaderMixer.ShaderSourceEvaluator.cs, specifically ProcessCompositions, and review the #3382 discussion and its removed commits. Check the Stride.Voxels usages and the other listed stage compose values, then inspect the removed CompositionArrayStageFromNested and StageCompositionSuppliedFromNestedKeepsItsSupplyPath tests. Done means the agreed nested-supply rule is implemented and affected compositions follow it without breaking resource paths.

Written by the indexing model from the issue text.

Assessment

Tech stack
csharp
Domain
computer-graphics, game-dev
Issue type
Feature
Difficulty
4/5
Estimated time
3-5 days
Activity status
Active
Clarity
Mostly clear
Newbie friendliness
45/100

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.