refactor: migrate all shaders from raw GLSL to ShaderLab format#347
refactor: migrate all shaders from raw GLSL to ShaderLab format#347zhuxudong wants to merge 5 commits intogalacean:dev/2.0from
Conversation
Convert all shader code to ShaderLab syntax: - Wrap in Shader/SubShader/Pass structure - Replace attribute/varying/uniform with struct Attributes/Varyings - Replace #include <xxx> macros with explicit vertex transform code - Replace old #include paths with engine ShaderLibrary paths - Change Shader.create(name, vs, fs) to Shader.create(shaderLabSource) - Consolidate separate .glsl files into .ts ShaderLab strings - Fix advanced-shader include paths for future extension use - Keep render states in TS material code (not in shader) Deleted: 10 .glsl files, 4 global.d.ts, bake-pbr vertex/fragment.ts BakePBRMaterial skipped (TODO: full PBR pipeline migration)
…igration - Restore global.d.ts for *.shader and *.wasm module declarations - Add ts-ignore for ShaderPass single-arg constructor (engine not yet published) - Update AssetType.Texture2D to AssetType.Texture to match engine API
…ions - PlanarShadowShaderFactory: fix lowercase "pbr" to "PBR" - PlainColorMaterial, FramebufferPicker, BoxSelectionSSMaterial: add explicit struct Varyings declaration
WalkthroughConsolidated numerous shaders by replacing separate vertex/fragment GLSL sources and external .glsl imports with inline ShaderLab-style shader strings, migrated to typed Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 9
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/gizmo/src/icon/Icon.ts (1)
60-71:⚠️ Potential issue | 🟠 MajorUnhandled rejection and stale-resolver race in async texture load.
Two concerns on the URL branch:
- The promise has no
.catch, so a failed load becomes an unhandled rejection with no recovery or diagnostic.- If
textureis reassigned (e.g., URL → different URL, or URL →Texture2D) before the previous load resolves, the stale resolver will still run and overwrite_material.baseTexturewith the obsolete texture. Guard the assignment againstthis._texture === valueat resolve time.🛡️ Proposed fix
} else { - this.engine.resourceManager.load({ url: value, type: AssetType.Texture }).then((texture: Texture2D) => { - this._material.baseTexture = texture; - }); + this.engine.resourceManager + .load<Texture2D>({ url: value, type: AssetType.Texture }) + .then((texture: Texture2D) => { + // Ignore stale result if `texture` was reassigned in the meantime. + if (this._texture !== value) return; + this._material.baseTexture = texture; + }) + .catch((err) => { + console.error(`Icon failed to load texture: ${value}`, err); + }); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/gizmo/src/icon/Icon.ts` around lines 60 - 71, The texture setter's async URL branch currently lacks error handling and can be conditionally overwritten by stale resolves; update the URL branch in the set texture(value: string | Texture2D) method to (1) capture the expected value (e.g., const expected = value) before calling engine.resourceManager.load({ url: value, type: AssetType.Texture }) and in the .then handler only assign this._material.baseTexture = texture if this._texture === expected, and (2) attach a .catch to the promise to log or handle load errors (use process/error logger or throw as appropriate) to avoid unhandled rejections; ensure the same guard is used if converting to a Texture2D synchronously when value instanceof Texture2D.
🧹 Nitpick comments (13)
packages/controls/src/box-selection/BoxSelectionSSMaterial.ts (1)
20-24: Consider initializingVaryingsbefore returning fromvert.Line 21 creates
Varyings vand returns it (Line 23) without assigningv._placeholder. While the field is not used in the fragment shader, explicitly initializing struct members before return is a defensive programming practice that can prevent portability issues on stricter GLSL backends.Suggested improvement
Varyings vert(Attributes attr) { Varyings v; + v._placeholder = 0.0; gl_Position = vec4(attr.POSITION, 1.0); return v; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/controls/src/box-selection/BoxSelectionSSMaterial.ts` around lines 20 - 24, The Varyings struct instance 'v' in the vertex function 'vert' is returned without initializing its member(s); initialize v._placeholder (or all Varyings fields) before returning from vert to ensure defined behavior across GLSL backends—locate the function vert and the local variable v in BoxSelectionSSMaterial.ts and set v._placeholder (or zero/appropriate default for each field) prior to the return statement.packages/geometry-sketch/src/material/TBNMaterial.ts (2)
14-17: Consolidate uniform declarations at the top of the Pass.
material_BaseColoris declared on Line 87 betweenvertandfrag, while the other uniforms (u_lineScale,camera_VPMat,u_worldMatrix,u_worldNormal) are grouped at Lines 14–17. Movingmaterial_BaseColorup with the rest keeps uniform declarations co-located and matches the pattern used elsewhere in the PR.♻️ Proposed grouping
float u_lineScale; mat4 camera_VPMat; mat4 u_worldMatrix; mat4 u_worldNormal; + vec4 material_BaseColor; ... - vec4 material_BaseColor; - void frag(Varyings v) { gl_FragColor = material_BaseColor; }Also applies to: 87-87
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/geometry-sketch/src/material/TBNMaterial.ts` around lines 14 - 17, Uniform declarations are split: move the material_BaseColor uniform declaration so it is co-located with the other uniforms (u_lineScale, camera_VPMat, u_worldMatrix, u_worldNormal) at the top of the Pass; locate the material_BaseColor declaration between vert and frag and cut/paste it up with those uniforms to keep all uniform declarations together and follow the project's pattern.
35-37: Nit:Varyingsreturned uninitialized.
Varyingsonly contains_placeholderandvert()returnsvwithout assigning it. The frag stage never reads it (it only uses thematerial_BaseColoruniform), so this is functionally safe, but you could drop the placeholder once your ShaderLab version allows an empty struct, or initialize it (Varyings v; v._placeholder = 0.0;) to silence potential driver warnings.Also applies to: 84-84
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/geometry-sketch/src/material/TBNMaterial.ts` around lines 35 - 37, The Varyings struct is left uninitialized because vert() returns a Varyings v without assigning its _placeholder field; either initialize the struct in vert() (e.g., set v._placeholder = 0.0 before return) or remove the placeholder field once empty structs are supported to avoid driver warnings; apply the same fix to the other Varyings occurrence referenced in this file so all vert() returns return initialized Varyings instances.packages/custom-material/src/planar-shadow/PlanarShadowShaderFactory.ts (2)
190-190: Prefer@ts-expect-errorover@ts-ignore.
@ts-expect-errorwill fail the build as soon as the engine publishes the matchingShaderPasssingle-arg overload, guaranteeing the suppression is cleaned up.@ts-ignoresilently lingers forever. Minor, but particularly useful since the comment explicitly says this is a temporary workaround.-// `@ts-ignore` - ShaderPass single-arg constructor supported in engine but not yet published +// `@ts-expect-error` - ShaderPass single-arg constructor supported in engine but not yet published🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/custom-material/src/planar-shadow/PlanarShadowShaderFactory.ts` at line 190, Replace the inline TypeScript suppression at the ShaderPass single-arg constructor site by changing the comment token from `@ts-ignore` to `@ts-expect-error` so the build will fail once the engine publishes the matching overload; update the adjacent explanatory comment (the one referencing "ShaderPass single-arg constructor supported in engine but not yet published") if needed to keep context while ensuring the suppression uses `@ts-expect-error` instead of `@ts-ignore` in PlanarShadowShaderFactory where ShaderPass is constructed.
21-22: Doc wording nit."Replace material Shader and initialization." / "Material to replace and initialization." reads awkwardly. Consider: "Replace the material's shader and initialize its render state." and "The material to configure."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/custom-material/src/planar-shadow/PlanarShadowShaderFactory.ts` around lines 21 - 22, Update the JSDoc wording to be clearer: change "Replace material Shader and initialization." to "Replace the material's shader and initialize its render state." and change "Material to replace and initialization." to "The material to configure."; locate these strings in the JSDoc for the PlanarShadowShaderFactory (or the function/method that replaces/configures the material) and update the comment blocks accordingly to match the suggested phrasing.packages/custom-material/src/bake-pbr/BakePBRMaterial.ts (1)
41-107: Duplicated PBR main-loop will drift from the engine's ForwardPassPBR.
BakePBRFragmentre-implements the whole surface/BSDF/shadow/direct-light/IBL/transmission/fog pipeline thatPBR/ForwardPassPBR.glslalready provides, and the#includeis effectively only being used to pull in helper functions. Any future engine-side change to the PBR pipeline (new BSDF inputs, additional light types, shadow API tweaks, output-format changes) won't propagate here and will silently diverge — matching the "partial/TODO" note in the PR description. Consider exposing a lightmap hook in the engine's forward pass (e.g. a weak-symbol / macro injection point likePBR_INDIRECT_DIFFUSE_OVERRIDE) so this material can reduce to just the vertex entry + a small override function instead of copy-pasting the main body.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/custom-material/src/bake-pbr/BakePBRMaterial.ts` around lines 41 - 107, BakePBRFragment duplicates the entire PBR pipeline from PBR/ForwardPassPBR.glsl (surface/BSDF/shadow/direct-light/IBL/transmission/fog) which will diverge from the engine; instead refactor to call into the engine forward pass and only inject the lightmap override. Replace the body of BakePBRFragment so it uses the included forward pass entry (keep Varyings BakePBRVertex / PBRVertex) and implement a small override hook (e.g., implement or call a macro like PBR_INDIRECT_DIFFUSE_OVERRIDE or a provided engine hook) to apply texture2D(u_lightMapTexture, lightMapUV) * u_lightMapIntensity * BRDF_Diffuse_Lambert(bsdfData.diffuseColor) to the indirect/diffuse term; remove or delegate usages of initBSDFData, evaluateDirectRadiance, evaluateIBL, sampleShadowMap, evaluateTransmission and fog so the engine implementation remains authoritative.packages/framebuffer-picker/src/FramebufferPicker.ts (1)
83-85: Nit:Varyingscarries an unused_placeholderfield.The fragment shader doesn't read any varying, and
vertreturnsvwithout assigning_placeholder, so the interpolated value is indeterminate (harmless here, just noise). If ShaderLab permits an empty struct you could drop the field; otherwise, a short comment explaining why the placeholder exists would help future readers.Optional tweak
struct Varyings { - float _placeholder; + float _placeholder; // ShaderLab requires a non-empty Varyings struct; unused by frag. };🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/framebuffer-picker/src/FramebufferPicker.ts` around lines 83 - 85, The Varyings struct contains an unused float _placeholder which produces indeterminate interpolants; either remove the field if ShaderLab allows an empty varying struct, or explicitly initialize it in the vertex function (the vert function that returns v) to a deterministic value (e.g., 0.0) and/or add a one-line comment on why the placeholder exists so readers understand it's intentional; update the declaration of Varyings and the vert function (or add the comment next to _placeholder) accordingly.packages/custom-material/src/grid/GridMaterial.ts (1)
47-47: Consider consolidating includes at the top of the Pass.
#include "Common/Transform.glsl"is placed between thevertentry function and the fragment-side uniforms/helpers. Functionally this is fine in ShaderLab (pass body is concatenated), but splitting includes makes it harder to see, at a glance, what symbols are available tovertvsfrag. Moving it up next to theCommon/Common.glslinclude would match the convention used by the other materials migrated in this PR.♻️ Proposed refactor
`#include` "Common/Common.glsl" + `#include` "Common/Transform.glsl" - mat4 camera_ViewInvMat; - mat4 camera_ProjMat; - struct Attributes { @@ return v; } - `#include` "Common/Transform.glsl" - float u_far;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/custom-material/src/grid/GridMaterial.ts` at line 47, The include "#include \"Common/Transform.glsl\"" is currently placed after the vert entry function which splits shared symbols between vertex and fragment code; move this include up next to the existing "#include \"Common/Common.glsl\"" in GridMaterial.ts so both the vert entry function and the fragment-side uniforms/helpers (frag) see the same transform definitions, preserving the original pass semantics while matching the include ordering convention used by other materials.packages/geometry-sketch/src/material/WireframeMaterial.ts (1)
14-33: Remove dead uniforms and redundant attribute members.Several declarations in this shader block are unused and add confusion:
u_lineScale(Line 14) andu_worldNormal(Line 17) are declared but never referenced anywhere invert/frag. They appear to be leftovers from the pre‑ShaderLab version.- Inside
struct Attributes, thePOSITION,NORMAL, andTANGENTmembers are dead from the vertex body's perspective: the body refers to barePOSITION/NORMAL/TANGENTon lines 52/55/57, which are locals injected by${geometryTextureVert}(texture‑sourced geometry), notattr.POSITIONetc. Keeping them inAttributesstill forces the engine to look up / bind those vertex‑buffer attributes on the draw, which is wasted work for this texture‑fed geometry.Only
JOINTS_0/WEIGHTS_0are actually consumed (viagetSkinMatrix(attr)), so the struct can shrink substantially.♻️ Suggested cleanup
- float u_lineScale; mat4 camera_VPMat; mat4 u_worldMatrix; - mat4 u_worldNormal; ${geometryTextureDefine} struct Attributes { - vec3 POSITION; `#ifdef` RENDERER_HAS_SKIN vec4 JOINTS_0; vec4 WEIGHTS_0; `#endif` - `#ifdef` RENDERER_HAS_NORMAL - vec3 NORMAL; - `#endif` - `#ifdef` RENDERER_HAS_TANGENT - vec4 TANGENT; - `#endif` };🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/geometry-sketch/src/material/WireframeMaterial.ts` around lines 14 - 33, Remove the unused uniforms and redundant attribute members: delete the declarations u_lineScale and u_worldNormal from the uniform block, and simplify the Attributes struct to only include JOINTS_0 and WEIGHTS_0 (since POSITION, NORMAL, and TANGENT are provided by the injected ${geometryTextureVert} locals and not via attr.*). Keep getSkinMatrix(attr) usage intact and ensure any code paths that expect attr.POSITION/NORMAL/TANGENT instead use the texture-sourced POSITION/NORMAL/TANGENT provided by geometryTextureVert so the engine no longer binds those unused vertex attributes.packages/lines/src/line/material/lineShader.ts (2)
18-36:v_directionvarying is write-only; can be removed.
v.v_directionis assigned invertbut never read in either stage, andfragdoesn't reference it. It's dead state passing through the pipeline.♻️ Proposed change
struct Varyings { vec2 v_origin; vec2 v_position; - float v_direction; float v_part; }; Varyings vert(Attributes attr) { Varyings v; - v.v_direction = attr.a_data.x; v.v_part = attr.a_data.y; - float layer_index = 1.0;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/lines/src/line/material/lineShader.ts` around lines 18 - 36, The Varyings struct contains an unused varying v_direction which is written in vert but never read; remove the dead varying by deleting its declaration in Varyings and removing the assignment v.v_direction = attr.a_data.x from the vert function (ensure no other code references v_direction or relies on attr.a_data.x in this shader). This keeps Varyings minimal and avoids passing unused state between stages.
44-58: UnusedfinalColorlocal.
vec4 finalColor;is declared but never written or read. Minor cleanup.♻️ Proposed change
void frag(Varyings v) { - vec4 finalColor; if (u_cap == 0 && v.v_part == IS_CAP) {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/lines/src/line/material/lineShader.ts` around lines 44 - 58, The fragment shader function frag declares an unused local variable vec4 finalColor; — remove that declaration from the frag(Varyings v) function (and any related dead code) so only needed variables like u_color, u_cap, u_join, v.v_part, v.v_position, v.v_origin, and u_width are used; ensure gl_FragColor = u_color; remains as the final output and run a quick build/lint to confirm no other references to finalColor exist.packages/lines/src/line/material/dashShader.ts (1)
20-48:v_directionvarying is never read in the fragment stage.
v.v_directionis written invert(and used locally there) but never consumed byfrag. It still costs an interpolator and vertex attribute copy. Consider dropping it fromVaryingsand treating it as a localfloatinsidevertinstead.♻️ Proposed change
struct Varyings { vec2 v_origin; vec2 v_position; - float v_direction; float v_part; vec2 v_tex; }; Varyings vert(Attributes attr) { Varyings v; - v.v_direction = attr.a_data.x; + float v_direction = attr.a_data.x; v.v_part = attr.a_data.y; float layer_index = 1.0; v.v_origin = attr.a_pos; float texcoord_y = 0.0; texcoord_y = attr.a_lengthsofar / (u_dash.x + u_dash.y); - if (v.v_direction == 1.0) { + if (v_direction == 1.0) { v.v_tex = vec2(1.0, texcoord_y); } else { v.v_tex = vec2(0.0, texcoord_y); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/lines/src/line/material/dashShader.ts` around lines 20 - 48, The Varyings struct currently includes v_direction which is written in vert but never read in frag; remove v_direction from Varyings and instead read attr.a_data.x into a local float (e.g., direction) inside vert, then use that local to set v.v_tex; update vert to stop assigning v.v_direction and delete the v.v_direction declaration in Varyings so you avoid wasting an interpolator and vertex attribute copy (leave v_part, v_origin, v_position, v_tex unchanged).packages/custom-material/src/plain-color/PlainColorMaterial.ts (1)
58-60: Consider removing the unused_placeholderfield from the emptyVaryingsstruct. EmptyVaryingsstructs are syntactically valid in Galacean ShaderLab, so the placeholder is unnecessary. The vertex shader doesn't populate any data, and the fragment shader doesn't use theVaryingsparameter, making this field a wasteful use of an interpolator slot.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/custom-material/src/plain-color/PlainColorMaterial.ts` around lines 58 - 60, The empty Varyings struct contains an unnecessary interpolator field `_placeholder`; remove `_placeholder` from the `struct Varyings` declaration and update the vertex and fragment shader function signatures that reference `Varyings` (in this file's shader code) to use an empty `Varyings` type instead, ensuring no code references `_placeholder` remain; this avoids consuming an interpolator slot while keeping the Galacean ShaderLab-compatible empty struct `Varyings`.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/custom-material/src/bake-pbr/BakePBRMaterial.ts`:
- Around line 99-106: The fragment currently writes gl_FragColor directly in
BakePBRFragment without applying the documented ACES tone mapping or using an
exposure uniform; add a uniform float (e.g. uExposure) and apply it to the
accumulated HDR color, then run the ACES tone mapping helper (use the existing
acesToneMapping/ACESFilm helper or add one if missing) and convert to sRGB
(linearToSRGB or equivalent) before assigning gl_FragColor; update
BakePBRFragment to multiply color.rgb by uExposure, pass the result through the
ACES tone mapping function, then set gl_FragColor = vec4(toneMappedColor,
color.a). If tone mapping is intentionally omitted instead, update the README to
remove the ACES/exposure claims.
In `@packages/custom-material/src/grid/GridMaterial.ts`:
- Around line 11-12: Remove the redundant uniform redeclarations for
camera_ViewInvMat and camera_ProjMat from GridMaterial shader source so the
shader relies on the shared includes (e.g., Common/Common.glsl and
Common/Transform.glsl) like the other migrated shaders; specifically delete the
two lines that declare mat4 camera_ViewInvMat; and mat4 camera_ProjMat; and
ensure references to camera_ViewMat and camera_ProjectionParams remain unchanged
so the common includes supply those uniforms consistently with outline, gizmo,
plain-color, and geometry-sketch shaders.
In `@packages/custom-material/src/plain-color/PlainColorMaterial.ts`:
- Around line 81-93: The fragment shader references material_AlphaCutoff inside
the frag function under the MATERIAL_IS_ALPHA_CUTOFF guard but never declares
it; add a declaration for material_AlphaCutoff (float) next to the existing vec4
material_BaseColor variable so the shader compiles when MATERIAL_IS_ALPHA_CUTOFF
is defined — update the declarations that contain material_BaseColor to also
declare material_AlphaCutoff so frag can use it safely.
In `@packages/custom-material/src/planar-shadow/PlanarShadowShaderFactory.ts`:
- Around line 190-194: The module currently calls Shader.find("PBR") and
constructs new ShaderPass(planarShadowPassSource) at import time which can NPE
if PBR isn't registered and produces orphan passes; fix by moving the
ShaderPass(planarShadowPassSource) construction and the
Shader.create("planarShadowShader", ...) call inside a guarded lazy initializer
function (e.g., registerPlanarShadowShader()) that first calls
Shader.find("PBR"), checks for null, and only then accesses
.subShaders[0].passes[2] and creates the shader; update replaceShader to call
registerPlanarShadowShader() before using the shader so registration is
deterministic and import-time side-effects are eliminated.
- Around line 108-114: In the Attributes struct inside PlanarShadowShaderFactory
(the struct named Attributes that currently declares vec4 POSITION), change the
POSITION field type from vec4 to vec3 so it matches the engine's glTF attribute
binding and other ShaderLab materials; update the declaration of POSITION in
Attributes to vec3 POSITION and leave the shader code that uses
vec4(attr.POSITION.xyz, 1.0) unchanged.
In `@packages/geometry-sketch/src/material/WireframeMaterial.ts`:
- Around line 15-17: Remove or guard the local uniform declaration camera_VPMat
in WireframeMaterial to avoid shadowing the engine-provided uniform from
Common/Transform.glsl; either delete the camera_VPMat declaration or wrap it in
a macro/ifdef that only defines it when the engine does not provide it, and keep
u_worldMatrix and u_worldNormal as-is (they are custom) while continuing to use
the engine's renderer_ModelMat where appropriate.
- Around line 41-45: The shader in WireframeMaterial.ts uses gl_VertexID (seen
in the snippet with gl_VertexID, getIndicesElement, u_indicesTextureWidth) which
requires GLSL ES 3.0/WebGL2; update the shader source to either prepend
"#version 300 es" at the top to enforce WebGL2 or replace gl_VertexID usage with
an explicit vertex attribute (e.g., pass a flat int/float index attribute from
the vertex buffer and use that instead of gl_VertexID) so it will compile on
WebGL1; also update any surrounding code that constructs the program to
document/require WebGL2 if you choose the version route.
In `@packages/gizmo/src/icon/Icon.ts`:
- Line 66: The load call using this.engine.resourceManager.load({ url: value,
type: AssetType.Texture }) is using a non-existent AssetType; change the asset
type to AssetType.Texture2D so the resource manager returns a Texture2D (update
the object literal to use type: AssetType.Texture2D), keeping the existing
then((texture: Texture2D) => ...) handler in the Icon class to ensure correct
typing and runtime resolution.
In `@packages/outline/src/OutlineManager.ts`:
- Around line 176-211: The shader is missing the Varyings struct used by
outlineReplaceShaderSource's vert and frag; add a Varyings declaration above the
vert function (in the outlineReplaceShaderSource string) that defines every
varying the vertex shader writes and the fragment shader reads (e.g., any
TEXCOORD, COLOR, or custom varyings used by frag), using the correct
interpolation qualifiers/precision as used elsewhere in our shaders so field
names/types match references in vert and frag.
---
Outside diff comments:
In `@packages/gizmo/src/icon/Icon.ts`:
- Around line 60-71: The texture setter's async URL branch currently lacks error
handling and can be conditionally overwritten by stale resolves; update the URL
branch in the set texture(value: string | Texture2D) method to (1) capture the
expected value (e.g., const expected = value) before calling
engine.resourceManager.load({ url: value, type: AssetType.Texture }) and in the
.then handler only assign this._material.baseTexture = texture if this._texture
=== expected, and (2) attach a .catch to the promise to log or handle load
errors (use process/error logger or throw as appropriate) to avoid unhandled
rejections; ensure the same guard is used if converting to a Texture2D
synchronously when value instanceof Texture2D.
---
Nitpick comments:
In `@packages/controls/src/box-selection/BoxSelectionSSMaterial.ts`:
- Around line 20-24: The Varyings struct instance 'v' in the vertex function
'vert' is returned without initializing its member(s); initialize v._placeholder
(or all Varyings fields) before returning from vert to ensure defined behavior
across GLSL backends—locate the function vert and the local variable v in
BoxSelectionSSMaterial.ts and set v._placeholder (or zero/appropriate default
for each field) prior to the return statement.
In `@packages/custom-material/src/bake-pbr/BakePBRMaterial.ts`:
- Around line 41-107: BakePBRFragment duplicates the entire PBR pipeline from
PBR/ForwardPassPBR.glsl (surface/BSDF/shadow/direct-light/IBL/transmission/fog)
which will diverge from the engine; instead refactor to call into the engine
forward pass and only inject the lightmap override. Replace the body of
BakePBRFragment so it uses the included forward pass entry (keep Varyings
BakePBRVertex / PBRVertex) and implement a small override hook (e.g., implement
or call a macro like PBR_INDIRECT_DIFFUSE_OVERRIDE or a provided engine hook) to
apply texture2D(u_lightMapTexture, lightMapUV) * u_lightMapIntensity *
BRDF_Diffuse_Lambert(bsdfData.diffuseColor) to the indirect/diffuse term; remove
or delegate usages of initBSDFData, evaluateDirectRadiance, evaluateIBL,
sampleShadowMap, evaluateTransmission and fog so the engine implementation
remains authoritative.
In `@packages/custom-material/src/grid/GridMaterial.ts`:
- Line 47: The include "#include \"Common/Transform.glsl\"" is currently placed
after the vert entry function which splits shared symbols between vertex and
fragment code; move this include up next to the existing "#include
\"Common/Common.glsl\"" in GridMaterial.ts so both the vert entry function and
the fragment-side uniforms/helpers (frag) see the same transform definitions,
preserving the original pass semantics while matching the include ordering
convention used by other materials.
In `@packages/custom-material/src/plain-color/PlainColorMaterial.ts`:
- Around line 58-60: The empty Varyings struct contains an unnecessary
interpolator field `_placeholder`; remove `_placeholder` from the `struct
Varyings` declaration and update the vertex and fragment shader function
signatures that reference `Varyings` (in this file's shader code) to use an
empty `Varyings` type instead, ensuring no code references `_placeholder`
remain; this avoids consuming an interpolator slot while keeping the Galacean
ShaderLab-compatible empty struct `Varyings`.
In `@packages/custom-material/src/planar-shadow/PlanarShadowShaderFactory.ts`:
- Line 190: Replace the inline TypeScript suppression at the ShaderPass
single-arg constructor site by changing the comment token from `@ts-ignore` to
`@ts-expect-error` so the build will fail once the engine publishes the matching
overload; update the adjacent explanatory comment (the one referencing
"ShaderPass single-arg constructor supported in engine but not yet published")
if needed to keep context while ensuring the suppression uses `@ts-expect-error`
instead of `@ts-ignore` in PlanarShadowShaderFactory where ShaderPass is
constructed.
- Around line 21-22: Update the JSDoc wording to be clearer: change "Replace
material Shader and initialization." to "Replace the material's shader and
initialize its render state." and change "Material to replace and
initialization." to "The material to configure."; locate these strings in the
JSDoc for the PlanarShadowShaderFactory (or the function/method that
replaces/configures the material) and update the comment blocks accordingly to
match the suggested phrasing.
In `@packages/framebuffer-picker/src/FramebufferPicker.ts`:
- Around line 83-85: The Varyings struct contains an unused float _placeholder
which produces indeterminate interpolants; either remove the field if ShaderLab
allows an empty varying struct, or explicitly initialize it in the vertex
function (the vert function that returns v) to a deterministic value (e.g., 0.0)
and/or add a one-line comment on why the placeholder exists so readers
understand it's intentional; update the declaration of Varyings and the vert
function (or add the comment next to _placeholder) accordingly.
In `@packages/geometry-sketch/src/material/TBNMaterial.ts`:
- Around line 14-17: Uniform declarations are split: move the material_BaseColor
uniform declaration so it is co-located with the other uniforms (u_lineScale,
camera_VPMat, u_worldMatrix, u_worldNormal) at the top of the Pass; locate the
material_BaseColor declaration between vert and frag and cut/paste it up with
those uniforms to keep all uniform declarations together and follow the
project's pattern.
- Around line 35-37: The Varyings struct is left uninitialized because vert()
returns a Varyings v without assigning its _placeholder field; either initialize
the struct in vert() (e.g., set v._placeholder = 0.0 before return) or remove
the placeholder field once empty structs are supported to avoid driver warnings;
apply the same fix to the other Varyings occurrence referenced in this file so
all vert() returns return initialized Varyings instances.
In `@packages/geometry-sketch/src/material/WireframeMaterial.ts`:
- Around line 14-33: Remove the unused uniforms and redundant attribute members:
delete the declarations u_lineScale and u_worldNormal from the uniform block,
and simplify the Attributes struct to only include JOINTS_0 and WEIGHTS_0 (since
POSITION, NORMAL, and TANGENT are provided by the injected
${geometryTextureVert} locals and not via attr.*). Keep getSkinMatrix(attr)
usage intact and ensure any code paths that expect attr.POSITION/NORMAL/TANGENT
instead use the texture-sourced POSITION/NORMAL/TANGENT provided by
geometryTextureVert so the engine no longer binds those unused vertex
attributes.
In `@packages/lines/src/line/material/dashShader.ts`:
- Around line 20-48: The Varyings struct currently includes v_direction which is
written in vert but never read in frag; remove v_direction from Varyings and
instead read attr.a_data.x into a local float (e.g., direction) inside vert,
then use that local to set v.v_tex; update vert to stop assigning v.v_direction
and delete the v.v_direction declaration in Varyings so you avoid wasting an
interpolator and vertex attribute copy (leave v_part, v_origin, v_position,
v_tex unchanged).
In `@packages/lines/src/line/material/lineShader.ts`:
- Around line 18-36: The Varyings struct contains an unused varying v_direction
which is written in vert but never read; remove the dead varying by deleting its
declaration in Varyings and removing the assignment v.v_direction =
attr.a_data.x from the vert function (ensure no other code references
v_direction or relies on attr.a_data.x in this shader). This keeps Varyings
minimal and avoids passing unused state between stages.
- Around line 44-58: The fragment shader function frag declares an unused local
variable vec4 finalColor; — remove that declaration from the frag(Varyings v)
function (and any related dead code) so only needed variables like u_color,
u_cap, u_join, v.v_part, v.v_position, v.v_origin, and u_width are used; ensure
gl_FragColor = u_color; remains as the final output and run a quick build/lint
to confirm no other references to finalColor exist.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: cf9cc120-dcac-4943-bb92-1c71d5dbe90e
⛔ Files ignored due to path filters (11)
packages/custom-material/src/advanced-shader/eye/EyeForwardPass.glslis excluded by!**/*.glslpackages/custom-material/src/advanced-shader/hair/HairForwardPass.glslis excluded by!**/*.glslpackages/custom-material/src/advanced-shader/hair/HairLightDirect.glslis excluded by!**/*.glslpackages/custom-material/src/advanced-shader/sss/SSSForwardPass.glslis excluded by!**/*.glslpackages/custom-material/src/advanced-shader/sss/SSSLightDirect.glslis excluded by!**/*.glslpackages/framebuffer-picker/src/color.fs.glslis excluded by!**/*.glslpackages/framebuffer-picker/src/color.vs.glslis excluded by!**/*.glslpackages/outline/src/outline.fs.glslis excluded by!**/*.glslpackages/outline/src/outline.vs.glslis excluded by!**/*.glslpackages/outline/src/replace.fs.glslis excluded by!**/*.glslpackages/outline/src/replace.vs.glslis excluded by!**/*.glsl
📒 Files selected for processing (22)
packages/controls/src/box-selection/BoxSelectionSSMaterial.tspackages/custom-material/src/bake-pbr/BakePBRMaterial.tspackages/custom-material/src/bake-pbr/fragment.tspackages/custom-material/src/bake-pbr/vertex.tspackages/custom-material/src/grid/GridMaterial.tspackages/custom-material/src/plain-color/PlainColorMaterial.tspackages/custom-material/src/planar-shadow/PlanarShadowShaderFactory.tspackages/custom-material/src/water/WaterMaterial.tspackages/custom-material/src/water/WaterRippleMaterial.tspackages/custom-material/src/water/WaterfallMaterial.tspackages/framebuffer-picker/src/FramebufferPicker.tspackages/framebuffer-picker/src/global.d.tspackages/geometry-sketch/src/material/GeometryShader.tspackages/geometry-sketch/src/material/TBNMaterial.tspackages/geometry-sketch/src/material/WireframeMaterial.tspackages/gizmo/src/icon/Icon.tspackages/gizmo/src/icon/IconMaterial.tspackages/lines/src/line/material/dashShader.tspackages/lines/src/line/material/lineShader.tspackages/outline/src/OutlineManager.tspackages/outline/src/global.d.tspackages/skeleton-viewer/src/SkeletonViewer.ts
💤 Files with no reviewable changes (4)
- packages/framebuffer-picker/src/global.d.ts
- packages/outline/src/global.d.ts
- packages/custom-material/src/bake-pbr/fragment.ts
- packages/custom-material/src/bake-pbr/vertex.ts
| vec4 color = vec4((totalDiffuseColor + totalSpecularColor).rgb, surfaceData.opacity); | ||
| color.rgb += surfaceData.emissiveColor; | ||
|
|
||
| #if SCENE_FOG_MODE != 0 | ||
| color = fog(color, varyings.positionVS); | ||
| #endif | ||
|
|
||
| gl_FragColor = color; |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Locate engine's ForwardPassPBR.glsl and inspect the final-write section to confirm
# which tone-mapping / color-space helpers the standard path applies.
fd -t f 'ForwardPassPBR.glsl'
fd -t f 'ForwardPassPBR.glsl' --exec sh -c 'echo "=== $1 ==="; tail -n 80 "$1"' _ {}
# Also look for tone-map / gamma helpers referenced by the standard pass.
rg -nP '\b(linearToGamma|gammaToLinear|HDRToLDR|toneMapping|ACESToneMap)\s*\(' -C2Repository: galacean/engine-toolkit
Length of output: 49
🏁 Script executed:
# First, let's understand the repository structure
git ls-files | head -20Repository: galacean/engine-toolkit
Length of output: 515
🏁 Script executed:
# Look for shader files in the custom-material package
fd -t f '\.(glsl|frag|vert)$' packages/custom-material/Repository: galacean/engine-toolkit
Length of output: 615
🏁 Script executed:
# Read the BakePBRMaterial.ts file to see the actual implementation
wc -l packages/custom-material/src/bake-pbr/BakePBRMaterial.tsRepository: galacean/engine-toolkit
Length of output: 127
🏁 Script executed:
# Search for tone mapping related functions across the codebase
rg -i 'tone.?map|linear.?gamma|hdr.?ldr' --type glsl --type tsRepository: galacean/engine-toolkit
Length of output: 95
🏁 Script executed:
cat -n packages/custom-material/src/bake-pbr/BakePBRMaterial.tsRepository: galacean/engine-toolkit
Length of output: 6349
🏁 Script executed:
# Search for tone mapping functions without file type restriction
rg -i 'tone.?map|linear.?gamma|hdr.?ldr' -A2 -B2Repository: galacean/engine-toolkit
Length of output: 1830
🏁 Script executed:
# Look for ForwardPassPBR references
rg -i 'forwardpass' -A2 -B2Repository: galacean/engine-toolkit
Length of output: 1593
🏁 Script executed:
# Search for exposure property definitions
rg -i 'exposure' -A2 -B2 | head -60Repository: galacean/engine-toolkit
Length of output: 1193
🏁 Script executed:
# Look at the base PBRMaterial to see what it inherits
rg -i 'class.*PBRMaterial' -A5Repository: galacean/engine-toolkit
Length of output: 668
🏁 Script executed:
# Search for where tone mapping might be applied in forward pass or elsewhere
rg -i 'aces|tone' -A2 -B2 | head -80Repository: galacean/engine-toolkit
Length of output: 6267
🏁 Script executed:
# Check if there are any other shader files that show how tone mapping is applied
fd -type f -name '*.glsl' -o -name '*.gs' | xargs rg -l 'gl_FragColor' | head -10Repository: galacean/engine-toolkit
Length of output: 779
BakePBRFragment missing ACES tone mapping promised in README.
The README documents that the material "supports...ACES toneMapping to correct the highlights" with an "Exposure" parameter "adjusted by ACES toneMapping automatically." However, the BakePBRFragment function (lines 99–106) writes gl_FragColor = color directly without reading any exposure value or applying tone mapping. The fragment shader accumulates totalDiffuseColor + totalSpecularColor + emissiveColor and applies fog, but skips the tone mapping step that would compensate for HDR lighting and match the visual output of PBRMaterial in the same scene.
Either add the exposure uniform and apply ACES tone mapping to the final color before gl_FragColor assignment, or remove/update the README documentation if tone mapping is not actually intended for this material.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/custom-material/src/bake-pbr/BakePBRMaterial.ts` around lines 99 -
106, The fragment currently writes gl_FragColor directly in BakePBRFragment
without applying the documented ACES tone mapping or using an exposure uniform;
add a uniform float (e.g. uExposure) and apply it to the accumulated HDR color,
then run the ACES tone mapping helper (use the existing acesToneMapping/ACESFilm
helper or add one if missing) and convert to sRGB (linearToSRGB or equivalent)
before assigning gl_FragColor; update BakePBRFragment to multiply color.rgb by
uExposure, pass the result through the ACES tone mapping function, then set
gl_FragColor = vec4(toneMappedColor, color.a). If tone mapping is intentionally
omitted instead, update the README to remove the ACES/exposure claims.
| mat4 camera_ViewInvMat; | ||
| mat4 camera_ProjMat; |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Look for engine-side declarations of the camera uniforms used here.
rg -nP '\bcamera_(ViewInvMat|ProjMat|ViewMat|ProjectionParams)\b' -C2
# Check sibling migrated shaders in this PR to see whether they redeclare these globals.
fd -e ts -e glsl | xargs rg -nP 'mat4\s+camera_(ViewInvMat|ProjMat|ViewMat)\s*;' -C1Repository: galacean/engine-toolkit
Length of output: 5463
🏁 Script executed:
fd -name "Common.glsl" -o -name "Transform.glsl" | head -20Repository: galacean/engine-toolkit
Length of output: 297
🏁 Script executed:
find . -name "*.glsl" -path "*/Common/*" | head -20Repository: galacean/engine-toolkit
Length of output: 49
🏁 Script executed:
# Look at other migrated .ts files in the same package to see if they also redeclare these uniforms
fd -path "*/custom-material/src/*" -name "*.ts" -type f | xargs rg -l "camera_ViewInvMat|camera_ProjMat|camera_ViewMat" | head -10Repository: galacean/engine-toolkit
Length of output: 348
🏁 Script executed:
# Find Common.glsl and Transform.glsl files
find . -name "Common.glsl" -o -name "Transform.glsl" 2>/dev/null | head -10Repository: galacean/engine-toolkit
Length of output: 49
🏁 Script executed:
# List all .ts files in custom-material to find migrated shaders
find packages/custom-material/src -name "*.ts" | head -20Repository: galacean/engine-toolkit
Length of output: 787
🏁 Script executed:
# Check what camera uniforms are used/declared in other migrated .ts shaders
find packages/custom-material/src -name "*.ts" -exec rg -l "camera_" {} \;Repository: galacean/engine-toolkit
Length of output: 188
🏁 Script executed:
# Check PlanarShadowShaderFactory.ts to see how it handles camera uniforms
cat -n packages/custom-material/src/planar-shadow/PlanarShadowShaderFactory.ts | grep -A5 -B5 "camera_"Repository: galacean/engine-toolkit
Length of output: 824
🏁 Script executed:
# Search for Common.glsl and Transform.glsl in the entire repository
find . -type f \( -name "Common.glsl" -o -name "Transform.glsl" \) 2>/dev/nullRepository: galacean/engine-toolkit
Length of output: 49
🏁 Script executed:
# Search for where camera uniforms are defined in the engine
rg "camera_ViewInvMat|camera_ProjMat" --type glsl --type ts -l 2>/dev/null | grep -v "GridMaterial\|Grid.shader\|PlanarShadow"Repository: galacean/engine-toolkit
Length of output: 49
🏁 Script executed:
# Look at full GridMaterial.ts file to understand structure
cat -n packages/custom-material/src/grid/GridMaterial.ts | head -100Repository: galacean/engine-toolkit
Length of output: 4335
🏁 Script executed:
# Search in engine core for Common.glsl
find . -path "*/engine/*" -name "Common.glsl" 2>/dev/null | head -5Repository: galacean/engine-toolkit
Length of output: 49
🏁 Script executed:
# Check if there's a shader loader or path resolution in the codebase
rg "Common/Common.glsl|Common/Transform.glsl" -A2 -B2Repository: galacean/engine-toolkit
Length of output: 6785
Remove redundant camera uniform redeclarations — inconsistent with all other migrated shaders.
Lines 11–12 explicitly declare camera_ViewInvMat and camera_ProjMat, but other camera uniforms like camera_ViewMat (used at lines 77, 83) and camera_ProjectionParams (used at line 37) are not declared locally—they come from the includes. All other migrated shaders in the codebase (e.g., outline, gizmo, plain-color, geometry-sketch) include Common/Common.glsl and Common/Transform.glsl without redeclaring these uniforms. Lines 11–12 should be removed to maintain consistency.
🛠️ Proposed fix
`#include` "Common/Common.glsl"
- mat4 camera_ViewInvMat;
- mat4 camera_ProjMat;
-
struct Attributes {
vec4 POSITION_FLIP;
};📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| mat4 camera_ViewInvMat; | |
| mat4 camera_ProjMat; | |
| `#include` "Common/Common.glsl" | |
| struct Attributes { | |
| vec4 POSITION_FLIP; | |
| }; |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/custom-material/src/grid/GridMaterial.ts` around lines 11 - 12,
Remove the redundant uniform redeclarations for camera_ViewInvMat and
camera_ProjMat from GridMaterial shader source so the shader relies on the
shared includes (e.g., Common/Common.glsl and Common/Transform.glsl) like the
other migrated shaders; specifically delete the two lines that declare mat4
camera_ViewInvMat; and mat4 camera_ProjMat; and ensure references to
camera_ViewMat and camera_ProjectionParams remain unchanged so the common
includes supply those uniforms consistently with outline, gizmo, plain-color,
and geometry-sketch shaders.
| vec4 material_BaseColor; | ||
|
|
||
| void frag(Varyings v) { | ||
| vec4 baseColor = material_BaseColor; | ||
|
|
||
| #ifdef MATERIAL_IS_ALPHA_CUTOFF | ||
| if( baseColor.a < material_AlphaCutoff ) { | ||
| discard; | ||
| } | ||
| #endif | ||
|
|
||
| gl_FragColor = baseColor; | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify that material_AlphaCutoff is declared in any include that PlainColorMaterial pulls in.
rg -nP '\bmaterial_AlphaCutoff\b' --type=ts --type=glsl -C2Repository: galacean/engine-toolkit
Length of output: 95
🌐 Web query:
galacean engine ShaderLibrary Common.glsl material_AlphaCutoff declaration
💡 Result:
No explicit declaration of 'material_AlphaCutoff' was found in publicly available sources for Galacean Engine's ShaderLibrary/Common.glsl file. Common.glsl is a built-in shader include file providing common macros (e.g., PI) and utility functions (e.g., pow2, gammaToLinear, sRGBToLinear) for use in custom shaders via #include "Common.glsl". It does not contain material-specific uniforms like alpha cutoff, which are typically declared in PBR-specific files such as FragmentPBR.glsl or material property blocks in ShaderLab passes (e.g., uniform float material_AlphaCutoff;). Alpha cutoff is a standard PBR material parameter used for alpha testing in cutout materials, controlling the threshold for discarding fragments (if alpha < cutoff, discard). Official docs reference PBR shaders like ForwardPassPBR for full organization, but source code details direct users to "see source code." The engine's GitHub repo (github.com/galacean/engine) contains shader-lab and shader packages where GLSL files reside, confirming 8.3% GLSL content.
Citations:
- 1: https://www.galacean.com/engine/en/docs/graphics/material/shaderAPI/
- 2: https://www.galacean.com/engine/en/docs/graphics/material/examples/shaderlab-05-advance/
- 3: https://github.com/galacean/engine
- 4: https://github.com/galacean/engine/tree/main/packages
🏁 Script executed:
#!/bin/bash
# Search for material_AlphaCutoff in the codebase (fix file type handling)
rg 'material_AlphaCutoff' -C2Repository: galacean/engine-toolkit
Length of output: 2127
🏁 Script executed:
#!/bin/bash
# Locate and examine PlainColorMaterial.ts
find . -name "PlainColorMaterial.ts" -type fRepository: galacean/engine-toolkit
Length of output: 131
🏁 Script executed:
#!/bin/bash
# Check the directory structure around the file
fd "PlainColorMaterial" -type fRepository: galacean/engine-toolkit
Length of output: 237
🏁 Script executed:
#!/bin/bash
# Read the PlainColorMaterial.ts file
cat -n packages/custom-material/src/plain-color/PlainColorMaterial.tsRepository: galacean/engine-toolkit
Length of output: 4765
Add missing material_AlphaCutoff uniform declaration.
The fragment shader at line 87 references material_AlphaCutoff under #ifdef MATERIAL_IS_ALPHA_CUTOFF, but this uniform is never declared in the shader. The includes (Common/Common.glsl, Transform.glsl, Skin.glsl, BlendShape.glsl) do not provide it. When the MATERIAL_IS_ALPHA_CUTOFF macro is enabled, the shader will fail to compile.
Add the declaration alongside material_BaseColor at line 81:
vec4 material_BaseColor;
float material_AlphaCutoff;
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/custom-material/src/plain-color/PlainColorMaterial.ts` around lines
81 - 93, The fragment shader references material_AlphaCutoff inside the frag
function under the MATERIAL_IS_ALPHA_CUTOFF guard but never declares it; add a
declaration for material_AlphaCutoff (float) next to the existing vec4
material_BaseColor variable so the shader compiles when MATERIAL_IS_ALPHA_CUTOFF
is defined — update the declarations that contain material_BaseColor to also
declare material_AlphaCutoff so frag can use it safely.
| struct Attributes { | ||
| vec4 POSITION; | ||
| #ifdef RENDERER_HAS_SKIN | ||
| attribute vec4 JOINTS_0; | ||
| attribute vec4 WEIGHTS_0; | ||
| vec4 JOINTS_0; | ||
| vec4 WEIGHTS_0; | ||
| #endif | ||
| }; |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check how POSITION is declared in other ShaderLab-migrated factories in this PR.
rg -nP --type=ts -C2 '\b(vec3|vec4)\s+POSITION\b' -g '!**/node_modules/**'Repository: galacean/engine-toolkit
Length of output: 5554
🏁 Script executed:
sed -n '145,165p' packages/custom-material/src/planar-shadow/PlanarShadowShaderFactory.tsRepository: galacean/engine-toolkit
Length of output: 982
Change POSITION from vec4 to vec3 to match the engine's attribute binding and all other materials in this PR.
Every other ShaderLab-migrated material declares POSITION as vec3 (WaterMaterial, PlainColorMaterial, OutlineManager, etc.), but this file declares it as vec4. While the code vec4(attr.POSITION.xyz, 1.0) works regardless of the declaration type, the struct field should match how the engine actually binds the glTF attribute (as vec3). Change line 109 from vec4 POSITION; to vec3 POSITION; for consistency.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/custom-material/src/planar-shadow/PlanarShadowShaderFactory.ts`
around lines 108 - 114, In the Attributes struct inside
PlanarShadowShaderFactory (the struct named Attributes that currently declares
vec4 POSITION), change the POSITION field type from vec4 to vec3 so it matches
the engine's glTF attribute binding and other ShaderLab materials; update the
declaration of POSITION in Attributes to vec3 POSITION and leave the shader code
that uses vec4(attr.POSITION.xyz, 1.0) unchanged.
| // @ts-ignore - ShaderPass single-arg constructor supported in engine but not yet published | ||
| const planarShadow = new ShaderPass(planarShadowPassSource); | ||
|
|
||
| // shadow color | ||
| color = u_planarShadowColor; | ||
| color.a *= falloff; | ||
| } | ||
| `, | ||
| ` | ||
| varying vec4 color; | ||
| void main() { | ||
| gl_FragColor = color; | ||
| } | ||
| ` | ||
| ); | ||
| Shader.create("planarShadowShader", [Shader.find("pbr").subShaders[0].passes[0], planarShadow]); | ||
| if (!Shader.find("planarShadowShader")) { | ||
| Shader.create("planarShadowShader", [Shader.find("PBR").subShaders[0].passes[2], planarShadow]); |
There was a problem hiding this comment.
Module-load ordering: unguarded Shader.find("PBR") can NPE.
Shader.find("PBR") is called at import time and its result is dereferenced as .subShaders[0].passes[2] without a null check. If this module is imported before the PBR shader is registered (e.g., depending on bundler/side-effect import order, or in a test environment), Shader.find("PBR") returns null and the whole module load crashes — and because the work sits at top level, there's no way for a caller to recover.
A few related concerns in the same block:
new ShaderPass(planarShadowPassSource)on Line 191 runs on every import even whenplanarShadowShaderwas already created by a previous load, producing an orphanShaderPass. Move it inside theif (!Shader.find(...))guard.- Consider wrapping the whole registration in a lazy init function invoked from
replaceShader(or a singleregisterPlanarShadowShader()export), so shader creation is deterministic and ordered with respect to PBR registration rather than implicit on module import.
🛠️ Suggested fix
-// `@ts-ignore` - ShaderPass single-arg constructor supported in engine but not yet published
-const planarShadow = new ShaderPass(planarShadowPassSource);
-
-if (!Shader.find("planarShadowShader")) {
- Shader.create("planarShadowShader", [Shader.find("PBR").subShaders[0].passes[2], planarShadow]);
-}
+if (!Shader.find("planarShadowShader")) {
+ const pbr = Shader.find("PBR");
+ if (!pbr) {
+ throw new Error(
+ `Cannot create "planarShadowShader": base "PBR" shader is not registered yet. ` +
+ `Ensure the engine's built-in shaders are initialized before importing PlanarShadowShaderFactory.`
+ );
+ }
+ // `@ts-expect-error` - ShaderPass single-arg constructor supported in engine but not yet published
+ const planarShadow = new ShaderPass(planarShadowPassSource);
+ Shader.create("planarShadowShader", [pbr.subShaders[0].passes[2], planarShadow]);
+}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/custom-material/src/planar-shadow/PlanarShadowShaderFactory.ts`
around lines 190 - 194, The module currently calls Shader.find("PBR") and
constructs new ShaderPass(planarShadowPassSource) at import time which can NPE
if PBR isn't registered and produces orphan passes; fix by moving the
ShaderPass(planarShadowPassSource) construction and the
Shader.create("planarShadowShader", ...) call inside a guarded lazy initializer
function (e.g., registerPlanarShadowShader()) that first calls
Shader.find("PBR"), checks for null, and only then accesses
.subShaders[0].passes[2] and creates the shader; update replaceShader to call
registerPlanarShadowShader() before using the shader so registration is
deterministic and import-time side-effects are eliminated.
| mat4 camera_VPMat; | ||
| mat4 u_worldMatrix; | ||
| mat4 u_worldNormal; |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check what Common/Transform.glsl and Common/Common.glsl expose for the VP / world matrices
fd -i -e glsl Transform
fd -i -e glsl Common
# And search the engine ShaderLab library for the canonical names
rg -nP '\b(camera_VPMat|u_worldMatrix|renderer_ModelMat|camera_ViewProjection)\b' -C2Repository: galacean/engine-toolkit
Length of output: 7026
🌐 Web query:
galacean engine ShaderLab built-in uniforms camera_VPMat u_worldMatrix Transform.glsl
💡 Result:
In Galacean Engine's ShaderLab system, built-in uniforms for transformation matrices and camera matrices are provided automatically. Key relevant uniforms include: Renderer uniforms (model transform): - renderer_LocalMat: mat4 - Model local coordinate matrix - renderer_ModelMat: mat4 - Model world coordinate matrix (equivalent to world matrix or u_worldMatrix) - renderer_MVMat: mat4 - Model view matrix - renderer_MVPMat: mat4 - Model view projection matrix (commonly used as gl_Position = renderer_MVPMat * attr.POSITION) - renderer_NormalMat: mat4 - Normal matrix Camera uniforms: - camera_ViewMat: mat4 - View matrix - camera_ProjMat: mat4 - Projection matrix - camera_VPMat: mat4 - View projection matrix These are declared directly in the Pass block or included via snippets like #include "Transform.glsl", which likely defines or exposes these matrices (used in examples for skinned animation and basic shaders). No explicit "u_worldMatrix" uniform is named, but renderer_ModelMat serves this role as the object-to-world transform. Transform.glsl is an engine-provided include file for transformation-related code, enabling modular use of these matrices.
Citations:
- 1: https://www.galacean.com/engine/en/docs/graphics/material/examples/shaderlab-01-basic-shader/
- 2: https://www.galacean.com/engine/en/docs/graphics/material/variables/
- 3: https://www.galacean.com/engine/en/docs/graphics/material/examples/shaderlab-05-advance/
- 4: https://galacean.antgroup.com/engine/docs/graphics/material/shader/
🏁 Script executed:
# Read the beginning of WireframeMaterial.ts to see the include and uniform declarations
head -25 packages/geometry-sketch/src/material/WireframeMaterial.ts
# Check SketchRenderer.ts to see how these uniforms are set
grep -A5 -B5 "_worldMatrixProp\|_worldNormalProp" packages/geometry-sketch/src/SketchRenderer.tsRepository: galacean/engine-toolkit
Length of output: 2061
🏁 Script executed:
# Check if there are any references or documentation about these custom uniforms
# Look for comments or commit messages explaining the choice
rg -i "camera_VPMat|u_worldMatrix" packages/geometry-sketch/src/ -B2 -A2 | head -50
# Also check if there's any specific reason these are custom vs using engine built-ins
grep -r "renderer_ModelMat" packages/geometry-sketch/src/ || echo "No renderer_ModelMat found"
grep -r "renderer_MVPMat" packages/geometry-sketch/src/ || echo "No renderer_MVPMat found"Repository: galacean/engine-toolkit
Length of output: 2725
Remove or guard camera_VPMat to avoid shadowing the engine's provided uniform.
camera_VPMat is already exposed by the included Common/Transform.glsl (line 11) as a built-in engine uniform. Redeclaring it locally shadows the engine's canonical definition and can lead to unexpected behavior or conflicts as the engine evolves.
u_worldMatrix and u_worldNormal appear to be intentionally custom uniforms (not standard engine uniforms, which uses renderer_ModelMat instead), so those are acceptable.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/geometry-sketch/src/material/WireframeMaterial.ts` around lines 15 -
17, Remove or guard the local uniform declaration camera_VPMat in
WireframeMaterial to avoid shadowing the engine-provided uniform from
Common/Transform.glsl; either delete the camera_VPMat declaration or wrap it in
a macro/ifdef that only defines it when the engine does not provide it, and keep
u_worldMatrix and u_worldNormal as-is (they are custom) while continuing to use
the engine's renderer_ModelMat where appropriate.
| int indicesIndex = gl_VertexID / 3; | ||
| int indicesRow = indicesIndex / int(u_indicesTextureWidth); | ||
| int indicesCol = indicesIndex % int(u_indicesTextureWidth); | ||
| vec3 triangleIndices = getIndicesElement(float(indicesRow), float(indicesCol)); | ||
| int subIndex = gl_VertexID % 3; |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
Galacean engine ShaderLab gl_VertexID WebGL1 support
💡 Result:
Galacean Engine's ShaderLab does not support gl_VertexID in WebGL1. Galacean Engine supports both WebGL1.0 and WebGL2.0 via WebGLEngine.create with graphicDeviceOptions.webGLMode to control the version. ShaderLab is a custom shader language that compiles to GLSL ES for WebGL, using explicit vertex attributes like POSITION, NORMAL, TEXCOORD_0 in Attributes structs passed to vertex shaders (e.g., Varyings vert(Attributes attr)). No official documentation mentions gl_VertexID or vertex index input; vertex processing relies on per-vertex attributes from meshes, not built-in vertex ID. gl_VertexID is a GLSL ES 3.0 (WebGL2) built-in not available in GLSL ES 1.0 (WebGL1), and since ShaderLab examples avoid it and use attribute-based input, it lacks support, especially in WebGL1 mode. For vertex ID needs, manually pass as custom attribute (TEXCOORD_n) or use WebGL2 if available.
Citations:
- 1: https://www.galacean.com/engine/en/docs/graphics/material/examples/shaderlab-01-basic-shader/
- 2: https://www.galacean.com/engine/en/docs/graphics/material/variables/
- 3: https://galacean.antgroup.com/engine/en/docs/core/engine/
- 4: https://github.com/galacean/engine
🏁 Script executed:
# Find WireframeMaterial and examine its full context
find . -name "WireframeMaterial.ts" -type fRepository: galacean/engine-toolkit
Length of output: 127
🏁 Script executed:
# Search for any WebGL version checks or documentation in the geometry-sketch package
fd -t f . packages/geometry-sketch/src/material/ | head -20Repository: galacean/engine-toolkit
Length of output: 281
🏁 Script executed:
# Look for gl_VertexID usage in the codebase and any version guards
rg "gl_VertexID" --type ts --type glsl --type js -B2 -A2Repository: galacean/engine-toolkit
Length of output: 95
🏁 Script executed:
# Check for any `#version` directives or WebGL mode enforcement in material shaders
rg "#version|webGLMode|WebGLMode|WebGL1|WebGL2" packages/geometry-sketch/src/material/ -A2Repository: galacean/engine-toolkit
Length of output: 49
🏁 Script executed:
# Read the WireframeMaterial.ts file
cat -n packages/geometry-sketch/src/material/WireframeMaterial.tsRepository: galacean/engine-toolkit
Length of output: 4057
🏁 Script executed:
# Search for gl_VertexID in the entire repo without file type restriction
rg "gl_VertexID" -B3 -A3Repository: galacean/engine-toolkit
Length of output: 4109
🏁 Script executed:
# Check if there are any WebGL version checks or guards around shader compilation
rg "version|WebGL|webGLMode" packages/geometry-sketch/src/material/ -B2 -A2Repository: galacean/engine-toolkit
Length of output: 49
🏁 Script executed:
# Search for Shader.create implementation and any version enforcement
rg "Shader\s*\.create|class Shader" --type ts | head -20Repository: galacean/engine-toolkit
Length of output: 2059
🏁 Script executed:
# Check if there's any WebGL version detection or fallback logic
rg "webGLMode|getWebGLVersion|WebGL.*version" --type ts | head -20Repository: galacean/engine-toolkit
Length of output: 49
🏁 Script executed:
# Look for any shader compilation guards or error handling related to gl_VertexID
rg "gl_VertexID|compileShader|version.*es" | head -20Repository: galacean/engine-toolkit
Length of output: 703
🏁 Script executed:
# Check if there's any shader validation or version handling in the geometry-sketch package
rg "version|Version|WebGL|webGL" packages/geometry-sketch/ -A2 -B2Repository: galacean/engine-toolkit
Length of output: 387
🏁 Script executed:
# Look at the GeometryShader.ts to see if it provides any version directives
cat -n packages/geometry-sketch/src/material/GeometryShader.ts | head -50Repository: galacean/engine-toolkit
Length of output: 2191
gl_VertexID is not supported in WebGL1 — add explicit WebGL2 version requirement or provide fallback.
The shader uses gl_VertexID (lines 41, 45), which requires GLSL ES 3.0 (WebGL2). Galacean Engine supports WebGL1 contexts, and this shader will fail to compile if instantiated in a WebGL1 pipeline without explicit version enforcement. Add #version 300 es at the shader entry point or document a hard requirement for WebGL2. Consider using a vertex attribute instead to maintain WebGL1 compatibility, per ShaderLab design patterns.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/geometry-sketch/src/material/WireframeMaterial.ts` around lines 41 -
45, The shader in WireframeMaterial.ts uses gl_VertexID (seen in the snippet
with gl_VertexID, getIndicesElement, u_indicesTextureWidth) which requires GLSL
ES 3.0/WebGL2; update the shader source to either prepend "#version 300 es" at
the top to enforce WebGL2 or replace gl_VertexID usage with an explicit vertex
attribute (e.g., pass a flat int/float index attribute from the vertex buffer
and use that instead of gl_VertexID) so it will compile on WebGL1; also update
any surrounding code that constructs the program to document/require WebGL2 if
you choose the version route.
| this._material.baseTexture = value; | ||
| } else { | ||
| this.engine.resourceManager.load({ url: value, type: AssetType.Texture2D }).then((texture: Texture2D) => { | ||
| this.engine.resourceManager.load({ url: value, type: AssetType.Texture }).then((texture: Texture2D) => { |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
galacean engine AssetType.Texture vs Texture2D resourceManager.load return type
💡 Result:
In Galacean Engine, use AssetType.Texture2D to load 2D textures via resourceManager.load, which returns a Texture2D object. There is no AssetType.Texture; official documentation consistently uses AssetType.Texture2D for image files (.png, .jpg, .webp) that create 2D textures. Texture refers to the general concept (including 2D, CubeTexture), while Texture2D is the specific class and asset type for 2D textures. Example from official docs: engine.resourceManager.load({ type: AssetType.Texture2D, url: 'image url' }).then((texture: Texture2D) => { ... }); AssetType values include Texture2D (for 2D images), with separate types like GLTF for models and Scene for scenes. Editor assets list "Texture" as UI label for 2D texture uploads, but script loading specifies AssetType.Texture2D.
Citations:
- 1: https://galacean.antgroup.com/engine/en/docs/graphics/texture/2d/
- 2: https://galacean.antgroup.com/engine/en/docs/assets/interface/
- 3: https://www.galacean.com/engine/en/docs/graphics/background/texture/
- 4: https://galacean.antgroup.com/engine/en/docs/assets/load/
- 5: https://github.com/oasis-engine/engine
🏁 Script executed:
cat packages/gizmo/src/icon/Icon.ts | head -80 | tail -20Repository: galacean/engine-toolkit
Length of output: 650
🏁 Script executed:
rg "enum AssetType|type AssetType" --type ts -A 10 -B 2Repository: galacean/engine-toolkit
Length of output: 49
🏁 Script executed:
rg "AssetType\." --type ts | head -20Repository: galacean/engine-toolkit
Length of output: 211
🏁 Script executed:
rg "AssetType" packages/gizmo/src/icon/Icon.ts -B 5 | head -30Repository: galacean/engine-toolkit
Length of output: 367
🏁 Script executed:
rg "export.*enum.*AssetType|export.*const.*AssetType" -type tsRepository: galacean/engine-toolkit
Length of output: 94
🏁 Script executed:
rg "Texture" packages/gizmo/src/icon/Icon.ts | head -10Repository: galacean/engine-toolkit
Length of output: 445
🏁 Script executed:
rg "AssetType\s*[=:{]" --type ts -A 30 | head -100Repository: galacean/engine-toolkit
Length of output: 49
🏁 Script executed:
fd "AssetType" --type tsRepository: galacean/engine-toolkit
Length of output: 236
🏁 Script executed:
find . -name "*.ts" -o -name "*.d.ts" | xargs grep -l "enum AssetType\|export.*AssetType" | head -5Repository: galacean/engine-toolkit
Length of output: 49
🏁 Script executed:
rg "AssetType\s*=" --type-listRepository: galacean/engine-toolkit
Length of output: 456
🏁 Script executed:
rg "AssetType\." . -A 2 -B 2 | grep -E "(Texture|AssetType)" | sort -u | head -20Repository: galacean/engine-toolkit
Length of output: 437
🏁 Script executed:
cat packages/gizmo/package.json | grep -A 10 -B 2 "galacean"Repository: galacean/engine-toolkit
Length of output: 956
🏁 Script executed:
rg "import.*AssetType" packages/gizmo/src/icon/Icon.ts -B 5 -A 5Repository: galacean/engine-toolkit
Length of output: 49
🏁 Script executed:
head -30 packages/gizmo/src/icon/Icon.tsRepository: galacean/engine-toolkit
Length of output: 760
Use AssetType.Texture2D instead of AssetType.Texture.
The code passes AssetType.Texture, but Galacean engine documentation specifies AssetType.Texture2D for loading 2D texture assets. There is no AssetType.Texture in the engine; the correct asset type for image files (.png, .jpg, .webp) is AssetType.Texture2D, which properly resolves to Texture2D objects via resourceManager.load().
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/gizmo/src/icon/Icon.ts` at line 66, The load call using
this.engine.resourceManager.load({ url: value, type: AssetType.Texture }) is
using a non-existent AssetType; change the asset type to AssetType.Texture2D so
the resource manager returns a Texture2D (update the object literal to use type:
AssetType.Texture2D), keeping the existing then((texture: Texture2D) => ...)
handler in the Icon class to ensure correct typing and runtime resolution.
| Varyings vert(Attributes attr) { | ||
| Varyings v; | ||
|
|
||
| vec4 position = vec4(attr.POSITION, 1.0); | ||
|
|
||
| #ifdef RENDERER_HAS_NORMAL | ||
| vec3 normal = vec3(attr.NORMAL); | ||
| #ifdef RENDERER_HAS_TANGENT | ||
| vec4 tangent = vec4(attr.TANGENT); | ||
| #endif | ||
| #endif | ||
|
|
||
| #ifdef RENDERER_HAS_BLENDSHAPE | ||
| calculateBlendShape(attr, position | ||
| #ifdef RENDERER_HAS_NORMAL | ||
| , normal | ||
| #ifdef RENDERER_HAS_TANGENT | ||
| , tangent | ||
| #endif | ||
| #endif | ||
| ); | ||
| #endif | ||
|
|
||
| #ifdef RENDERER_HAS_SKIN | ||
| mat4 skinMatrix = getSkinMatrix(attr); | ||
| position = skinMatrix * position; | ||
| #endif | ||
|
|
||
| gl_Position = renderer_MVPMat * position; | ||
|
|
||
| return v; | ||
| } | ||
|
|
||
| vec4 camera_OutlineReplaceColor; | ||
|
|
||
| void frag(Varyings v) { |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Verify that the outline replacement shader declares Varyings when it uses Varyings.
python - <<'PY'
from pathlib import Path
path = Path("packages/outline/src/OutlineManager.ts")
text = path.read_text()
start = text.index("const outlineReplaceShaderSource")
end = text.index("`;", start)
shader = text[start:end]
uses_varyings = "Varyings vert" in shader or "frag(Varyings" in shader
declares_varyings = "struct Varyings" in shader
print(f"uses Varyings: {uses_varyings}")
print(f"declares Varyings: {declares_varyings}")
if uses_varyings and not declares_varyings:
raise SystemExit("outlineReplaceShaderSource uses Varyings without declaring struct Varyings")
PYRepository: galacean/engine-toolkit
Length of output: 203
🏁 Script executed:
# Check the OutlineManager.ts file around lines 176-211 and search for Varyings declarations
python - <<'PY'
from pathlib import Path
path = Path("packages/outline/src/OutlineManager.ts")
content = path.read_text()
lines = content.split('\n')
# Find the outlineReplaceShaderSource definition
start_idx = None
for i, line in enumerate(lines):
if "const outlineReplaceShaderSource" in line:
start_idx = i
break
if start_idx is None:
print("ERROR: Could not find outlineReplaceShaderSource")
else:
# Find the end of the shader source (the closing backtick)
end_idx = None
for i in range(start_idx, len(lines)):
if "`;" in lines[i]:
end_idx = i
break
print(f"outlineReplaceShaderSource found at line {start_idx + 1}")
print(f"Shader source ends at line {end_idx + 1}")
print(f"\nTotal lines in shader: {end_idx - start_idx + 1}")
# Extract the shader source
shader_lines = lines[start_idx:end_idx+1]
# Check for struct declarations
print("\n=== Struct declarations in shader source ===")
for i, line in enumerate(shader_lines, start=start_idx+1):
if "struct " in line:
print(f"Line {i}: {line.strip()}")
# Check for Varyings usage
print("\n=== Varyings usage in shader source ===")
for i, line in enumerate(shader_lines, start=start_idx+1):
if "Varyings" in line:
print(f"Line {i}: {line.strip()}")
# Check what's around line 176
print("\n=== Lines 176-211 context ===")
for i in range(max(0, 175), min(len(lines), 211)):
print(f"Line {i+1}: {lines[i]}")
PYRepository: galacean/engine-toolkit
Length of output: 1657
🏁 Script executed:
# Search for Varyings struct definitions elsewhere in the codebase
rg "struct Varyings" -A 3
# Also check if there are other shader sources that might show the pattern
rg "Varyings vert\(" -B 5 -A 10Repository: galacean/engine-toolkit
Length of output: 25113
Add missing Varyings struct declaration in outlineReplaceShaderSource.
The outlineReplaceShaderSource shader uses Varyings in the vert and frag functions but never declares the struct. This will cause shader compilation to fail and break the outline replacement pass.
🐛 Proposed fix
};
+ struct Varyings {
+ float _placeholder;
+ };
+
Varyings vert(Attributes attr) {
Varyings v;
+ v._placeholder = 0.0;
vec4 position = vec4(attr.POSITION, 1.0);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/outline/src/OutlineManager.ts` around lines 176 - 211, The shader is
missing the Varyings struct used by outlineReplaceShaderSource's vert and frag;
add a Varyings declaration above the vert function (in the
outlineReplaceShaderSource string) that defines every varying the vertex shader
writes and the fragment shader reads (e.g., any TEXCOORD, COLOR, or custom
varyings used by frag), using the correct interpolation qualifiers/precision as
used elsewhere in our shaders so field names/types match references in vert and
frag.
- Replace inline struct Attributes with #include "Common/Attributes.glsl" - Add #ifdef guards for unconditional attribute usage - Replace PBR/AttributesPBR.glsl refs in ForwardPass shaders
- Use void vert/frag signature for shaders without varyings - Affected: TBNMaterial, WireframeMaterial, FramebufferPicker, PlainColorMaterial, BoxSelectionSSMaterial
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (1)
packages/custom-material/src/plain-color/PlainColorMaterial.ts (1)
31-43:⚠️ Potential issue | 🔴 CriticalDeclare
material_AlphaCutoffbefore using it.Line 37 references
material_AlphaCutoff, but this shader source only declaresmaterial_BaseColor. EnablingMATERIAL_IS_ALPHA_CUTOFFcan fail shader compilation.Proposed fix
vec4 material_BaseColor; + float material_AlphaCutoff; void frag() {To verify the declaration is still missing:
#!/bin/bash # Description: Confirm `material_AlphaCutoff` references/declarations in the repo. # Expected: PlainColorMaterial references it in frag; a declaration should exist in the shader source or included shader library. sed -n '25,43p' packages/custom-material/src/plain-color/PlainColorMaterial.ts rg -nP '\bmaterial_AlphaCutoff\b' -C2🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/custom-material/src/plain-color/PlainColorMaterial.ts` around lines 31 - 43, The shader uses material_AlphaCutoff inside the frag() function guarded by MATERIAL_IS_ALPHA_CUTOFF but never declares it, causing compile errors when that define is enabled; add a GLSL declaration for material_AlphaCutoff (e.g., a float uniform or varying depending on how materials are passed) alongside the existing material_BaseColor declaration in PlainColorMaterial so the symbol material_AlphaCutoff is defined when MATERIAL_IS_ALPHA_CUTOFF is used in frag(); update the declarations block that contains material_BaseColor in the PlainColorMaterial shader source to include this new material_AlphaCutoff declaration.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/custom-material/src/water/WaterMaterial.ts`:
- Around line 22-56: The vert(Attributes attr) function leaves v.v_color,
v.waterTexCoords and v.normalTexCoords uninitialized when
RENDERER_HAS_UV/RENDERER_ENABLE_VERTEXCOLOR are not defined, while frag() always
reads those varyings (v.v_color, v.waterTexCoords, v.normalTexCoords); to fix,
set safe default values at the start of Varyings vert(Attributes attr) (e.g.
v.v_color = vec4(1.0), v.waterTexCoords = vec2(0.0), v.normalTexCoords =
vec2(0.0)) before any conditional assignments so the fragment shader always
receives defined values.
In `@packages/custom-material/src/water/WaterRippleMaterial.ts`:
- Around line 22-51: The Varyings struct fields used in frag (v.normalTexCoords,
v.waterTexCoords, v.v_color) are only set inside conditional blocks in vert and
can be left uninitialized when RENDERER_HAS_UV or RENDERER_ENABLE_VERTEXCOLOR
are disabled; update vert(Attributes attr) to initialize v.normalTexCoords,
v.waterTexCoords, and v.v_color to safe defaults (e.g., zero vec2/vec4 or
neutral values) before the `#ifdef` blocks so frag() always reads defined values
and avoids undefined behavior.
In `@packages/skeleton-viewer/src/SkeletonViewer.ts`:
- Around line 29-49: The frag shader reads v.v_normal unconditionally while vert
only writes it when RENDERER_HAS_NORMAL is defined, so make the usage symmetric:
either initialize v.v_normal with a safe default in Varyings/vert (e.g., set
v.v_normal = vec3(0,1,0) before the `#ifdef` in the vert function) or wrap the
dot() and related code in frag with `#ifdef` RENDERER_HAS_NORMAL to provide a
fallback diffuse (use a constant color or compute ndl=1.0). Update the Varyings
struct/vert/frag symbols (Varyings, vert, frag, v_normal, RENDERER_HAS_NORMAL)
accordingly so v.v_normal is never read uninitialized.
---
Duplicate comments:
In `@packages/custom-material/src/plain-color/PlainColorMaterial.ts`:
- Around line 31-43: The shader uses material_AlphaCutoff inside the frag()
function guarded by MATERIAL_IS_ALPHA_CUTOFF but never declares it, causing
compile errors when that define is enabled; add a GLSL declaration for
material_AlphaCutoff (e.g., a float uniform or varying depending on how
materials are passed) alongside the existing material_BaseColor declaration in
PlainColorMaterial so the symbol material_AlphaCutoff is defined when
MATERIAL_IS_ALPHA_CUTOFF is used in frag(); update the declarations block that
contains material_BaseColor in the PlainColorMaterial shader source to include
this new material_AlphaCutoff declaration.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 78ef16c9-8467-485a-b6e1-3ee71a879b91
⛔ Files ignored due to path filters (3)
packages/custom-material/src/advanced-shader/eye/EyeForwardPass.glslis excluded by!**/*.glslpackages/custom-material/src/advanced-shader/hair/HairForwardPass.glslis excluded by!**/*.glslpackages/custom-material/src/advanced-shader/sss/SSSForwardPass.glslis excluded by!**/*.glsl
📒 Files selected for processing (11)
packages/controls/src/box-selection/BoxSelectionSSMaterial.tspackages/custom-material/src/plain-color/PlainColorMaterial.tspackages/custom-material/src/water/WaterMaterial.tspackages/custom-material/src/water/WaterRippleMaterial.tspackages/custom-material/src/water/WaterfallMaterial.tspackages/framebuffer-picker/src/FramebufferPicker.tspackages/geometry-sketch/src/material/TBNMaterial.tspackages/geometry-sketch/src/material/WireframeMaterial.tspackages/gizmo/src/icon/IconMaterial.tspackages/outline/src/OutlineManager.tspackages/skeleton-viewer/src/SkeletonViewer.ts
✅ Files skipped from review due to trivial changes (1)
- packages/controls/src/box-selection/BoxSelectionSSMaterial.ts
🚧 Files skipped from review as they are similar to previous changes (6)
- packages/outline/src/OutlineManager.ts
- packages/framebuffer-picker/src/FramebufferPicker.ts
- packages/custom-material/src/water/WaterfallMaterial.ts
- packages/geometry-sketch/src/material/WireframeMaterial.ts
- packages/gizmo/src/icon/IconMaterial.ts
- packages/geometry-sketch/src/material/TBNMaterial.ts
| Varyings vert(Attributes attr) { | ||
| Varyings v; | ||
| gl_Position = renderer_MVPMat * vec4(attr.POSITION, 1.0); | ||
|
|
||
| #ifdef RENDERER_HAS_UV | ||
| v.waterTexCoords = attr.TEXCOORD_0 + vec2(u_water_speed.x * sin(u_time), u_water_speed.y * cos(u_time)); | ||
| v.normalTexCoords = attr.TEXCOORD_0 + vec2(u_distorsion_speed.x * cos(u_time), u_distorsion_speed.y * sin(u_time)); | ||
| #endif | ||
|
|
||
| #ifdef RENDERER_ENABLE_VERTEXCOLOR | ||
| v.v_color = attr.COLOR_0; | ||
| #endif | ||
| return v; | ||
| } | ||
|
|
||
| #include "Common/Common.glsl" | ||
|
|
||
| sampler2D material_NormalTexture; | ||
| sampler2D u_waterTex; | ||
| sampler2D u_edgeTex; | ||
|
|
||
| vec4 u_edgeColor; | ||
| vec2 u_edgeParam; | ||
| float u_distorsion_amount; | ||
|
|
||
| void frag(Varyings v) { | ||
| vec4 normalTex = texture2D(material_NormalTexture, v.normalTexCoords) * 2.0 - 1.0; | ||
| vec4 waterTex = texture2D(u_waterTex, v.waterTexCoords + (normalTex.rg * u_distorsion_amount)); | ||
| vec4 edgeTex = texture2D(u_edgeTex, v.waterTexCoords + (normalTex.rg * u_distorsion_amount)); | ||
|
|
||
| float edge = pow((v.v_color.r + edgeTex.r) * v.v_color.r, 2.0); | ||
| edge = saturate(1.0 - smoothstep(u_edgeParam.x - u_edgeParam.y, u_edgeParam.x + u_edgeParam.y, edge)); | ||
| vec4 finalCol = mix(waterTex, u_edgeColor, edge); | ||
|
|
||
| gl_FragColor = finalCol; |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
find . -name "WaterMaterial.ts" -type fRepository: galacean/engine-toolkit
Length of output: 120
🏁 Script executed:
cat -n packages/custom-material/src/water/WaterMaterial.ts | head -100Repository: galacean/engine-toolkit
Length of output: 3987
🏁 Script executed:
# Check if there are other shader files using similar patterns to understand the convention
find . -name "*.ts" -path "*/material*" -type f | head -10Repository: galacean/engine-toolkit
Length of output: 489
🏁 Script executed:
cat -n packages/custom-material/src/water/WaterMaterial.ts | sed -n '47,60p'Repository: galacean/engine-toolkit
Length of output: 789
🏁 Script executed:
# Check other shader files to see if they use similar patterns or guard against undefined varyings
grep -r "Varyings\|struct.*{" packages/geometry-sketch/src/material/*.ts packages/lines/src/line/material/*.ts -A 5 | head -50Repository: galacean/engine-toolkit
Length of output: 4250
Initialize varyings to defaults before conditional assignment.
The vert() function conditionally initializes waterTexCoords, normalTexCoords (lines 27–28 under RENDERER_HAS_UV), and v_color (line 32 under RENDERER_ENABLE_VERTEXCOLOR), but frag() unconditionally reads all three varyings (lines 48–52). When these macros are absent, varyings propagate undefined values to the fragment shader, causing unpredictable rendering.
Initialize all varyings to defaults at the start of vert():
Varyings vert(Attributes attr) {
Varyings v;
v.v_color = vec4(1.0);
v.waterTexCoords = vec2(0.0);
v.normalTexCoords = vec2(0.0);
gl_Position = renderer_MVPMat * vec4(attr.POSITION, 1.0);
// ... rest of function
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/custom-material/src/water/WaterMaterial.ts` around lines 22 - 56,
The vert(Attributes attr) function leaves v.v_color, v.waterTexCoords and
v.normalTexCoords uninitialized when RENDERER_HAS_UV/RENDERER_ENABLE_VERTEXCOLOR
are not defined, while frag() always reads those varyings (v.v_color,
v.waterTexCoords, v.normalTexCoords); to fix, set safe default values at the
start of Varyings vert(Attributes attr) (e.g. v.v_color = vec4(1.0),
v.waterTexCoords = vec2(0.0), v.normalTexCoords = vec2(0.0)) before any
conditional assignments so the fragment shader always receives defined values.
| Varyings vert(Attributes attr) { | ||
| Varyings v; | ||
| gl_Position = renderer_MVPMat * vec4(attr.POSITION, 1.0); | ||
| #ifdef RENDERER_HAS_UV | ||
| v.waterTexCoords = attr.TEXCOORD_0 + vec2(u_foam_speed.x * u_time, u_foam_speed.y * u_time); | ||
| v.normalTexCoords = attr.TEXCOORD_0 + vec2(u_distorsion_speed.x * cos(u_time), u_distorsion_speed.y * sin(u_time)); | ||
| #endif | ||
| #ifdef RENDERER_ENABLE_VERTEXCOLOR | ||
| v.v_color = attr.COLOR_0; | ||
| #endif | ||
| return v; | ||
| } | ||
|
|
||
| #include "Common/Common.glsl" | ||
|
|
||
| sampler2D material_NormalTexture; | ||
| sampler2D u_foamTex; | ||
| vec3 u_foamColor; | ||
| vec2 u_foam_param; | ||
| float u_distorsion_amount; | ||
|
|
||
| void frag(Varyings v) { | ||
| vec4 normalTex = texture2D(material_NormalTexture, v.normalTexCoords) * 2.0 - 1.0; | ||
| vec4 waterTex = texture2D(u_foamTex, v.waterTexCoords + (normalTex.rg * u_distorsion_amount)); | ||
| float alphaComp = v.v_color.r * waterTex.r * u_foam_param.x; | ||
| float alpha = pow(alphaComp, 2.0); | ||
| alpha = smoothstep(0.5 - u_foam_param.y, 0.5 + u_foam_param.y, alpha); | ||
| alpha = saturate(alpha); | ||
|
|
||
| gl_FragColor = vec4(u_foamColor.rgb, alpha); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
find . -path "*/packages/custom-material/src/water/WaterRippleMaterial.ts" -type fRepository: galacean/engine-toolkit
Length of output: 126
🏁 Script executed:
cat -n ./packages/custom-material/src/water/WaterRippleMaterial.tsRepository: galacean/engine-toolkit
Length of output: 6177
🏁 Script executed:
rg "RENDERER_HAS_UV|RENDERER_ENABLE_VERTEXCOLOR" --type glsl -A 2 -B 2Repository: galacean/engine-toolkit
Length of output: 95
🏁 Script executed:
rg "RENDERER_HAS_UV|RENDERER_ENABLE_VERTEXCOLOR" -A 2 -B 2 --max-count=20Repository: galacean/engine-toolkit
Length of output: 9311
🏁 Script executed:
cat -n ./packages/custom-material/src/water/WaterMaterial.ts | head -60Repository: galacean/engine-toolkit
Length of output: 2375
🏁 Script executed:
cat -n ./packages/custom-material/src/water/WaterfallMaterial.ts | head -70Repository: galacean/engine-toolkit
Length of output: 2942
🏁 Script executed:
rg "struct Varyings" -A 10 packages/custom-material/src/water/ | head -80Repository: galacean/engine-toolkit
Length of output: 2818
Initialize varyings to prevent undefined behavior when conditionals are disabled.
v.normalTexCoords, v.waterTexCoords, and v.v_color are only assigned when RENDERER_HAS_UV or RENDERER_ENABLE_VERTEXCOLOR are enabled (lines 25–31), but are unconditionally read in the fragment shader (lines 44–46). When these macros are disabled, undefined values propagate to the fragment shader, causing unstable foam rendering that may vary across GPUs.
Initialize all varyings to safe defaults before the conditional blocks:
Suggested fix
Varyings vert(Attributes attr) {
Varyings v;
+ v.waterTexCoords = vec2(0.0);
+ v.normalTexCoords = vec2(0.0);
+ v.v_color = vec4(1.0);
gl_Position = renderer_MVPMat * vec4(attr.POSITION, 1.0);
`#ifdef` RENDERER_HAS_UV
v.waterTexCoords = attr.TEXCOORD_0 + vec2(u_foam_speed.x * u_time, u_foam_speed.y * u_time);
v.normalTexCoords = attr.TEXCOORD_0 + vec2(u_distorsion_speed.x * cos(u_time), u_distorsion_speed.y * sin(u_time));📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| Varyings vert(Attributes attr) { | |
| Varyings v; | |
| gl_Position = renderer_MVPMat * vec4(attr.POSITION, 1.0); | |
| #ifdef RENDERER_HAS_UV | |
| v.waterTexCoords = attr.TEXCOORD_0 + vec2(u_foam_speed.x * u_time, u_foam_speed.y * u_time); | |
| v.normalTexCoords = attr.TEXCOORD_0 + vec2(u_distorsion_speed.x * cos(u_time), u_distorsion_speed.y * sin(u_time)); | |
| #endif | |
| #ifdef RENDERER_ENABLE_VERTEXCOLOR | |
| v.v_color = attr.COLOR_0; | |
| #endif | |
| return v; | |
| } | |
| #include "Common/Common.glsl" | |
| sampler2D material_NormalTexture; | |
| sampler2D u_foamTex; | |
| vec3 u_foamColor; | |
| vec2 u_foam_param; | |
| float u_distorsion_amount; | |
| void frag(Varyings v) { | |
| vec4 normalTex = texture2D(material_NormalTexture, v.normalTexCoords) * 2.0 - 1.0; | |
| vec4 waterTex = texture2D(u_foamTex, v.waterTexCoords + (normalTex.rg * u_distorsion_amount)); | |
| float alphaComp = v.v_color.r * waterTex.r * u_foam_param.x; | |
| float alpha = pow(alphaComp, 2.0); | |
| alpha = smoothstep(0.5 - u_foam_param.y, 0.5 + u_foam_param.y, alpha); | |
| alpha = saturate(alpha); | |
| gl_FragColor = vec4(u_foamColor.rgb, alpha); | |
| Varyings vert(Attributes attr) { | |
| Varyings v; | |
| v.waterTexCoords = vec2(0.0); | |
| v.normalTexCoords = vec2(0.0); | |
| v.v_color = vec4(1.0); | |
| gl_Position = renderer_MVPMat * vec4(attr.POSITION, 1.0); | |
| `#ifdef` RENDERER_HAS_UV | |
| v.waterTexCoords = attr.TEXCOORD_0 + vec2(u_foam_speed.x * u_time, u_foam_speed.y * u_time); | |
| v.normalTexCoords = attr.TEXCOORD_0 + vec2(u_distorsion_speed.x * cos(u_time), u_distorsion_speed.y * sin(u_time)); | |
| `#endif` | |
| `#ifdef` RENDERER_ENABLE_VERTEXCOLOR | |
| v.v_color = attr.COLOR_0; | |
| `#endif` | |
| return v; | |
| } | |
| `#include` "Common/Common.glsl" | |
| sampler2D material_NormalTexture; | |
| sampler2D u_foamTex; | |
| vec3 u_foamColor; | |
| vec2 u_foam_param; | |
| float u_distorsion_amount; | |
| void frag(Varyings v) { | |
| vec4 normalTex = texture2D(material_NormalTexture, v.normalTexCoords) * 2.0 - 1.0; | |
| vec4 waterTex = texture2D(u_foamTex, v.waterTexCoords + (normalTex.rg * u_distorsion_amount)); | |
| float alphaComp = v.v_color.r * waterTex.r * u_foam_param.x; | |
| float alpha = pow(alphaComp, 2.0); | |
| alpha = smoothstep(0.5 - u_foam_param.y, 0.5 + u_foam_param.y, alpha); | |
| alpha = saturate(alpha); | |
| gl_FragColor = vec4(u_foamColor.rgb, alpha); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/custom-material/src/water/WaterRippleMaterial.ts` around lines 22 -
51, The Varyings struct fields used in frag (v.normalTexCoords,
v.waterTexCoords, v.v_color) are only set inside conditional blocks in vert and
can be left uninitialized when RENDERER_HAS_UV or RENDERER_ENABLE_VERTEXCOLOR
are disabled; update vert(Attributes attr) to initialize v.normalTexCoords,
v.waterTexCoords, and v.v_color to safe defaults (e.g., zero vec2/vec4 or
neutral values) before the `#ifdef` blocks so frag() always reads defined values
and avoids undefined behavior.
| struct Varyings { | ||
| vec3 v_normal; | ||
| }; | ||
|
|
||
| Varyings vert(Attributes attr) { | ||
| Varyings v; | ||
| gl_Position = renderer_MVPMat * vec4(attr.POSITION, 1.0); | ||
| #ifdef RENDERER_HAS_NORMAL | ||
| v.v_normal = normalize( mat3(renderer_NormalMat) * attr.NORMAL ); | ||
| #endif | ||
| return v; | ||
| } | ||
|
|
||
| vec3 u_colorMin; | ||
| vec3 u_colorMax; | ||
|
|
||
| void frag(Varyings v) { | ||
| float ndl = dot(v.v_normal, vec3(0, 1, 0)) * 0.5 + 0.5; | ||
| vec3 diffuse = mix(u_colorMin, u_colorMax, ndl); | ||
| gl_FragColor = vec4(diffuse, 1.0); | ||
| } |
There was a problem hiding this comment.
Unguarded read of v_normal when RENDERER_HAS_NORMAL is not defined.
v.v_normal is only written inside #ifdef RENDERER_HAS_NORMAL (Line 36–38), but frag unconditionally uses it in dot(v.v_normal, vec3(0, 1, 0)) (Line 46). When the renderer/mesh lacks a normal attribute, the varying is undefined, producing garbage shading (and on some drivers, NaNs).
Since _createSpur/PrimitiveMesh.createSphere both upload normals, the skeleton viewer's own meshes will set this macro; but the failure mode is silent if that ever changes. Either guard the frag read symmetrically or supply a safe default.
🛡️ Proposed fix
Varyings vert(Attributes attr) {
Varyings v;
gl_Position = renderer_MVPMat * vec4(attr.POSITION, 1.0);
`#ifdef` RENDERER_HAS_NORMAL
v.v_normal = normalize( mat3(renderer_NormalMat) * attr.NORMAL );
+ `#else`
+ v.v_normal = vec3(0.0, 1.0, 0.0);
`#endif`
return v;
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| struct Varyings { | |
| vec3 v_normal; | |
| }; | |
| Varyings vert(Attributes attr) { | |
| Varyings v; | |
| gl_Position = renderer_MVPMat * vec4(attr.POSITION, 1.0); | |
| #ifdef RENDERER_HAS_NORMAL | |
| v.v_normal = normalize( mat3(renderer_NormalMat) * attr.NORMAL ); | |
| #endif | |
| return v; | |
| } | |
| vec3 u_colorMin; | |
| vec3 u_colorMax; | |
| void frag(Varyings v) { | |
| float ndl = dot(v.v_normal, vec3(0, 1, 0)) * 0.5 + 0.5; | |
| vec3 diffuse = mix(u_colorMin, u_colorMax, ndl); | |
| gl_FragColor = vec4(diffuse, 1.0); | |
| } | |
| struct Varyings { | |
| vec3 v_normal; | |
| }; | |
| Varyings vert(Attributes attr) { | |
| Varyings v; | |
| gl_Position = renderer_MVPMat * vec4(attr.POSITION, 1.0); | |
| `#ifdef` RENDERER_HAS_NORMAL | |
| v.v_normal = normalize( mat3(renderer_NormalMat) * attr.NORMAL ); | |
| `#else` | |
| v.v_normal = vec3(0.0, 1.0, 0.0); | |
| `#endif` | |
| return v; | |
| } | |
| vec3 u_colorMin; | |
| vec3 u_colorMax; | |
| void frag(Varyings v) { | |
| float ndl = dot(v.v_normal, vec3(0, 1, 0)) * 0.5 + 0.5; | |
| vec3 diffuse = mix(u_colorMin, u_colorMax, ndl); | |
| gl_FragColor = vec4(diffuse, 1.0); | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/skeleton-viewer/src/SkeletonViewer.ts` around lines 29 - 49, The
frag shader reads v.v_normal unconditionally while vert only writes it when
RENDERER_HAS_NORMAL is defined, so make the usage symmetric: either initialize
v.v_normal with a safe default in Varyings/vert (e.g., set v.v_normal =
vec3(0,1,0) before the `#ifdef` in the vert function) or wrap the dot() and
related code in frag with `#ifdef` RENDERER_HAS_NORMAL to provide a fallback
diffuse (use a constant color or compute ndl=1.0). Update the Varyings
struct/vert/frag symbols (Varyings, vert, frag, v_normal, RENDERER_HAS_NORMAL)
accordingly so v.v_normal is never read uninitialized.
1 participant
Summary
Shader.create(name, vs, fs)) to ShaderLab format (Shader.create(shaderLabSource))attribute/varying/uniformwithstruct Attributes/Varyingsand bare declarations#include <xxx>angle-bracket macros with explicitcalculateBlendShape()/getSkinMatrix()calls"Common/Common.glsl","Skin/Skin.glsl", etc.)Shader.find("pbr")→Shader.find("PBR")and PBR pass indexpasses[0]→passes[2].glslfiles into.tsShaderLab strings.glsl,global.d.ts,vertex.ts,fragment.ts)#include "PBR/ForwardPassPBR.glsl"with lightmap extensionDependencies
Test plan