Uh oh!
There was an error while loading. Please reload this page.
Limit color array to 16 elements in flock material shader - #436
Merged
Conversation
- createMaterial: accept 2+ colors for two-color materials (was exact match on 2) - createMultiColorGradientMaterial: clamp colors to max 16 matching shader array size
Contributor
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (3)
📝 WalkthroughWalkthroughMulti-color/material handling was expanded: arrays with length ≥2 are now treated as multi-color/gradient inputs; shader color arrays are capped at 16 and colorCount aligned. Gradient texture creation returns a DynamicTexture. Map material assignment/update paths changed. Block-to-map logic distinguishes material-blocks vs raw color inputs and treats empty arrays as empty. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
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)
Comment |
Contributor
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
api/material.js (1)
1-1:⚠️ Potential issue | 🟡 MinorPrettier formatting issue flagged by pipeline.
The GitHub Actions pipeline reports a Prettier formatting issue. Run
prettier --write api/material.jsto fix code style before merging.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@api/material.js` at line 1, The file has Prettier formatting issues for the top-level declaration (variable "flock" in api/material.js); run the formatter to fix it by executing prettier --write api/material.js (or apply your project's configured Prettier settings/IDE formatter) and commit the resulting changes so the declaration and overall file conform to the project's Prettier rules.
🧹 Nitpick comments (1)
api/material.js (1)
1002-1008: Debug log shows uncapped color count.When
colors.length > 16, the debug log at line 1003 will show the original count (e.g., "Color count: 20"), but the shader actually receives the capped value of 16. This could be misleading during debugging.🔧 Suggested fix for consistency
+ const cappedColorCount = Math.min(colors.length, 16);+ if (flock.materialsDebug) { - console.log("Color count:", colors.length);+ console.log("Color count:", cappedColorCount, colors.length > 16 ? `(truncated from ${colors.length})` : ""); console.log("Color array:", color3Array); } - shaderMaterial.setInt("colorCount", Math.min(colors.length, 16));+ shaderMaterial.setInt("colorCount", cappedColorCount);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@api/material.js` around lines 1002 - 1008, The debug logging prints the uncapped colors.length which can mislead because shaderMaterial.setInt("colorCount", Math.min(colors.length, 16)) actually caps to 16; update the debug output (when flock.materialsDebug is true) to log the capped count (e.g., Math.min(colors.length, 16)) or both original and capped values so the logs match the shader state—modify the block that references flock.materialsDebug, colors.length, color3Array, shaderMaterial.setInt("colorCount", ...) and shaderMaterial.setArray3("colors", ...) accordingly.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@api/material.js`:
- Line 1: The file has Prettier formatting issues for the top-level declaration
(variable "flock" in api/material.js); run the formatter to fix it by executing
prettier --write api/material.js (or apply your project's configured Prettier
settings/IDE formatter) and commit the resulting changes so the declaration and
overall file conform to the project's Prettier rules.
---
Nitpick comments:
In `@api/material.js`:
- Around line 1002-1008: The debug logging prints the uncapped colors.length
which can mislead because shaderMaterial.setInt("colorCount",
Math.min(colors.length, 16)) actually caps to 16; update the debug output (when
flock.materialsDebug is true) to log the capped count (e.g.,
Math.min(colors.length, 16)) or both original and capped values so the logs
match the shader state—modify the block that references flock.materialsDebug,
colors.length, color3Array, shaderMaterial.setInt("colorCount", ...) and
shaderMaterial.setArray3("colors", ...) accordingly.
setMaterialWithCleanup expects a descriptor object, not a Babylon.js material instance. Passing a StandardMaterial to it caused getOrCreateMaterial to fall back to #ffffff, producing a white ground instead of the gradient. Directly assign mesh.material when the gradient material is pre-built. https://claude.ai/code/session_01C23exQ7PpjerGEgnMYJL6W
…earGradientTexture The function created a Texture(null, scene) that was never returned or used, but was registered in the scene's texture list and triggered a 'missing texture' warning/visual on every call (terrain change or color list update). Also dispose the previous gradient material when replacing it on the same mesh to avoid accumulating orphaned StandardMaterials and DynamicTextures. https://claude.ai/code/session_01C23exQ7PpjerGEgnMYJL6W
Contributor
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
api/scene.js (1)
201-219:⚠️ Potential issue | 🟡 MinorAddress material disposal asymmetry to prevent memory leaks.
When transitioning from a non-gradient material (set via the
elsebranch) to a gradient material (set via theifbranch with 2+ colors), the old material is not disposed. ThesetMaterialWithCleanupfunction marks materials withmetadata.isManaged = true, but the gradient path only checks if the material name is "mapGradientMat", ignoring this flag.Change the disposal condition to properly clean up any previous material:
Fix
mesh.material = standardMat; - if (oldMat && oldMat.name === "mapGradientMat") {+ if (oldMat && oldMat !== standardMat) { oldMat.dispose(true, true); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@api/scene.js` around lines 201 - 219, The gradient-material branch creates a new StandardMaterial ("mapGradientMat") but only disposes prior materials if oldMat.name === "mapGradientMat", which misses managed materials set by setMaterialWithCleanup; update the disposal logic around mesh.material assignment (where oldMat, standardMat, dt are used) to dispose the previous material when oldMat exists and is marked managed (oldMat.metadata?.isManaged === true) or when its name === "mapGradientMat", i.e., check oldMat.metadata?.isManaged || oldMat.name === "mapGradientMat" and call oldMat.dispose(true, true) so any previously managed non-gradient material is cleaned up when assigning the new gradient material.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@api/scene.js`:
- Around line 201-219: The gradient-material branch creates a new
StandardMaterial ("mapGradientMat") but only disposes prior materials if
oldMat.name === "mapGradientMat", which misses managed materials set by
setMaterialWithCleanup; update the disposal logic around mesh.material
assignment (where oldMat, standardMat, dt are used) to dispose the previous
material when oldMat exists and is marked managed (oldMat.metadata?.isManaged
=== true) or when its name === "mapGradientMat", i.e., check
oldMat.metadata?.isManaged || oldMat.name === "mapGradientMat" and call
oldMat.dispose(true, true) so any previously managed non-gradient material is
cleaned up when assigning the new gradient material.
… lists When replacing a GradientMaterial (or StandardMaterial) via setMaterialWithCleanup, the old material was disposed with forceDisposeEffect=true. This force-destroyed the compiled WebGL shader Effect from the global cache, breaking the newly assigned material which shares the same Effect. On the next render frame, Babylon.js would show the "black and red" fallback while recompiling the shader. Changed dispose(true, true) to dispose(false, true) in both setMaterialWithCleanup and applyMaterialToGround so the compiled Effect stays cached and available to the replacement material. Textures are still properly disposed (second param remains true). https://claude.ai/code/session_01C23exQ7PpjerGEgnMYJL6W
… flash When a GradientMaterial already exists on the ground mesh and the color list changes, instead of creating a new GradientMaterial and disposing the old one (which could destroy the shared compiled shader Effect and cause a one-frame "black and red" fallback while it recompiles), update the existing material's bottomColor and topColor directly. This avoids any shader lifecycle issues entirely: no disposal, no recompilation, no Effect reference count concerns. The material cache is re-keyed to reflect the new color combination. https://claude.ai/code/session_01C23exQ7PpjerGEgnMYJL6W
…operations
When a lists_create_with block is mutated (e.g. adding a new color item), Blockly
fires a change event while connections are still being rebuilt. readColourValue
returns an empty array [] rather than null, so the existing null-check retry guard
did not fire. The empty array passed through to createMaterial which fell into the
StandardMaterial branch, loading the semi-transparent none.png texture and making
the ground invisible ("missing texture").
Extend the retry condition to also cover an empty color list so the update is
deferred until after the mutator finishes and all color connections are restored.
https://claude.ai/code/session_01C23exQ7PpjerGEgnMYJL6W…RIAL updateMapFromBlock always treated the MATERIAL input block as a material block, calling readColourFromInputOrShadow(materialBlock, "BASE_COLOR"). When the user connects a lists_create_with block (or a single colour block) directly to MATERIAL instead of via a material block, that input has no BASE_COLOR sub-input, so the read always returned null and the retry loop never resolved. Detect when materialBlock is not a material block and read it directly via readColourValue, defaulting the texture to "none.png". https://claude.ai/code/session_01C23exQ7PpjerGEgnMYJL6W
…/list blocks
When a colour list (lists_create_with) or single colour block is wired directly
to the MATERIAL input, the live-update was wrapping the value in a descriptor
{ color, materialName: "none.png" } which routed through GradientMaterial.
The generated JS passes the raw array/string directly, which routes through the
StandardMaterial + canvas-gradient-texture path instead.
Pass read.value directly to createMap for non-material blocks so both paths
produce the same visual result.
https://claude.ai/code/session_01C23exQ7PpjerGEgnMYJL6W- blockmesh.js: compute read and mapArg in each branch, then share the single colorIsEmpty + retry guard and createMap call instead of repeating it in both branches - scene.js: remove two comments that just described the following line https://claude.ai/code/session_01C23exQ7PpjerGEgnMYJL6W
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
api/scene.js (1)
201-219:⚠️ Potential issue | 🟠 MajorClean up the previous ground material on this branch too.
This no longer goes through
setMaterialWithCleanup(), but it only disposes the old material when its name is exactlymapGradientMat. Switching the ground from a cached textured material or another managed material to a raw colour-list gradient will leave the previous material alive, so repeated live edits leak GPU memory and staleflock.materialCacheentries.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@api/scene.js` around lines 201 - 219, The branch currently only disposes oldMat when its name === "mapGradientMat", causing material leaks; change the logic in the mesh material swap (the block that creates standardMat and assigns mesh.material) to always clean up the previous material when it exists and is not the newly created standardMat: call oldMat.dispose(...) and also remove any references from flock.materialCache (delete by oldMat.name or cache key used elsewhere) so cached entries and GPU resources are freed; keep the guard to avoid disposing the same instance (oldMat !== standardMat) to prevent double-dispose and ensure you still set mesh.material = standardMat afterwards.
🤖 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/scene.js`:
- Around line 227-247: The branch that reuses an existing GradientMaterial
updates the cache key but never applies the new opacity to the material
instance; set the material's alpha before re-keying: parse mat.alpha (e.g. const
alpha = parseFloat(mat.alpha ?? 1)) and assign it to existingMat.alpha (or the
correct opacity property on flock.GradientMaterial) so the rendered opacity
matches the cache key update; keep the existing updates to existingMat.name,
existingMat.metadata.cacheKey and flock.materialCache[newKey] as-is.
In `@ui/blockmesh.js`:
- Around line 788-803: extractMaterialInfo can return the sentinel "NONE", which
should be normalized before calling flock.createMap so the live-update path in
api/scene.js sees a falsy/"none.png" materialName; update the code around where
extractMaterialInfo is used (referencing extractMaterialInfo, textureSet,
flock.createMap and updateMapFromBlock) to map the "NONE" sentinel to the
API-expected form (either null/undefined or "none.png" / lowercased as needed)
before passing materialName into flock.createMap so flat-map gradient
live-updates take the in-place path.
- Around line 791-817: The retry flag is being cleared before re-entering
updateMapFromBlock so a permanently-empty color keeps scheduling new frames; in
both callbacks that set block.__mapRetry = true, stop clearing it inside the
requestAnimationFrame callback (remove or move the block.__mapRetry = false
line) and instead only clear __mapRetry when the block's color value actually
changes or once updateMapFromBlock has successfully processed a non-empty value;
locate the occurrences of block.__mapRetry, the requestAnimationFrame callbacks,
and the updateMapFromBlock function to implement this (ensure block.__mapRetry
remains true while awaiting a real value and is reset only on a meaningful
change or success).
---
Outside diff comments:
In `@api/scene.js`:
- Around line 201-219: The branch currently only disposes oldMat when its name
=== "mapGradientMat", causing material leaks; change the logic in the mesh
material swap (the block that creates standardMat and assigns mesh.material) to
always clean up the previous material when it exists and is not the newly
created standardMat: call oldMat.dispose(...) and also remove any references
from flock.materialCache (delete by oldMat.name or cache key used elsewhere) so
cached entries and GPU resources are freed; keep the guard to avoid disposing
the same instance (oldMat !== standardMat) to prevent double-dispose and ensure
you still set mesh.material = standardMat afterwards.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
- scene.js: apply mat.alpha to the GradientMaterial instance when updating in-place; previously only the cache key reflected the new alpha value - blockmesh.js: normalize the "NONE" sentinel from extractMaterialInfo to "none.png" so the in-place gradient path in scene.js is correctly matched - blockmesh.js: fix infinite requestAnimationFrame loop when a colour list is permanently empty; now retries at most once by tracking wasRetrying before clearing the flag https://claude.ai/code/session_01C23exQ7PpjerGEgnMYJL6W
… block getRawColor in applyMaterialToHierarchy was returning the raw array (e.g. ["#ff5733"]) for a single-element colour list. getColorFromString then matched the regex (arrays coerce to string for .test()) but tried to call .toLowerCase() on the array, throwing a TypeError that prevented setMaterialWithCleanup from ever being called, leaving the mesh with no material applied. Normalise single-element arrays to a plain string in getRawColor, matching the same normalisation that createMaterial already performs. https://claude.ai/code/session_01C23exQ7PpjerGEgnMYJL6W
The colorReplace shader already handles two colours: color[0] replaces near-white pixels and color[1] tints grey pixels. A third colour in the list now replaces near-black pixels (brightness < 0.05, low saturation). - Added darkColor and colorCount uniforms to the fragment shader - The black-replacement branch only activates when colorCount >= 3 so existing two-colour materials are unaffected - darkColor is set from colors[2] when present; colorCount carries the length of the colours array https://claude.ai/code/session_01C23exQ7PpjerGEgnMYJL6W
Uh oh!
There was an error while loading. Please reload this page.
This was referenced Mar 22, 2026
This was referenced Mar 30, 2026
Merged
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
Updated the flock material color handling to enforce a maximum of 16 colors, matching the shader's array size limit. This prevents potential issues from exceeding the shader's capacity while gracefully handling excess colors.
Key Changes
.slice(0, 16)to limit the color3Array to maximum 16 elements before mappingMath.min(colors.length, 16)to ensure the shader receives the correct capped countImplementation Details
The changes ensure that:
https://claude.ai/code/session_01C23exQ7PpjerGEgnMYJL6W
Summary by CodeRabbit
Bug Fixes
Improvements