Uh oh!
There was an error while loading. Please reload this page.
Refactor material parameter extraction and add fog support - #502
Merged
Conversation
- Add missing gradient_material block generator - Fix makeTargetCacheKey to match getOrCreateMaterial format (alpha toFixed(2) and glow key), so the cache-hit early-exit in applyMaterialToHierarchy now fires correctly - Extract getMaterialParamsFromMesh helper that reads colour from metadata.cacheKey (preserving gradient arrays) with fallback to diffuseColor/albedoColor for raw GLTF materials; use it in glowMesh, clearEffects and setAlpha, replacing duplicated logic that was losing gradient colours by falling back to white - Add fog uniforms and fog-aware registration to createMultiColorGradientMaterial so multicolour gradients respond to scene fog consistently with other shader materials - Add tests covering gradient preservation through glow, clearEffects, setAlpha and cache-hit behaviour https://claude.ai/code/session_01Jckn16VqMzdP7SYXerpW9y
Contributor
📝 WalkthroughWalkthroughCentralized material parameter extraction from meshes; made multi-color gradient shaders fog-aware; included glow and quantized alpha in material cache keys; added a Blockly generator for gradient materials; and added tests validating gradient-material preservation, alpha handling, and cache reuse. Changes
Sequence Diagram(s)sequenceDiagram
participant Mesh
participant FlockAPI as Flock API
participant MaterialCache as Material Cache
participant ShaderFactory as Shader/Material Factory
participant Scene
Mesh->>FlockAPI: request material params / apply material
FlockAPI->>FlockAPI: getMaterialParamsFromMesh(mesh)
FlockAPI->>MaterialCache: lookup key (mat_<color>_<alphaKey>_<tex>_<glowKey>)
alt cache hit
MaterialCache-->>FlockAPI: existing material
else cache miss
FlockAPI->>ShaderFactory: create/register fog-aware shader/material
ShaderFactory-->>MaterialCache: store material
MaterialCache-->>FlockAPI: new material
end
FlockAPI->>Scene: apply material to mesh (setMaterialWithCleanup)
Scene-->>Mesh: material applied
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested labels
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 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 |
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
tests/materials.test.js (1)
1079-1110: One of the changed gradient branches is still untested.
setAlphais only exercised against the 3+ colorShaderMaterialpath, and the cache-hit assertion uses a scalar descriptor. That leaves the 2-color/array-descriptor branch out of the new alpha/caching coverage. Switching the cache case to a 2-color descriptor, or adding a dedicated 2-colorsetAlphatest, would protect the other half of this refactor.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/materials.test.js` around lines 1079 - 1110, The test suite misses coverage for the 2-color/array-descriptor gradient code path; update tests so the caching/assertion or alpha path exercises that branch by either (A) changing the "should hit cache when applying the same material twice via applyMaterialToHierarchy" test to use a 2-color gradient descriptor (e.g., descriptor.color as an array like ["#ff0000","#00ff00"]) when calling flock.applyMaterialToHierarchy on the mesh, or (B) add a new spec that calls createGradientBox (or createBox + apply a 2-color descriptor) then calls flock.setAlpha(id, {value: ...}) and asserts the material class and alpha just like the 3+ color test; reference flock.setAlpha and flock.applyMaterialToHierarchy to locate where to change/add the test.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@api/material.js`:
- Around line 187-191: The code is overwriting a possible gradient color
descriptor by replacing params.color with a scalar glow tint; instead preserve
params.color (so createMaterial keeps gradients) and carry the glow tint
separately (e.g., add glowTint or glowColor property on materialParams) and use
that tint only for the glow call (see getMaterialParamsFromMesh, materialParams,
createMaterial and glow(mesh, { color })). Modify the assignment so color:
params.color (or leave out color) and add glowTint: glowColor ?
flock.getColorFromString(glowColor) : undefined, then update the code that calls
glow(mesh, { color }) to read the tint from glowTint instead of params.color.
- Around line 155-179: getMaterialParamsFromMesh is mis-parsing cacheKey and
dropping valid state: split("_") breaks when materialName contains underscores,
parseFloat(parts[2]) || 1 turns legitimate 0.00 alphas into 1, and the
non-cacheKey fallback always returns "none.png" causing textured materials to
lose their texture. Fix getMaterialParamsFromMesh by: parsing cacheKey more
robustly (e.g., split with a limit or extract materialName as the remainder
after the known prefix parts so underscores are preserved), treating alpha using
a strict numeric check rather than `||` so 0 or 0.00 stays 0 (use
Number.isFinite or explicit NaN check after Number()/parseFloat), and in the
fallback branch preserve any existing texture name from mat (e.g., read
mat.diffuseTexture/name or equivalent) instead of hardcoding "none.png"; update
references in callers that use getMaterialParamsFromMesh to rely on the
corrected fields.
---
Nitpick comments:
In `@tests/materials.test.js`:
- Around line 1079-1110: The test suite misses coverage for the
2-color/array-descriptor gradient code path; update tests so the
caching/assertion or alpha path exercises that branch by either (A) changing the
"should hit cache when applying the same material twice via
applyMaterialToHierarchy" test to use a 2-color gradient descriptor (e.g.,
descriptor.color as an array like ["#ff0000","#00ff00"]) when calling
flock.applyMaterialToHierarchy on the mesh, or (B) add a new spec that calls
createGradientBox (or createBox + apply a 2-color descriptor) then calls
flock.setAlpha(id, {value: ...}) and asserts the material class and alpha just
like the 3+ color test; reference flock.setAlpha and
flock.applyMaterialToHierarchy to locate where to change/add the test.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 775df81d-9a7b-4a02-9715-4aa4527f7ac5
📒 Files selected for processing (3)
api/material.jsgenerators/generators-material.jstests/materials.test.js
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
- Parse materialName by taking all parts between index 3 and the glow suffix, joined with "_", so texture names containing underscores are preserved - Use Number.isFinite check instead of || 1 so a cached alpha of 0.00 is not coerced to 1 - Detect glow/noglow suffix by checking the last part, with fallback to mesh.metadata.glow for older unmanaged materials - In fallback branch, read texture name from mat.diffuseTexture/ albedoTexture instead of hardcoding "none.png" so unmanaged textured GLTF materials preserve their texture on glow/setAlpha/clearEffects - In glowMesh, only replace params.color with the explicit glowColor when params.color is a scalar; gradient arrays are kept intact so createMaterial continues to produce a GradientMaterial/ShaderMaterial Addresses CodeRabbit review comments on PR #502. https://claude.ai/code/session_01Jckn16VqMzdP7SYXerpW9y
Deploying flockdev with |
| Latest commit: | c610685 |
| Status: | ✅ Deploy successful! |
| Preview URL: | https://aa762a52.flockdev.pages.dev |
| Branch Preview URL: | https://claude-review-material-handl.flockdev.pages.dev |
GradientMaterial and ShaderMaterial have no emissiveColor property so Babylon's GlowLayer never produced a visible effect on gradient meshes. Install customEmissiveColorSelector on the GlowLayer that reads metadata.glowColor per-mesh, bypassing the material entirely. Store the resolved glow colour on metadata.glowColor in glowMesh (using the first array colour for gradients, or the explicit glowColor arg for scalar materials). Clear metadata.glowColor in clearEffects. This makes glow work consistently for all material types: Standard, GradientMaterial (2-colour), and ShaderMaterial (3+ colour). Update tests to assert glowColor metadata is set after glow and cleared after clearEffects. https://claude.ai/code/session_01Jckn16VqMzdP7SYXerpW9y
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
api/material.js (1)
219-225:⚠️ Potential issue | 🟠 MajorDon't feed the glow tint back into the base material.
The gradient case is fixed, but Lines 223-225 still overwrite scalar
params.coloreven though the GlowLayer now readsmetadata.glowColor. That makesglow(mesh, { color })change the rendered material as well as the glow, and it also creates a separate cache entry for what should be a per-mesh glow-only override.Suggested change
const materialParams = { ...params, - color:- glowColor && !Array.isArray(baseColor)- ? flock.getColorFromString(glowColor)- : baseColor,+ color: baseColor, glow: true, };🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@api/material.js` around lines 219 - 225, The materialParams object should not inject glowColor into the base material; remove any use of glowColor when building materialParams so the base material's color remains params.color/baseColor and glow tint stays only in metadata.glowColor. Update the code that constructs materialParams (the materialParams spread in api/material.js) to stop setting color from glowColor — either remove the color override entirely or only set color from baseColor when baseColor is a scalar — ensuring glowColor is only used for metadata and does not produce a separate material cache entry.
🧹 Nitpick comments (1)
api/material.js (1)
1109-1110: Gradient fog uniforms are refreshed after fog changes viasetFog().The main concern is addressed:
api/effects.jsline 224 callsflock.updateFogAwareShaderMaterials?.()immediately after all fog properties are set insetFog(), ensuring registered materials get updated. However,createMultiGradientShaderMaterial()lacks the defensiveonBindObservablerefresh pattern used bycreateColorReplaceShaderMaterial(). Consider adding the same pattern for consistency—this guards against potential directscene.fog*mutations that bypasssetFog()and provides redundant safety on every material bind.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@api/material.js` around lines 1109 - 1110, createMultiGradientShaderMaterial lacks the defensive onBindObservable refresh used by createColorReplaceShaderMaterial; add an onBindObservable observer on the material created in createMultiGradientShaderMaterial that calls flock.updateFogUniformsForShaderMaterial(shaderMaterial) (the same pattern used for createColorReplaceShaderMaterial) so fog uniforms are refreshed on every bind in case scene.fog* was mutated outside setFog(); keep the existing flock.registerFogAwareShaderMaterial call but add the onBindObservable hook to ensure redundant safety.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@api/material.js`:
- Around line 173-188: The code currently reconstructs materialName by parsing
mat.metadata.cacheKey (using parts.slice(...)) which uses a
canonicalized/lowercased key and can mangle original texture case; instead, read
the original texture name from a lossless metadata field (e.g.
mat.metadata.originalTextureName or mat.metadata.materialName) or, if present,
the live material texture property (e.g. mat.textureName / mat.name) and use
that for materialName; only fall back to parsing cacheKey if no original name
exists, and preserve casing (do not .toLowerCase) — update the return to use
that metadata/original property for materialName and keep existing alpha/glow
logic (mat.alpha, mesh.metadata?.glow) unchanged.
---
Duplicate comments:
In `@api/material.js`:
- Around line 219-225: The materialParams object should not inject glowColor
into the base material; remove any use of glowColor when building materialParams
so the base material's color remains params.color/baseColor and glow tint stays
only in metadata.glowColor. Update the code that constructs materialParams (the
materialParams spread in api/material.js) to stop setting color from glowColor —
either remove the color override entirely or only set color from baseColor when
baseColor is a scalar — ensuring glowColor is only used for metadata and does
not produce a separate material cache entry.
---
Nitpick comments:
In `@api/material.js`:
- Around line 1109-1110: createMultiGradientShaderMaterial lacks the defensive
onBindObservable refresh used by createColorReplaceShaderMaterial; add an
onBindObservable observer on the material created in
createMultiGradientShaderMaterial that calls
flock.updateFogUniformsForShaderMaterial(shaderMaterial) (the same pattern used
for createColorReplaceShaderMaterial) so fog uniforms are refreshed on every
bind in case scene.fog* was mutated outside setFog(); keep the existing
flock.registerFogAwareShaderMaterial call but add the onBindObservable hook to
ensure redundant safety.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 52d9d4d8-832b-4f74-8331-48879d9c3791
📒 Files selected for processing (2)
api/material.jstests/materials.test.js
🚧 Files skipped from review as they are similar to previous changes (1)
- tests/materials.test.js
Uh oh!
There was an error while loading. Please reload this page.
The cacheKey is lowercased before storage, so deserialising materialName from it produces a lowercased path. On case-sensitive hosts (Linux, PWA service worker cache) this breaks asset lookup. Store texName with its original case in metadata.texName when the material is created in getOrCreateMaterial, and prefer that in getMaterialParamsFromMesh over the cacheKey-derived string. Keeps the cacheKey-split as a fallback for any pre-existing materials that lack the field. Addresses CodeRabbit review comment (offline PWA behaviour). https://claude.ai/code/session_01Jckn16VqMzdP7SYXerpW9y
Contributor
There was a problem hiding this comment.
♻️ Duplicate comments (2)
api/material.js (2)
1611-1618:⚠️ Potential issue | 🟠 MajorDon't fold
texNameinto a case-insensitive cache key.This helper still lowercases the texture portion of the material identity, so
Foo.PNGandfoo.pngcollapse to the same cached material. On case-sensitive hosts and the service-worker cache, that can reuse/skip the wrong asset even thoughmetadata.texNamenow preserves the original case.💡 Suggested direction
const texName = String(getTexName(v)); const alphaKey = parseFloat(getAlpha(v)).toFixed(2); const glow = typeof v === "object" && v !== null && !Array.isArray(v) ? (v.glow ?? false) : false; const glowKey = glow ? "glow" : "noglow"; - return `mat_${colorKey}_${alphaKey}_${texName}_${glowKey}`.toLowerCase();+ const normalizedColorKey = String(colorKey).toLowerCase();+ return `mat_${normalizedColorKey}_${alphaKey}_${texName}_${glowKey}`;Please mirror the same change where
cacheKeyis built ingetOrCreateMaterial(). As per coding guidelines, "Don't introduce changes that would break offline PWA behaviour".🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@api/material.js` around lines 1611 - 1618, The material cache key lowercases the texture name causing case-sensitive hosts/service-worker caches to mix assets; update the key builder to avoid lowercasing texName while keeping the rest lowercased: in the key assembly that uses colorKey, alphaKey, texName, and glowKey (the block returning `mat_${colorKey}_${alphaKey}_${texName}_${glowKey}`), remove the .toLowerCase() call and instead apply .toLowerCase() only to the other components (e.g., colorKey and glowKey) so texName preserves its original case; apply the same change where cacheKey is constructed in getOrCreateMaterial() so both places treat texName case-sensitively.
220-226:⚠️ Potential issue | 🟠 MajorKeep the base material color when a custom glow tint is provided.
m.metadata.glowColoralready carries the glow-only tint, but this branch still rewritesmaterialParams.colorfor scalar colors. That permanently changes the managed material, soclearEffects()/later round-trips come back in the glow tint instead of the original base color.💡 Suggested fix
if (params) { const materialParams = { ...params, - color:- glowColor && !Array.isArray(baseColor)- ? flock.getColorFromString(glowColor)- : baseColor,+ color: baseColor, glow: true, }; flock.setMaterialWithCleanup(m, materialParams); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@api/material.js` around lines 220 - 226, The branch that sets materialParams.color uses glowColor to rewrite the base color for scalar baseColor values, which overwrites the managed material; change the logic so materialParams.color always preserves baseColor (never replace it with flock.getColorFromString(glowColor)), and if a glow tint must be stored on the material, set a separate property (e.g., materialParams.glowColor or metadata.glowColor) to flock.getColorFromString(glowColor) instead; update the assignment around materialParams, color, glowColor, baseColor and flock.getColorFromString so the base color remains unchanged while the glow tint is stored independently.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@api/material.js`:
- Around line 1611-1618: The material cache key lowercases the texture name
causing case-sensitive hosts/service-worker caches to mix assets; update the key
builder to avoid lowercasing texName while keeping the rest lowercased: in the
key assembly that uses colorKey, alphaKey, texName, and glowKey (the block
returning `mat_${colorKey}_${alphaKey}_${texName}_${glowKey}`), remove the
.toLowerCase() call and instead apply .toLowerCase() only to the other
components (e.g., colorKey and glowKey) so texName preserves its original case;
apply the same change where cacheKey is constructed in getOrCreateMaterial() so
both places treat texName case-sensitively.
- Around line 220-226: The branch that sets materialParams.color uses glowColor
to rewrite the base color for scalar baseColor values, which overwrites the
managed material; change the logic so materialParams.color always preserves
baseColor (never replace it with flock.getColorFromString(glowColor)), and if a
glow tint must be stored on the material, set a separate property (e.g.,
materialParams.glowColor or metadata.glowColor) to
flock.getColorFromString(glowColor) instead; update the assignment around
materialParams, color, glowColor, baseColor and flock.getColorFromString so the
base color remains unchanged while the glow tint is stored independently.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for freeto join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
This PR refactors material parameter extraction into a reusable utility function and enhances the multi-color gradient shader with fog support. It also improves material caching by including glow state in cache keys.
Key Changes
New
getMaterialParamsFromMesh()utility function: Extracts material parameters (color, materialName, alpha, glow) from a mesh's material, supporting both cached materials (with metadata.cacheKey) and raw GLTF materials. This eliminates duplicate parameter extraction logic across multiple methods.Refactored material effect methods: Updated
glowMesh(),setAlpha(), andclearEffects()to use the new utility function, reducing code duplication and improving maintainability.Enhanced multi-color gradient shader with fog support:
multiColorGradienttomultiColorGradientFogto avoid collisionsregisterFogAwareShaderMaterial()andupdateFogUniformsForShaderMaterial()callsImproved material cache key generation: Modified
generateMaterialCacheKey()to include glow state in the cache key (mat_${colorKey}_${alphaKey}_${texName}_${glowKey}), ensuring materials with different glow states are cached separately.Added comprehensive gradient material tests: New test suite verifying that gradient materials (2-color and 3+ color) are properly preserved after applying glow, clearing effects, and setting alpha values. Also tests cache hit behavior for repeated material application.
Added gradient material block generator: New Blockly code generator for gradient materials that outputs material descriptor objects with color, materialName, and alpha properties.
Notable Implementation Details
getMaterialParamsFromMesh()function handles both cached materials (parsing metadata.cacheKey) and raw GLTF materials (extracting from diffuseColor/albedoColor).toFixed(2)) for better cache consistencyhttps://claude.ai/code/session_01Jckn16VqMzdP7SYXerpW9y
Summary by CodeRabbit
New Features
Improvements
Tests