docs(sky): fix 9 issues found during design review of sky-rendering.md - #778
Open
JeanPhilippeKernel wants to merge 2 commits into
Open
docs(sky): fix 9 issues found during design review of sky-rendering.md#778JeanPhilippeKernel wants to merge 2 commits into
JeanPhilippeKernel wants to merge 2 commits into
Conversation
- Fix NodeMap API error: replace with SetPassEnabled() (Section 7) - Resolve SkyAtmosphereUBO set/binding conflict: canonicalise to set 0 binding 1 (Section 3.4 and 9) - Resolve IBL set number conflict: canonicalise to set 2 (Section 4.4 and 9) - Fix aerial perspective local_size: 8x8x1 -> 8x8x4 so dispatch 4x4x8 is correct (Section 3.4/3.5) - Fix SkySpherePush size: remove _pad field, total is now 80 bytes as stated (Section 6) - Add compute->fragment barrier note for SkyLightPass IBL writes (Section 4.4, 8) - Clarify LUT output bindings are in separate shaders, not co-resident (Section 3.4) - Add MieScattering CPU packing note: scalar expanded to Vec4f(x,x,x,0) (Section 3.3) - Fix pass name GeometryPass -> GbufferPass and note .exr not supported initially (Section 8, 5)
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Design review of
sky-rendering.mdfound 9 issues. All fixed in this PR.Critical (would fail to compile or produce wrong GPU output)
NodeMapAPI error in Section 7 — replaced withgraph->SetPassEnabled("SkyboxPass", false)(the actual RenderGraph API)SkyAtmosphereUBOset/binding conflict between Section 3.4 (set 0 binding 1) and Section 9 (set 1 binding 0) — canonicalised to set 0 binding 1 throughoutWrong computation
local_size 8x8x1+ dispatch4x4x8= 32x32x8 threads, not 32x32x32. Fixed by changinglocal_sizeto 8x8x4 so dispatch 4x4x8 gives the correct 32x32x32 totalStruct bug
SkySpherePushcomment said "80 bytes" but struct had 84 (extra_padfield). Removed_pad; struct is now exactly 80 bytesMissing synchronization spec
SkyLightPassIBL compute outputs are persistent externals that bypass the RenderGraph barrier system. Added explicit note:SkyLightPass::ExecuteCompute()must record aCOMPUTE_SHADER_WRITE → FRAGMENT_SHADER_READbarrier before returningClarifications
set 1 binding 0outputs are in separate shaders, not co-residentMieScatteringCPU packing:SkyConfig::MieScatteringis a scalar butSkyAtmosphereUBO::MieScatteringisVec4f— added note to expand asVec4f(x,x,x,0)GeometryPass→GbufferPass(matches actual pass name inGraphicRenderer).exrnot supported in initial implementation (stb_image handles.hdronly)