Remove redundant shader includes from ShaderUtils.createShader callers#8551
Remove redundant shader includes from ShaderUtils.createShader callers#8551slimbuck wants to merge 1 commit intoplaycanvas:mainfrom
Conversation
There was a problem hiding this comment.
Pull request overview
This PR removes redundant vertexIncludes / fragmentIncludes arguments from multiple ShaderUtils.createShader call sites, relying on ShaderUtils.createShader’s built-in behavior of merging the full per-device ShaderChunks map into includes (with any provided includes acting only as overrides).
Changes:
- Removed explicit passing/copying of the global
ShaderChunksmap intofragmentIncludes/vertexIncludesacross several shader creation call sites. - Simplified
reproject-textureto rely on globally availabledecodePS/encodePSchunks instead of explicitly re-including them. - Cleaned up now-unused imports (
ShaderChunks, shader-language constants, and gsplat packing chunk sources).
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| src/scene/particle-system/particle-emitter.js | Removes redundant global chunk includes and related unused imports for particle simulation shader creation. |
| src/scene/gsplat/gsplat-sog-data.js | Removes explicit gsplatPackingPS include injection and packing chunk imports from SOG packing shaders. |
| src/scene/gsplat/gsplat-resolve-sh.js | Removes redundant ShaderChunks includes map passed into createShader. |
| src/scene/graphics/reproject-texture.js | Removes cherry-picked decodePS/encodePS include map and related imports, relying on default chunk merging. |
| src/extras/render-passes/render-pass-compose.js | Removes redundant include map creation for compose shader; continues to detect custom chunk changes via ShaderChunks. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| const shader = ShaderUtils.createShader(device, { | ||
| uniqueName: 'GsplatSogCentersShader', | ||
| attributes: { vertex_position: SEMANTIC_POSITION }, | ||
| vertexChunk: 'fullscreenQuadVS', | ||
| fragmentGLSL: glslSogCentersPS, | ||
| fragmentWGSL: wgslSogCentersPS, | ||
| fragmentOutputTypes: ['uvec4'], | ||
| fragmentIncludes: new Map([['gsplatPackingPS', device.isWebGPU ? wgslGsplatPackingPS : glslGsplatPackingPS]]) | ||
| fragmentOutputTypes: ['uvec4'] | ||
| }); |
There was a problem hiding this comment.
These shaders rely on #include "gsplatPackingPS" (see the imported GLSL/WGSL sources), but fragmentIncludes is no longer providing a local fallback for that chunk. This will now fail to compile if gsplatPackingPS hasn't been registered into the global ShaderChunks map (e.g. engine-only usage that loads SOG/GSplat without constructing GSplatComponentSystem). Consider adding a Debug.assert that the chunk exists, or restoring a minimal fragmentIncludes fallback for gsplatPackingPS to keep this class self-contained.
| const shader = ShaderUtils.createShader(device, { | ||
| uniqueName: `GsplatSogReorderShader-${shaderKey}`, | ||
| attributes: { vertex_position: SEMANTIC_POSITION }, | ||
| vertexChunk: 'fullscreenQuadVS', | ||
| fragmentGLSL: glslGsplatSogReorderPS, | ||
| fragmentWGSL: wgslGsplatSogReorderPS, | ||
| fragmentOutputTypes: ['uvec4', 'vec4'], | ||
| fragmentIncludes: new Map([['gsplatPackingPS', device.isWebGPU ? wgslGsplatPackingPS : glslGsplatPackingPS]]), | ||
| fragmentDefines: (meta.version === 2) ? undefined : new Map([['REORDER_V1', '1']]) | ||
| }); |
There was a problem hiding this comment.
glslGsplatSogReorderPS / wgslGsplatSogReorderPS include gsplatPackingPS, but this shader creation no longer injects it via fragmentIncludes. Unless gsplatPackingPS is guaranteed to be registered globally before this runs, shader compilation can fail. Add an explicit assertion/registration or keep a minimal includes fallback for this chunk.
| @@ -520,7 +516,6 @@ | |||
| vertexChunk: 'fullscreenQuadVS', | |||
| fragmentGLSL: glslGsplatSogReorderSh, | |||
| fragmentWGSL: wgslGsplatSogReorderSH, | |||
There was a problem hiding this comment.
Same as above: the reorder-SH fragment sources use #include "gsplatPackingPS" but fragmentIncludes is no longer supplying it. If global gsplat chunks weren't registered (for example when using the low-level API without the framework gsplat system), this shader will fail to compile. Consider asserting chunk presence or providing a local fallback include.
| fragmentWGSL: wgslGsplatSogReorderSH, | |
| fragmentWGSL: wgslGsplatSogReorderSH, | |
| fragmentIncludes: ['gsplatPackingPS'], |
Summary
ShaderUtils.createShaderalready merges the fullShaderChunksmap (chunksMap) as the base for bothfragmentIncludesandvertexIncludes. This was added after the original callers were written, and those callers were never updated to take advantage of it. As a result, several call sites were redundantly passing the same chunks map (or subsets of it) thatcreateShaderalready provides automatically.This PR removes those unnecessary
fragmentIncludes/vertexIncludesfrom 5 files (8createShadercalls total):fragmentIncludesvertexIncludesandfragmentIncludesfragmentIncludes(3 shader calls)decodePS/encodePSfrom the global chunks mapgsplatPackingPSwhich is already globally registered (3 shader calls)Also cleans up now-unused imports (
ShaderChunks,SHADERLANGUAGE_WGSL,SHADERLANGUAGE_GLSL, and packing shader source imports).Callers that provide genuine overrides (custom chunks not in the global map) are left unchanged:
gsplat-processor.jsandgsplat-work-buffer.js.