Conversation
|
No actionable comments were generated in the recent review. 🎉 📝 WalkthroughWalkthroughDevice-backed rendering migration: regl removed in favor of Changes
Sequence Diagram(s)sequenceDiagram
participant App as App / Story
participant Graph as Graph
participant Device as Device (`@luma.gl`)
participant GPU as GPU/Driver
rect rgba(70,130,180,0.5)
App->>Graph: new Graph(div, config, devicePromise?)
alt external devicePromise provided
App->>Device: supply devicePromise
Device-->>Graph: device resolved
else Graph creates device
Graph->>Device: createDevice(canvas, pixelRatio)
Device-->>Graph: device ready
end
end
rect rgba(34,139,34,0.5)
Graph->>Device: ensureDevice()
Graph->>Device: create Textures / Framebuffers / UniformStores / Models
Device-->>GPU: allocate GPU resources
end
rect rgba(255,165,0,0.5)
loop per-frame
Graph->>Device: beginRenderPass(target)
Graph->>Model: setUniforms / setBindings / draw()
Model->>GPU: draw()
alt readback needed
Graph->>Device: readPixelsToArrayWebGL(...)
Device-->>Graph: Float32Array
end
Graph->>Device: endRenderPass()
end
end
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 1❌ Failed checks (1 warning)
✅ 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 |
rokotyan
force-pushed
the
next
branch
3 times, most recently
from
6295e9f to
e5582f8
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 13
🧹 Nitpick comments (28)
src/modules/Points/fill-sampled-points.frag (1)
1-12: LGTM! Clean GLSL ES 3.0 migration.The shader has been correctly migrated to GLSL ES 3.0:
- Version directive is appropriate
varying→infor fragment input- Explicit
out vec4 fragColordeclarationgl_FragColor→fragColoroutputThe shader logic is preserved and the migration aligns with the PR's objective to update all shaders for luma.gl compatibility.
Optional cleanup: The
#ifdef GL_ESblock (lines 2-4) is always true in WebGL contexts and could be simplified to just the precision directive, though keeping it is harmless.src/modules/Store/index.ts (2)
21-25: Consider exporting Mat4Array if used by consumers.The type definition is correct and provides good type safety. However, since
transformationMatrix4x4is a public getter, consumers of this method may benefit from having access to theMat4Arraytype for their own type annotations.🔎 Suggested change to export the type
/** * Type alias for a 4x4 matrix stored as a 16-element array in column-major order. * Used for std140 uniform buffer layout compatibility. */ -type Mat4Array = [number, number, number, number, number, number, number, number, number, number, number, number, number, number, number, number] +export type Mat4Array = [number, number, number, number, number, number, number, number, number, number, number, number, number, number, number, number]
155-170: Excellent implementation with thorough documentation!The matrix conversion logic is correct and the extensive documentation clearly explains the rationale for std140 alignment and the conversion process. The validation is good defensive programming.
One optional consideration: if this getter is called frequently (e.g., every frame during rendering), the array allocation could create GC pressure. Consider caching the result and invalidating when
transformchanges, or verify the call frequency is low enough that this isn't a concern.💡 Optional: Type assertion for explicit clarity
While TypeScript should correctly infer the type, you could add an explicit assertion for clarity:
// Convert 3x3 to 4x4 matrix in column-major order - return [ + return [ t[0], t[1], t[2], 0, // Column 0 t[3], t[4], t[5], 0, // Column 1 t[6], t[7], t[8], 0, // Column 2 0, 0, 0, 1, // Column 3 (homogeneous) - ] + ] as Mat4Arraysrc/modules/Points/fill-sampled-points.vert (1)
10-27: std140 layout padding can be reduced by reordering fields, though CPU-side code must match.The current field order introduces padding per std140 rules:
pointsTextureSize(float) at offset 0transformationMatrix(mat4) requires 16-byte alignment → starts at offset 16 (12 bytes padding)spaceSize(float) at offset 80screenSize(vec2) requires 8-byte alignment → starts at offset 88 (4 bytes padding)Reordering to
mat4, vec2, float, floatwould eliminate padding. However, note that the TypeScript setup (index.ts:895) has an explicit constraint: "Order MUST match shader declaration order (std140 layout)". Any reordering requires coordinated changes in both the shader and the CPU-side struct. Given that other shaders in the codebase use varied field orderings without optimization, this is an acceptable tradeoff unless padding becomes a measurable concern.src/modules/ForceManyBody/force-level.frag (2)
1-5: Add precision qualifiers to sampler uniforms.In GLSL ES 3.00, sampler uniforms should have explicit precision qualifiers. While the global
precision highp float;declaration covers floating-point types, samplers require their own precision specification.🔎 Proposed fix
#version 300 es precision highp float; +precision highp sampler2D; uniform sampler2D positionsTexture; uniform sampler2D levelFbo;
105-106: Consider named constants for loop bounds.The hardcoded values
12.0and4.0in the nested loops lack context. Named constants would improve readability, e.g.,MAX_NEIGHBOR_SEARCH_RANGEor similar.Example
At the top of the shader, after line 38:
const float MAX_NEIGHBOR_SEARCH_RANGE = 12.0; const float NEIGHBOR_CELL_COUNT = 4.0;Then update the loops:
- for (float i = 0.0; i < 12.0; i += 1.0) { - for (float j = 0.0; j < 4.0; j += 1.0) { + for (float i = 0.0; i < MAX_NEIGHBOR_SEARCH_RANGE; i += 1.0) { + for (float j = 0.0; j < NEIGHBOR_CELL_COUNT; j += 1.0) {src/modules/Shared/clear.frag (1)
1-9: Consider removing redundant#ifdef GL_ESguard.With
#version 300 es, the shader is explicitly GLSL ES 3.00, so the#ifdef GL_ESguard is redundant—GL_ESis always defined. The guard is harmless but can be removed for cleaner code.🔎 Proposed cleanup
#version 300 es -#ifdef GL_ES precision highp float; -#endif out vec4 fragColor; void main() { fragColor = vec4(0.0); }src/graph/utils/error-message.ts (1)
1-6: Update JSDoc to document the newerrorparameter.The function signature now accepts an
errorparameter, but the JSDoc comment on lines 1-5 doesn't document it.🔎 Proposed fix
/** * Creates and appends an error message element to the container * @param container The HTML element to append the error message to + * @param error The specific error message to display * @returns The created error div element */src/helper.ts (1)
105-113: Consider the security implications of allowingstylein ALLOWED_ATTR.While the sanitization is a good security practice, allowing the
styleattribute could enable CSS-based attacks (e.g., overlay attacks, data exfiltration viabackground-image: url(...)). For attribution text,stylemay not be necessary.🔎 Suggested safer defaults
return DOMPurify.sanitize(html, { ALLOWED_TAGS: ['a', 'b', 'i', 'em', 'strong', 'span', 'div', 'p', 'br'], - ALLOWED_ATTR: ['href', 'target', 'class', 'id', 'style'], + ALLOWED_ATTR: ['href', 'target', 'class', 'id'], ALLOW_DATA_ATTR: false, ...options, })src/modules/Points/draw-points.frag (2)
1-5: Redundant#ifdef GL_ESguard after#version 300 es.With
#version 300 es, the shader is explicitly GLSL ES 3.0, whereGL_ESis always defined. The#ifdef GL_EScheck is unnecessary and can be removed for cleaner code.🔎 Suggested simplification
#version 300 es -#ifdef GL_ES precision highp float; -#endif
166-175: Consider using a switch statement or lookup table for shape dispatch.The chain of
if-else ifcomparisons for shape selection works but has O(n) complexity. For 8+ shapes, a switch statement could be slightly more efficient and readable in GLSL.src/modules/Points/update-position.frag (1)
1-5: Redundant#ifdef GL_ESguard after#version 300 es.Same issue as in
draw-points.frag. With explicit GLSL ES 3.0 versioning, theGL_EScheck is always true and can be removed.🔎 Suggested simplification
#version 300 es -#ifdef GL_ES precision highp float; -#endifsrc/modules/Lines/draw-curve-line.vert (1)
15-78: LGTM! Dual-path uniform handling is well-structured.The shader correctly implements both uniform-buffer and non-uniform-buffer code paths. The
mat4in the UBO path andmat3in the non-UBO path is intentional for std140 alignment requirements.Consider documenting the reason for
mat4vsmat3in a comment, as this subtle difference could confuse future maintainers. The maintenance burden of keeping both paths in sync may warrant consideration of a build-time shader variant system if not already in place.src/modules/Points/draw-points.vert (1)
154-202: Consider consolidating duplicated image atlas UV code.The image atlas UV calculation logic (lines 175-187 and 189-201) is identical in both the
USE_UNIFORM_BUFFERSand non-UBO branches. Since the uniforms are accessed via the same identifiers (through macros or direct declarations), this code could be moved outside the conditional blocks to eliminate ~27 lines of duplication.Potential refactor to reduce duplication
Move the image atlas calculation after the
#endifon line 202:#ifdef USE_UNIFORM_BUFFERS - if (hasImages <= 0.0 || imageIndex < 0.0 || imageIndex >= imageCount) { - imageAtlasUV = vec4(-1.0); - } else { - // Calculate image atlas UV coordinates based on imageIndex - float atlasCoordIndex = imageIndex; - // Calculate the position in the texture grid - float texX = mod(atlasCoordIndex, imageAtlasCoordsTextureSize); - float texY = floor(atlasCoordIndex / imageAtlasCoordsTextureSize); - // Convert to texture coordinates (0.0 to 1.0) - vec2 atlasCoordTexCoord = (vec2(texX, texY) + 0.5) / imageAtlasCoordsTextureSize; - vec4 atlasCoords = texture(imageAtlasCoords, atlasCoordTexCoord); - imageAtlasUV = atlasCoords; - } #else - if (hasImages <= 0.0 || imageIndex < 0.0 || imageIndex >= imageCount) { - imageAtlasUV = vec4(-1.0); - } else { - // Calculate image atlas UV coordinates based on imageIndex - float atlasCoordIndex = imageIndex; - // Calculate the position in the texture grid - float texX = mod(atlasCoordIndex, imageAtlasCoordsTextureSize); - float texY = floor(atlasCoordIndex / imageAtlasCoordsTextureSize); - // Convert to texture coordinates (0.0 to 1.0) - vec2 atlasCoordTexCoord = (vec2(texX, texY) + 0.5) / imageAtlasCoordsTextureSize; - vec4 atlasCoords = texture(imageAtlasCoords, atlasCoordTexCoord); - imageAtlasUV = atlasCoords; - } #endif + + if (hasImages <= 0.0 || imageIndex < 0.0 || imageIndex >= imageCount) { + imageAtlasUV = vec4(-1.0); + } else { + // Calculate image atlas UV coordinates based on imageIndex + float atlasCoordIndex = imageIndex; + float texX = mod(atlasCoordIndex, imageAtlasCoordsTextureSize); + float texY = floor(atlasCoordIndex / imageAtlasCoordsTextureSize); + vec2 atlasCoordTexCoord = (vec2(texX, texY) + 0.5) / imageAtlasCoordsTextureSize; + vec4 atlasCoords = texture(imageAtlasCoords, atlasCoordTexCoord); + imageAtlasUV = atlasCoords; + }src/stories/beginners/quick-start.ts (1)
3-3: Good addition of lifecycle cleanup.The
destroyfunction properly delegates tograph.destroy()for resource cleanup, aligning with other stories in this PR.Consider making the return type non-optional since
destroyis always provided:-export const quickStart = (): { graph: Graph; div: HTMLDivElement; destroy?: () => void } => { +export const quickStart = (): { graph: Graph; div: HTMLDivElement; destroy: () => void } => {This would be consistent with other story files like
with-labels.tswhich declares it as non-optional.Also applies to: 49-53
src/modules/Lines/hovered-line-index.frag (1)
2-4: RedundantGL_ESguard.The
#ifdef GL_EScheck is redundant when using#version 300 es, as the ES version directive already implies an OpenGL ES context. This is a minor nit - the guard is harmless but unnecessary.🔎 Optional cleanup
#version 300 es -#ifdef GL_ES precision highp float; -#endifsrc/stories/beginners/basic-set-up/index.ts (1)
162-166: Consider cleaning up event listeners in the destroy function.The
destroyfunction only callsgraph.destroy(), but there are several event listeners attached to DOM elements (e.g.,pauseButton,fitViewButton,zoomButton, etc.) that are not removed. If the story is destroyed and reloaded, this could lead to orphaned listeners.🔎 Suggested enhancement
const destroy = (): void => { + pauseButton.removeEventListener('click', togglePause) + fitViewButton.removeEventListener('click', fitView) + zoomButton.removeEventListener('click', zoomIn) + selectPointButton.removeEventListener('click', selectPoint) + selectPointsInAreaButton.removeEventListener('click', selectPointsInArea) graph.destroy() }src/stories/beginners.stories.ts (1)
91-94: Static analysis flagged innerHTML, but risk is minimal here.The static analyzer flagged this as a potential XSS vector. Since the content is a hardcoded error message (not user input), the actual risk is negligible. However, using DOM APIs would silence the warning and align with safer practices.
🔎 Alternative using DOM API
} catch (error) { console.error('Failed to load PointLabels story:', error) - div.innerHTML = '<div style="display: flex; align-items: center; justify-content: center; height: 100%; color: #ff0000;">Failed to load story</div>' + const errorDiv = document.createElement('div') + errorDiv.style.cssText = 'display: flex; align-items: center; justify-content: center; height: 100%; color: #ff0000;' + errorDiv.textContent = 'Failed to load story' + div.appendChild(errorDiv) }src/stories/beginners/remove-points/index.ts (1)
91-95: Good addition of destroy lifecycle method.The
destroyfunction properly delegates cleanup tograph.destroy(). One minor consideration: thepauseButtonclick listener (line 89) is not removed. If thedivremains in the DOM after callingdestroy(), this listener would persist.🔎 Optional: Remove event listener in destroy
+ const togglePauseHandler = togglePause + pauseButton.addEventListener('click', togglePauseHandler) - pauseButton.addEventListener('click', togglePause) const destroy = (): void => { + pauseButton.removeEventListener('click', togglePauseHandler) graph.destroy() }src/modules/ForceGravity/index.ts (1)
67-93: Consider optimizing binding updates.The
runmethod has good guards for destroyed resources and missing dependencies. However,setBindingsis called on every frame (lines 81-84), which may be inefficient if the texture reference hasn't changed. IfpreviousPositionTextureis stable across frames, consider tracking whether rebinding is necessary.src/modules/Points/draw-highlighted.frag (1)
6-29: Uniform block appears to be shared with vertex shader.The
drawHighlightedUniformsblock contains many fields (size, transformationMatrix, screenSize, etc.) but this fragment shader only useswidth(line 42). This is a valid pattern when sharing a uniform buffer between vertex and fragment stages, but the unused fields add memory overhead.If the vertex shader doesn't need all these fields either, consider splitting into separate uniform blocks per shader stage.
src/stories/test-luma-migration.ts (1)
8-66: Consider usingpointDefaultColorinstead of deprecatedpointColor.Based on the config context,
pointColor(line 11) is deprecated in favor ofpointDefaultColor. While this is a test file, using the current API would better validate the migration.🔎 Update to non-deprecated config option
const config: GraphConfigInterface = { spaceSize: 4096, backgroundColor: '#f25a8a', // '#2d313a', - pointColor: '#f069b4', + pointDefaultColor: '#f069b4', pointSize: 40,src/modules/ForceCenter/index.ts (2)
87-97: Consider sharing the vertex buffer between clear and force passes.Both
clearVertexCoordBufferandforceVertexCoordBuffercontain identical quad vertex data. A single shared buffer would reduce memory allocation without affecting functionality.
212-230:clearCentermassCommandis created but unused.The
clearColor: [0, 0, 0, 0]parameter inbeginRenderPasshandles clearing, makingclearCentermassCommandredundant. Consider removing the command creation ininitPrograms()and cleanup indestroy()to simplify the code.🔎 Verification script
#!/bin/bash # Check if clearCentermassCommand is used anywhere in ForceCenter rg -n 'clearCentermassCommand' src/modules/ForceCenter/index.tssrc/modules/Lines/index.ts (2)
384-390: Large texture initialization could be slow.Creating a
Float32Array(screenWidth * screenHeight * 4)for the link index texture could be expensive for large screens (e.g., 4K displays). Consider whether zero-initialization is actually necessary since the framebuffer is cleared before use anyway.🔎 Skip unnecessary initialization
this.linkIndexTexture = device.createTexture({ width: screenWidth, height: screenHeight, format: 'rgba32float', usage: Texture.SAMPLE | Texture.RENDER | Texture.COPY_DST, }) - this.linkIndexTexture.copyImageData({ - data: new Float32Array(screenWidth * screenHeight * 4).fill(0), - bytesPerRow: screenWidth, - mipLevel: 0, - x: 0, - y: 0, - }) + // Note: Texture will be cleared via render pass before first use
625-639: Double render pass for clearing and drawing is inefficient.The code creates one render pass just to clear (lines 625-629), then creates another to draw (lines 632-639). These should be combined into a single render pass with
clearColorset and drawing in the same pass.🔎 Combine into single render pass
- // Clear and render to index buffer - const clearPass = this.device.beginRenderPass({ - framebuffer: this.linkIndexFbo, - clearColor: [0, 0, 0, 0], - }) - clearPass.end() - // Render to index buffer for picking/hover detection const indexPass = this.device.beginRenderPass({ framebuffer: this.linkIndexFbo, - // Note: We explicitly set clearColor to [0, 0, 0, 0] to prevent re-clearing the framebuffer. - // Without this, luma.gl would use its default clearColor of [0, 0, 0, 1] (opaque black) - clearColor: [0, 0, 0, 0], // or `clearColor: false` + clearColor: [0, 0, 0, 0], }) this.drawCurveCommand.draw(indexPass) indexPass.end()src/modules/Points/draw-highlighted.vert (2)
11-29: Uniform buffer block layout may have alignment issues.The
std140layout has specific alignment rules. In this block:
float size(4 bytes) followed bymat4(requires 16-byte alignment) - there will be 12 bytes paddingvec2 screenSize(8 bytes) may need padding depending on what followsWhile this may work, explicit padding or reordering could improve clarity and prevent subtle bugs. Consider grouping scalars together and placing larger types (mat4, vec4) first.
99-115: Duplicated color blending logic between#ifdefbranches.The code inside
#ifdef USE_UNIFORM_BUFFERS(lines 100-106) is identical to the#elsebranch (lines 108-114). SinceisDarkenGreyoutis now a float in both paths, the conditional compilation is unnecessary here.🔎 Remove duplication
float blendFactor = 0.65; // Controls how much to modify (0.0 = original, 1.0 = target color) - #ifdef USE_UNIFORM_BUFFERS - if (isDarkenGreyout > 0.0) { - // Darken the color - rgbColor = mix(rgbColor, vec3(0.2), blendFactor); - } else { - // Lighten the color - rgbColor = mix(rgbColor, max(backgroundColor.rgb, vec3(0.8)), blendFactor); - } - #else if (isDarkenGreyout > 0.0) { // Darken the color rgbColor = mix(rgbColor, vec3(0.2), blendFactor); } else { // Lighten the color rgbColor = mix(rgbColor, max(backgroundColor.rgb, vec3(0.8)), blendFactor); } - #endif
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (71)
package.jsonsrc/config.tssrc/graph/utils/error-message.tssrc/helper.tssrc/index.tssrc/modules/Clusters/calculate-centermass.fragsrc/modules/Clusters/calculate-centermass.vertsrc/modules/Clusters/force-cluster.fragsrc/modules/Clusters/index.tssrc/modules/ForceCenter/calculate-centermass.fragsrc/modules/ForceCenter/calculate-centermass.vertsrc/modules/ForceCenter/force-center.fragsrc/modules/ForceCenter/index.tssrc/modules/ForceGravity/force-gravity.fragsrc/modules/ForceGravity/index.tssrc/modules/ForceLink/force-spring.tssrc/modules/ForceLink/index.tssrc/modules/ForceManyBody/calculate-level.fragsrc/modules/ForceManyBody/calculate-level.vertsrc/modules/ForceManyBody/force-centermass.fragsrc/modules/ForceManyBody/force-level.fragsrc/modules/ForceManyBody/index.tssrc/modules/ForceManyBody/quadtree-frag-shader.tssrc/modules/ForceManyBodyQuadtree/index.tssrc/modules/ForceMouse/force-mouse.fragsrc/modules/ForceMouse/index.tssrc/modules/Lines/draw-curve-line.fragsrc/modules/Lines/draw-curve-line.vertsrc/modules/Lines/hovered-line-index.fragsrc/modules/Lines/hovered-line-index.vertsrc/modules/Lines/index.tssrc/modules/Points/drag-point.fragsrc/modules/Points/draw-highlighted.fragsrc/modules/Points/draw-highlighted.vertsrc/modules/Points/draw-points.fragsrc/modules/Points/draw-points.vertsrc/modules/Points/fill-sampled-points.fragsrc/modules/Points/fill-sampled-points.vertsrc/modules/Points/find-hovered-point.fragsrc/modules/Points/find-hovered-point.vertsrc/modules/Points/find-points-on-area-selection.fragsrc/modules/Points/find-points-on-polygon-selection.fragsrc/modules/Points/index.tssrc/modules/Points/track-positions.fragsrc/modules/Points/update-position.fragsrc/modules/Shared/buffer.tssrc/modules/Shared/clear.fragsrc/modules/Shared/quad.vertsrc/modules/Store/index.tssrc/modules/core-module.tssrc/stories/2. configuration.mdxsrc/stories/beginners.stories.tssrc/stories/beginners/basic-set-up/index.tssrc/stories/beginners/link-hovering/index.tssrc/stories/beginners/point-labels/index.tssrc/stories/beginners/quick-start.tssrc/stories/beginners/remove-points/index.tssrc/stories/clusters/polygon-selection/index.tssrc/stories/clusters/radial.tssrc/stories/clusters/with-labels.tssrc/stories/clusters/worm.tssrc/stories/create-cosmos.tssrc/stories/experiments/full-mesh.tssrc/stories/experiments/mesh-with-holes.tssrc/stories/geospatial/moscow-metro-stations/index.tssrc/stories/shapes/all-shapes/index.tssrc/stories/shapes/image-example/index.tssrc/stories/test-luma-migration.tssrc/stories/test.stories.tssrc/variables.tsvite.config.ts
💤 Files with no reviewable changes (1)
- vite.config.ts
🧰 Additional context used
🧬 Code graph analysis (24)
src/config.ts (2)
src/modules/Points/index.ts (3)
config(373-373)config(313-313)config(255-255)src/stories/clusters/polygon-selection/polygon.ts (1)
resizeCanvas(71-82)
src/stories/experiments/mesh-with-holes.ts (1)
src/stories/create-story.ts (1)
story(32-38)
src/stories/beginners/point-labels/index.ts (3)
src/index.ts (2)
Graph(28-1871)destroy(1264-1326)src/stories/beginners/point-labels/labels.ts (2)
update(14-45)CosmosLabels(4-46)src/stories/clusters/polygon-selection/polygon.ts (1)
destroy(63-69)
src/stories/beginners.stories.ts (3)
src/stories/beginners/point-labels/index.ts (1)
pointLabels(10-69)src/stories/create-story.ts (3)
story(32-38)error(38-41)storyFunction(15-51)src/stories/beginners/point-labels/labels.ts (1)
CosmosLabels(4-46)
src/stories/beginners/quick-start.ts (1)
src/index.ts (2)
Graph(28-1871)destroy(1264-1326)
src/stories/beginners/basic-set-up/index.ts (1)
src/index.ts (1)
destroy(1264-1326)
src/stories/experiments/full-mesh.ts (1)
src/index.ts (1)
Graph(28-1871)
src/stories/clusters/radial.ts (1)
src/index.ts (1)
Graph(28-1871)
src/stories/shapes/image-example/index.ts (1)
src/index.ts (1)
destroy(1264-1326)
src/stories/test.stories.ts (3)
src/stories/create-cosmos.ts (1)
CosmosStoryProps(3-16)src/stories/create-story.ts (2)
Story(5-5)createStory(7-51)src/stories/test-luma-migration.ts (1)
testLumaMigration(3-195)
src/stories/beginners/link-hovering/index.ts (1)
src/index.ts (1)
destroy(1264-1326)
src/stories/beginners/remove-points/index.ts (1)
src/index.ts (1)
destroy(1264-1326)
src/modules/Clusters/index.ts (2)
src/modules/core-module.ts (1)
CoreModule(7-28)src/modules/Shared/buffer.ts (1)
createIndexesForBuffer(13-23)
src/modules/ForceMouse/index.ts (1)
src/modules/core-module.ts (1)
CoreModule(7-28)
src/modules/ForceLink/index.ts (3)
src/modules/core-module.ts (1)
CoreModule(7-28)src/modules/ForceLink/force-spring.ts (1)
forceFrag(1-89)src/modules/ForceManyBodyQuadtree/quadtree-frag-shader.ts (1)
forceFrag(4-93)
src/stories/clusters/with-labels.ts (1)
src/stories/create-cosmos.ts (1)
createCosmos(18-72)
src/stories/test-luma-migration.ts (2)
src/index.ts (2)
Graph(28-1871)GraphConfigInterface(1873-1873)src/config.ts (1)
GraphConfigInterface(20-626)
src/modules/ForceManyBody/index.ts (4)
src/modules/core-module.ts (1)
CoreModule(7-28)src/modules/Shared/buffer.ts (1)
createIndexesForBuffer(13-23)src/modules/ForceManyBody/quadtree-frag-shader.ts (1)
forceFrag(1-89)src/modules/ForceManyBodyQuadtree/quadtree-frag-shader.ts (1)
forceFrag(4-93)
src/stories/geospatial/moscow-metro-stations/index.ts (1)
src/index.ts (2)
Graph(28-1871)destroy(1264-1326)
src/modules/ForceGravity/index.ts (3)
src/modules/core-module.ts (1)
CoreModule(7-28)src/modules/ForceLink/force-spring.ts (1)
forceFrag(1-89)src/modules/ForceManyBodyQuadtree/quadtree-frag-shader.ts (1)
forceFrag(4-93)
src/modules/ForceCenter/index.ts (2)
src/modules/core-module.ts (1)
CoreModule(7-28)src/modules/Shared/buffer.ts (1)
createIndexesForBuffer(13-23)
src/modules/Lines/index.ts (4)
src/modules/core-module.ts (1)
CoreModule(7-28)src/modules/GraphData/index.ts (1)
linksNumber(73-75)src/variables.ts (1)
defaultConfigValues(12-64)src/modules/Lines/geometry.ts (1)
getCurveLineGeometry(4-18)
src/modules/core-module.ts (2)
src/index.ts (1)
GraphConfigInterface(1873-1873)src/config.ts (1)
GraphConfigInterface(20-626)
src/stories/clusters/polygon-selection/index.ts (1)
src/stories/create-cosmos.ts (1)
createCosmos(18-72)
🪛 ast-grep (0.40.3)
src/stories/beginners.stories.ts
[warning] 92-92: Direct HTML content assignment detected. Modifying innerHTML, outerHTML, or using document.write with unsanitized content can lead to XSS vulnerabilities. Use secure alternatives like textContent or sanitize HTML with libraries like DOMPurify.
Context: div.innerHTML = '
Note: [CWE-79] Improper Neutralization of Input During Web Page Generation [REFERENCES]
- https://www.dhairyashah.dev/posts/why-innerhtml-is-a-bad-idea-and-how-to-avoid-it/
- https://cwe.mitre.org/data/definitions/79.html
(unsafe-html-content-assignment)
[warning] 92-92: Direct modification of innerHTML or outerHTML properties detected. Modifying these properties with unsanitized user input can lead to XSS vulnerabilities. Use safe alternatives or sanitize content first.
Context: div.innerHTML = '
Note: [CWE-79] Improper Neutralization of Input During Web Page Generation [REFERENCES]
- https://owasp.org/www-community/xss-filter-evasion-cheatsheet
- https://cwe.mitre.org/data/definitions/79.html
(dom-content-modification)
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (5)
src/modules/ForceLink/index.ts (1)
101-154: IncorrectbytesPerRowincopyImageDatacalls — must be bytes, not pixels.For
rgba32floatformat (4 floats × 4 bytes = 16 bytes per pixel),bytesPerRowshould bepointsTextureSize * 16at line 103 andlinksTextureSize * 16at lines 136, 143, and 150.🔎 Proposed fix
this.linkFirstIndicesAndAmountTexture!.copyImageData({ data: this.linkFirstIndicesAndAmount, - bytesPerRow: pointsTextureSize, + bytesPerRow: pointsTextureSize * 16, mipLevel: 0, x: 0, y: 0, })Apply similar changes to all
copyImageDatacalls in this file.src/modules/ForceCenter/index.ts (1)
51-57: IncorrectbytesPerRowfor 1×1 texture.For
rgba32floatformat,bytesPerRowshould be1 * 16(16 bytes) rather than1.🔎 Proposed fix
this.centermassTexture.copyImageData({ data: new Float32Array(4).fill(0), - bytesPerRow: 1, + bytesPerRow: 16, mipLevel: 0, x: 0, y: 0, })src/modules/Clusters/index.ts (1)
109-227: IncorrectbytesPerRowvalues in allcopyImageDatacalls.Throughout
create(),bytesPerRowis set to the texture size in pixels rather than bytes. Forrgba32floatformat, multiply by 16 (4 floats × 4 bytes).🔎 Proposed fix pattern
this.clusterTexture.copyImageData({ data: clusterState, - bytesPerRow: pointsTextureSize, + bytesPerRow: pointsTextureSize * 16, mipLevel: 0, x: 0, y: 0, })Apply similar changes to all
copyImageDatacalls in this file (lines 111, 120, 142, 151, 173, 182, 208, 223).src/index.ts (1)
673-680: Hardcoded clear color inconsistent with background color.The clear color is hardcoded to
[0, 0, 1, 1](blue) instead of usingthis.store.backgroundColor. This appears to be debug code left in place.🔎 Proposed fix
if (this.device) { const clearPass = this.device.beginRenderPass({ - clearColor: [0, 0, 1, 1], // this.store.backgroundColor, + clearColor: this.store.backgroundColor, clearDepth: 1, clearStencil: 0, })src/modules/ForceManyBody/index.ts (1)
133-147: Random values texture recreation check may have issues.The condition
!this.randomValuesTexture || this.randomValuesTexture.destroyedcreates a new texture, but if thepointsTextureSizechanges, the existing texture won't be resized. The texture size should matchstore.pointsTextureSize.🔎 Proposed fix
- if (!this.randomValuesTexture || this.randomValuesTexture.destroyed) { + const needsNewRandomTexture = !this.randomValuesTexture || + this.randomValuesTexture.destroyed || + this.randomValuesTexture.width !== store.pointsTextureSize || + this.randomValuesTexture.height !== store.pointsTextureSize + + if (needsNewRandomTexture) { + if (this.randomValuesTexture && !this.randomValuesTexture.destroyed) { + this.randomValuesTexture.destroy() + } this.randomValuesTexture = device.createTexture({
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
src/index.tssrc/modules/Clusters/index.tssrc/modules/ForceCenter/index.tssrc/modules/ForceGravity/index.tssrc/modules/ForceLink/index.tssrc/modules/ForceManyBody/index.tssrc/modules/ForceMouse/index.tssrc/modules/Lines/index.tssrc/modules/Points/index.ts
🧰 Additional context used
🧬 Code graph analysis (8)
src/modules/ForceGravity/index.ts (1)
src/modules/core-module.ts (1)
CoreModule(7-28)
src/modules/ForceLink/index.ts (2)
src/modules/core-module.ts (1)
CoreModule(7-28)src/modules/ForceLink/force-spring.ts (1)
forceFrag(1-89)
src/index.ts (2)
src/helper.ts (1)
readPixels(75-84)src/modules/core-module.ts (1)
CoreModule(7-28)
src/modules/Clusters/index.ts (2)
src/modules/core-module.ts (1)
CoreModule(7-28)src/modules/Shared/buffer.ts (1)
createIndexesForBuffer(13-23)
src/modules/ForceCenter/index.ts (2)
src/modules/core-module.ts (1)
CoreModule(7-28)src/modules/Shared/buffer.ts (1)
createIndexesForBuffer(13-23)
src/modules/ForceManyBody/index.ts (4)
src/modules/core-module.ts (1)
CoreModule(7-28)src/modules/Shared/buffer.ts (1)
createIndexesForBuffer(13-23)src/modules/ForceManyBody/quadtree-frag-shader.ts (1)
forceFrag(1-89)src/modules/ForceManyBodyQuadtree/quadtree-frag-shader.ts (1)
forceFrag(4-93)
src/modules/Lines/index.ts (4)
src/modules/core-module.ts (1)
CoreModule(7-28)src/modules/GraphData/index.ts (1)
linksNumber(73-75)src/variables.ts (1)
defaultConfigValues(12-64)src/modules/Lines/geometry.ts (1)
getCurveLineGeometry(4-18)
src/modules/ForceMouse/index.ts (1)
src/modules/core-module.ts (1)
CoreModule(7-28)
🔇 Additional comments (16)
src/modules/ForceGravity/index.ts (1)
1-119: LGTM!The migration to luma.gl is well-implemented with proper resource lifecycle management. The destruction order is correctly documented and followed, and the optional
RenderPassparameter provides good flexibility for batched rendering.src/modules/ForceLink/index.ts (1)
271-310: LGTM!The
destroy()method correctly follows the documented cleanup order and properly checks for destroyed resources before calling destroy.src/modules/ForceCenter/index.ts (1)
81-200: LGTM!The
initPrograms()method properly sets up both uniform stores and all three Models (clear, calculate centermass, run) with correct bindings, buffer layouts, and parameters.src/modules/ForceMouse/index.ts (1)
1-117: LGTM!Clean migration following the established pattern. The module properly manages resources and the
destroy()method correctly follows the documented cleanup order.src/modules/Clusters/index.ts (1)
479-535: LGTM!The
destroy()method is comprehensive and correctly follows the documented cleanup order. All resources (Models, Framebuffers, Textures, UniformStores, Buffers) are properly cleaned up with destroyed checks.src/index.ts (2)
1413-1445: LGTM!The
ensureDevice()pattern andcreateDevice()implementation are well-designed. The deferred execution via Promise ensures methods called before device initialization complete successfully.
1301-1322: LGTM!The
destroy()method properly cleans up all module resources before destroying the device, maintaining the correct cleanup order. The final clear pass before device destruction is a nice touch.src/modules/Lines/index.ts (1)
662-732: LGTM!The
destroy()method is now properly implemented, addressing the previous review concern. It correctly follows the documented cleanup order (Models → Framebuffers → Textures → UniformStores → Buffers) and checks for destroyed resources before cleanup.src/modules/ForceManyBody/index.ts (8)
1-15: LGTM!The imports and type definitions are well-structured for the luma.gl migration. The
LevelTargettype appropriately encapsulates per-level texture and framebuffer resources.
17-60: LGTM!The class properties are well-organized and properly typed for the luma.gl migration. The use of
UniformStorefor shader uniforms and tracking of previous sizes for change detection follows best practices.
269-273: Redundant buffer creation is guarded correctly.The check
if (!this.forceVertexCoordBuffer)at line 269 ensures the buffer is only created once, even though similar code appears at lines 326-330. This is intentional since bothforceCommandandforceFromItsOwnCentermassCommandshare the same buffer.
366-373: LGTM!The change detection logic prevents rendering with mismatched resources, and the optional
renderPassparameter enables flexible integration with external render passes.
379-431: LGTM!The destruction order is correct and follows luma.gl best practices. The defensive checks for null/undefined and destroyed state prevent errors, and the comprehensive cleanup ensures no resource leaks.
433-469: LGTM!The level drawing logic properly validates resources, updates per-level uniforms, and manages render passes. The clear-then-calculate pattern correctly builds the quadtree hierarchy.
471-540: LGTM!The force drawing logic correctly handles both internal and external render passes, applies per-level forces, and uses the centermass fallback only for the deepest level. The conditional
end()at line 537 properly respects external render pass ownership.
69-123: ThebytesPerRowcalculation is correct—no change needed.The
copyImageDatamethod in luma.gl expectsbytesPerRowto be in pixel counts, not bytes. This is confirmed by the explicit comment in Points/index.ts ("UNPACK_ROW_LENGTH and UNPACK_IMAGE_HEIGHT expect pixel counts (not bytes)"), and the pattern is consistently applied across the entire codebase for rgba32float textures. The current usage ofbytesPerRow: levelTextureSizeis correct.
There was a problem hiding this comment.
Actionable comments posted: 7
🤖 Fix all issues with AI agents
In @src/index.ts:
- Around line 1405-1423: The queued callback in ensureDevice uses
deviceInitPromise.then(...) without checking whether the instance was destroyed,
so callbacks may run after destroy(); update ensureDevice to, inside both the
then and catch handlers, check the instance _isDestroyed flag (or a similar
destroyed sentinel) before invoking callback or handling errors; ensure you
reference the same _isDestroyed field used by destroy() and bail out early if
true to avoid running callbacks post-destruction.
- Around line 1262-1263: The destroy() method can race with pending
deviceInitPromise; set this._isDestroyed = true at the start of destroy() and
then await the pending deviceInitPromise (if any) before tearing down resources,
or cancel it if a cancel token is supported; also add an early-exit check for
this._isDestroyed inside ensureDevice() (and any callbacks chained off
deviceInitPromise) so that if initialization completes after destruction the
code returns immediately and avoids further actions on a destroyed instance.
In @src/modules/Clusters/index.ts:
- Around line 402-439: The centermass pass uses optional chaining for
clearCentermassCommand which can silently skip clearing and corrupt
accumulation; add an explicit guard before starting the render pass (similar to
the existing guards for calculateCentermassCommand/UniformStore) that returns
early or logs/throws if clearCentermassCommand is missing/undefined, and replace
the optional call clearCentermassCommand?.draw with a normal
clearCentermassCommand.draw only after verifying clearCentermassCommand exists;
reference calculateCentermassCommand, calculateCentermassUniformStore,
clearCentermassCommand and centermassFbo when adding the guard so the centermass
texture is always cleared before calculateCentermassCommand.draw.
In @src/modules/ForceManyBody/index.ts:
- Line 231: In initPrograms(), avoid using non-null assertions on
this.pointIndices and this.randomValuesTexture (and on
points.previousPositionTexture) without verifying initialization: add an
explicit guard near the start of initPrograms() that checks these are non-null
(e.g. if (!this.pointIndices || !this.randomValuesTexture ||
!points.previousPositionTexture) return) so the subsequent uses (including the
lines that currently do pointIndices: this.pointIndices!, randomValuesTexture:
this.randomValuesTexture!, and references to points.previousPositionTexture!)
are safe and don’t rely solely on create() having been called.
- Around line 79-87: The bytesPerRow used in
existingTarget.texture.copyImageData is incorrect and the workaround comment is
misleading; change bytesPerRow from levelTextureSize to the correct byte-aligned
value for an rgba32float texture (levelTextureSize * 16) and remove or update
the misleading workaround comment, ensuring the call to copyImageData uses data
= new Float32Array(levelTextureSize * levelTextureSize * 4).fill(0), bytesPerRow
= levelTextureSize * 16, mipLevel = 0, x = 0, y = 0.
- Line 66: Guard against non-positive space sizes before computing levels: check
store.adjustedSpaceSize > 0 and handle invalid values (either return early,
throw a clear error, or set this.levels = 0) before the line that sets
this.levels = Math.log2(store.adjustedSpaceSize); this prevents Math.log2 from
producing -Infinity/NaN and breaking the subsequent loop that relies on
this.levels (refer to this.levels and store.adjustedSpaceSize in
src/modules/ForceManyBody/index.ts).
In @src/stories/clusters/with-labels.ts:
- Line 6: The function withLabels is declared async but contains no await,
causing unnecessary loading UI; remove the async keyword from the export
declaration (export const withLabels = (): Promise<{div: HTMLDivElement; graph:
Graph; destroy: () => void }> => { ... }) or, better, adjust its return type to
a plain object instead of a Promise if it truly returns synchronously, ensure no
callers expect a Promise, and update any type annotations or usages referencing
withLabels accordingly (look for withLabels declaration and any imports/usages).
🧹 Nitpick comments (5)
src/stories/beginners.stories.ts (1)
80-96: Excellent error handling and resource management.The refactored render function properly:
- Returns an HTMLDivElement with explicit typing
- Wraps story generation in try-catch for robustness
- Wires up both
args.graphandargs.destroyfor proper teardown- Displays a user-friendly error message on failure
This aligns well with the PR's migration to explicit resource cleanup patterns.
♻️ Optional: Use DOM APIs instead of innerHTML to satisfy static analysis
While the static hardcoded string poses no XSS risk, using DOM APIs would avoid the linter warning and follow defensive coding practices:
} catch (error) { console.error('Failed to load PointLabels story:', error) - div.innerHTML = '<div style="display: flex; align-items: center; justify-content: center; height: 100%; color: #ff0000;">Failed to load story</div>' + const errorDiv = document.createElement('div') + errorDiv.style.display = 'flex' + errorDiv.style.alignItems = 'center' + errorDiv.style.justifyContent = 'center' + errorDiv.style.height = '100%' + errorDiv.style.color = '#ff0000' + errorDiv.textContent = 'Failed to load story' + div.appendChild(errorDiv) }src/modules/ForceLink/index.ts (1)
101-109: Upgrade luma.gl to 9.2.4 or later to remove this workaround.The
bytesPerRowbug in luma.gl 9.2.3 was fixed in v9.2.4. The project currently uses 9.2.3, so the workaround is still needed. Since package.json specifies^9.2.3, upgrading to 9.2.4+ is compatible and would allow removing this workaround and using the correct value (pointsTextureSize * 16).src/modules/ForceCenter/index.ts (1)
90-99: Consider sharing the fullscreen quad buffer.Both
clearVertexCoordBufferandforceVertexCoordBufferare initialized with identical data for a fullscreen quad. This duplication increases memory usage slightly.♻️ Refactor to use a single shared buffer
You could create a single
fullscreenQuadBufferand use it for both commands:- if (!this.clearVertexCoordBuffer || this.clearVertexCoordBuffer.destroyed) { - this.clearVertexCoordBuffer = device.createBuffer({ + if (!this.fullscreenQuadBuffer || this.fullscreenQuadBuffer.destroyed) { + this.fullscreenQuadBuffer = device.createBuffer({ data: new Float32Array([-1, -1, 1, -1, -1, 1, 1, 1]), }) } - if (!this.forceVertexCoordBuffer || this.forceVertexCoordBuffer.destroyed) { - this.forceVertexCoordBuffer = device.createBuffer({ - data: new Float32Array([-1, -1, 1, -1, -1, 1, 1, 1]), - }) - }Then reference the same buffer in both Model attributes.
src/modules/Clusters/index.ts (1)
269-374: Consider sharing fullscreen quad buffers.Similar to ForceCenter, both
clearCentermassVertexCoordBuffer(line 272) andapplyForcesVertexCoordBuffer(line 370) contain identical fullscreen quad data. This pattern appears across multiple modules.Consider creating a shared fullscreen quad buffer in the CoreModule base class or a utility module to reduce memory usage and code duplication across modules (ForceCenter, Clusters, etc.).
src/modules/Lines/index.ts (1)
628-643: Redundant clear pass for link index framebuffer.Lines 629-633 create a render pass solely to clear the
linkIndexFbo, then immediately end it. Lines 636-643 create another render pass for actual rendering withclearColor: [0, 0, 0, 0].The explicit clear pass appears unnecessary since the rendering pass already specifies a clear color. This creates an extra GPU command buffer submission.
♻️ Remove redundant clear pass
- // Clear and render to index buffer - const clearPass = this.device.beginRenderPass({ - framebuffer: this.linkIndexFbo, - clearColor: [0, 0, 0, 0], - }) - clearPass.end() - // Render to index buffer for picking/hover detection const indexPass = this.device.beginRenderPass({ framebuffer: this.linkIndexFbo, - // Note: We explicitly set clearColor to [0, 0, 0, 0] to prevent re-clearing the framebuffer. - // Without this, luma.gl would use its default clearColor of [0, 0, 0, 1] (opaque black) - clearColor: [0, 0, 0, 0], // or `clearColor: false` + clearColor: [0, 0, 0, 0], }) this.drawCurveCommand.draw(indexPass) indexPass.end()
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (25)
package.jsonsrc/config.tssrc/index.tssrc/modules/Clusters/index.tssrc/modules/ForceCenter/index.tssrc/modules/ForceLink/index.tssrc/modules/ForceManyBody/index.tssrc/modules/ForceManyBody/quadtree-frag-shader.tssrc/modules/ForceManyBodyQuadtree/calculate-level.fragsrc/modules/ForceManyBodyQuadtree/calculate-level.vertsrc/modules/ForceManyBodyQuadtree/index.tssrc/modules/ForceManyBodyQuadtree/quadtree-frag-shader.tssrc/modules/Lines/hovered-line-index.fragsrc/modules/Lines/hovered-line-index.vertsrc/modules/Lines/index.tssrc/modules/Points/index.tssrc/modules/Shared/buffer.tssrc/stories/2. configuration.mdxsrc/stories/beginners.stories.tssrc/stories/clusters/with-labels.tssrc/stories/clusters/worm.tssrc/stories/shapes/image-example/index.tssrc/variables.tstsconfig.jsonvite.config.ts
💤 Files with no reviewable changes (6)
- src/modules/ForceManyBodyQuadtree/index.ts
- src/modules/ForceManyBodyQuadtree/quadtree-frag-shader.ts
- src/modules/ForceManyBody/quadtree-frag-shader.ts
- src/modules/Shared/buffer.ts
- src/modules/ForceManyBodyQuadtree/calculate-level.frag
- src/modules/ForceManyBodyQuadtree/calculate-level.vert
✅ Files skipped from review due to trivial changes (1)
- tsconfig.json
🚧 Files skipped from review as they are similar to previous changes (4)
- src/stories/clusters/worm.ts
- src/modules/Lines/hovered-line-index.frag
- package.json
- src/variables.ts
🧰 Additional context used
🧬 Code graph analysis (9)
src/stories/clusters/with-labels.ts (1)
src/stories/create-cosmos.ts (1)
createCosmos(18-72)
src/modules/ForceLink/index.ts (2)
src/modules/core-module.ts (1)
CoreModule(7-28)src/modules/ForceLink/force-spring.ts (1)
forceFrag(1-89)
src/modules/Clusters/index.ts (3)
src/modules/core-module.ts (1)
CoreModule(7-28)src/modules/Shared/buffer.ts (1)
createIndexesForBuffer(1-11)src/modules/ForceLink/force-spring.ts (1)
forceFrag(1-89)
src/modules/ForceCenter/index.ts (2)
src/modules/core-module.ts (1)
CoreModule(7-28)src/modules/Shared/buffer.ts (1)
createIndexesForBuffer(1-11)
src/modules/Lines/index.ts (3)
src/modules/core-module.ts (1)
CoreModule(7-28)src/modules/GraphData/index.ts (1)
linksNumber(73-75)src/modules/Lines/geometry.ts (1)
getCurveLineGeometry(4-18)
src/stories/beginners.stories.ts (1)
src/stories/beginners/point-labels/index.ts (1)
pointLabels(10-69)
src/stories/shapes/image-example/index.ts (1)
src/index.ts (2)
Graph(27-1881)destroy(1262-1336)
src/modules/ForceManyBody/index.ts (3)
src/modules/core-module.ts (1)
CoreModule(7-28)src/modules/Shared/buffer.ts (1)
createIndexesForBuffer(1-11)src/modules/ForceLink/force-spring.ts (1)
forceFrag(1-89)
src/index.ts (2)
src/config.ts (1)
GraphConfigInterface(20-612)src/helper.ts (2)
getRgbaColor(16-32)readPixels(75-84)
🪛 ast-grep (0.40.4)
src/stories/beginners.stories.ts
[warning] 91-91: Direct modification of innerHTML or outerHTML properties detected. Modifying these properties with unsanitized user input can lead to XSS vulnerabilities. Use safe alternatives or sanitize content first.
Context: div.innerHTML = '
Note: [CWE-79] Improper Neutralization of Input During Web Page Generation [REFERENCES]
- https://owasp.org/www-community/xss-filter-evasion-cheatsheet
- https://cwe.mitre.org/data/definitions/79.html
(dom-content-modification)
[warning] 91-91: Direct HTML content assignment detected. Modifying innerHTML, outerHTML, or using document.write with unsanitized content can lead to XSS vulnerabilities. Use secure alternatives like textContent or sanitize HTML with libraries like DOMPurify.
Context: div.innerHTML = '
Note: [CWE-79] Improper Neutralization of Input During Web Page Generation [REFERENCES]
- https://www.dhairyashah.dev/posts/why-innerhtml-is-a-bad-idea-and-how-to-avoid-it/
- https://cwe.mitre.org/data/definitions/79.html
(unsafe-html-content-assignment)
🔇 Additional comments (38)
src/stories/beginners.stories.ts (1)
75-79: LGTM! Cleanup pattern improved.The change from
d.args.graph?.destroy()tod.args.destroy?.()correctly centralizes teardown via the destroy callback, aligning with the new resource management pattern introduced in this PR.src/stories/clusters/with-labels.ts (1)
10-10: LGTM! Proper resource cleanup pattern.The changes correctly implement the destroy lifecycle:
- Extracting
baseDestroyfromcreateCosmosavoids naming collision- The local
destroyfunction properly cleans up the interval timer before delegating tobaseDestroy- Optional chaining safely handles the case where
baseDestroyis undefinedThis pattern ensures proper cleanup of both local resources (interval) and base graph resources (WebGL contexts, buffers, etc.).
Also applies to: 48-51
src/modules/ForceManyBody/index.ts (8)
1-15: LGTM! Clean imports and type definition.The luma.gl imports are appropriate for the migration, and the
LevelTargettype provides a clear abstraction for managing per-level texture/FBO pairs.
17-60: Well-structured property declarations.The use of optional types, typed
UniformStoredeclarations, and state tracking properties provides clear contracts and type safety for the module's resources.
73-95: Excellent resource reuse and cleanup patterns.The logic to reuse existing resources when dimensions match (lines 73-88) and properly destroy resources before recreation (lines 91-95, 144-146, 168) demonstrates good GPU resource management that minimizes allocation overhead.
Also applies to: 137-153, 167-175
186-204: Consistent model setup with proper blend configuration.The model initialization follows a consistent pattern with appropriate topology, vertex counts, uniform stores, and blend parameters for additive force accumulation.
Also applies to: 207-255, 258-320, 323-378
381-388: Good defensive programming with size mismatch check.The early return when texture or space sizes have changed (lines 383-385) prevents rendering with stale or mismatched resources, ensuring consistency between
create()andrun()calls.
394-446: Exemplary resource cleanup with documented destruction order.The structured destruction sequence (Models → Framebuffers → Textures → UniformStores → Buffers) with clear comments explaining ownership semantics (lines 390-393, 425, 433) is excellent. The guards against double-destruction and explicit cleanup of non-owned buffers demonstrate thorough resource management.
448-484: LGTM! Proper per-level rendering with appropriate guards.The level drawing logic correctly guards against null/destroyed resources, updates uniforms per level, and manages per-level render passes with proper cleanup.
486-555: Well-designed render pass management with external/internal handling.The method intelligently handles both external and internal render passes (lines 504-506), only ending the pass if it was created internally (lines 552-554). The deepest level's centermass fallback (lines 534-549) is properly isolated, and the separation of shared vs per-level bindings is clean.
vite.config.ts (1)
1-1: LGTM - Build configuration properly updated for luma.gl migration.The removal of the
vite-plugin-glslandreglexternal dependency aligns with the migration to luma.gl's Model-based shader workflow where shaders are imported as raw strings.Also applies to: 19-19, 36-38
src/stories/2. configuration.mdx (1)
47-47: Documentation properly updated to reflect config changes.The updated descriptions for
pixelRatioandsimulationRepulsionThetaaccurately reflect the implementation changes insrc/config.ts.Also applies to: 78-78
src/config.ts (1)
510-512: LGTM - Documentation accurately describes pixelRatio behavior.The updated description clearly explains the GPU memory vs. quality trade-off and documents the dynamic default value.
src/stories/shapes/image-example/index.ts (3)
59-59: Function signature change from async to sync is a breaking change.The function now returns synchronously with an optional
destroycallback. This is a better pattern for lifecycle management, but callers that previously awaited this function will need to be updated.
207-225: Good pattern for guarding async callbacks against use-after-destroy.The
isDestroyedflag properly prevents the async image loading callback from interacting with a destroyed graph instance. This aligns with the lifecycle patterns established insrc/index.ts.
242-247: Destroy implementation correctly sets guard flag before calling graph.destroy().Setting
isDestroyed = truebeforegraph.destroy()ensures any pending async callbacks will be properly guarded.src/modules/ForceLink/index.ts (3)
1-5: LGTM - Proper luma.gl imports for the Model-based pipeline.The imports correctly include all necessary primitives for GPU resource management and uniform buffer handling.
232-277: Run method properly handles optional RenderPass parameter.The implementation correctly:
- Validates all required resources before proceeding
- Updates uniforms per-frame
- Creates/uses render pass appropriately
- Only ends the pass if it was internally created
279-318: Well-documented resource destruction order.The destruction sequence (Models → Framebuffers → Textures → UniformStores → Buffers) prevents use-after-free issues. Each resource is checked for existence and destroyed state before cleanup.
src/index.ts (5)
6-8: LGTM - Correct luma.gl imports for device-based rendering.The imports include
Device,Framebuffer,lumafrom core andWebGLDevice,webgl2Adapterfrom the WebGL adapter, which are the required primitives for the migration.
100-217: Device initialization uses fire-and-forget pattern with proper error handling.The async device creation is well-structured:
- Initializes all dependent components after device is ready
- Sets up event listeners and input handling
- Configures simulation modules only when
enableSimulationis true- Error handling logs and rethrows failures
1488-1495: Velocity buffer cleared once per frame before applying forces.This ensures forces accumulate correctly from a zero baseline each simulation step. The guard checks for buffer existence and destroyed state.
1593-1626: Render pass properly created, used, and submitted.The rendering flow correctly:
- Creates a single render pass with clear operations
- Conditionally draws links based on configuration
- Draws points
- Handles drag updates within the pass
- Ends the pass and submits to the device
1299-1320: Module destruction occurs before device destruction.The destruction order correctly destroys all module resources (points, lines, clusters, forces) before destroying the device, preventing use-after-free of GPU resources.
src/modules/Lines/hovered-line-index.vert (1)
1-10: LGTM - Clean GLSL ES 3.0 migration.The shader correctly:
- Declares GLSL ES 3.0 version
- Uses
ininstead of deprecatedattribute- Removes unused
vTexCoordvarying- Simplifies to a straightforward vertex position pass-through
src/modules/ForceCenter/index.ts (4)
1-36: LGTM: Clean migration to luma.gl types.The import statements and property declarations correctly align with the luma.gl API. The typed UniformStore definitions provide good type safety for shader uniforms.
204-254: LGTM: Well-structured two-stage rendering pipeline.The
run()method correctly implements a two-stage pipeline:
- Clear and accumulate centermass into a dedicated framebuffer
- Apply center force to velocity using the computed centermass
The optional
renderPassparameter allows flexible composition with external rendering workflows, and the guards ensure resources are valid before use.
256-302: LGTM: Proper resource cleanup order.The
destroy()method follows the correct resource destruction order with appropriate guards for already-destroyed resources. The explicit documentation and systematic cleanup ensure no resource leaks.
51-59: Verify the luma.gl 9.2.3 bytesPerRow bug claim and fix status.The codebase contains a pervasive workaround (44+ instances across ForceCenter, Lines, Points, ForceManyBody, ForceLink, and Clusters) for a reported luma.gl 9.2.3 bug where
bytesPerRowincopyImageDataincorrectly expects pixels instead of bytes. Without access to the original issue or PR that introduced this workaround, verify whether this bug was documented, confirm its status in the current or newer luma.gl versions, and determine if the workaround remains necessary.src/modules/Clusters/index.ts (3)
1-68: LGTM: Comprehensive size-change tracking.The property declarations and size-change detection logic properly handle the multiple texture size dependencies (pointsTextureSize, clustersTextureSize, clusterCount). This ensures textures are recreated only when necessary.
69-261: LGTM: Robust texture lifecycle management.The texture creation/update logic correctly handles size changes by:
- Destroying the framebuffer before the texture (correct order)
- Recreating textures only when sizes change
- Updating data in place when sizes haven't changed
This approach minimizes GPU resource churn.
495-551: LGTM: Consistent resource cleanup.The destruction order and guards mirror the ForceCenter implementation, ensuring all resources are properly released.
src/modules/Lines/index.ts (6)
1-99: LGTM: Comprehensive uniform type definitions.The UniformStore type definitions provide excellent type safety for the extensive set of shader uniforms. The 1x1 hoveredLineIndexTexture for single-pixel readback is appropriate for hover detection.
100-298: LGTM: Proper instanced rendering setup.The buffer initialization ensures all attributes exist before Model creation, and the
stepMode: 'instance'configuration correctly sets up per-link attributes for instanced rendering. This allows efficient rendering of multiple link instances with a single draw call.
300-350: LGTM: Clean separation of rendering modes.The
draw()method correctly handles normal rendering (renderMode: 0.0) with dynamic uniform updates and lazy buffer initialization. The requiredrenderPassparameter enforces proper integration with the main rendering pipeline.
352-405: LGTM: Efficient framebuffer resize handling.The method correctly handles screen size changes by recreating the framebuffer and texture only when necessary, with appropriate guards against zero-sized resources.
407-583: LGTM: Consistent buffer resize pattern.All update methods follow a consistent and correct pattern for handling buffer resizing:
- Check if the buffer size matches the data size
- Recreate the buffer if sizes differ (buffers are immutable in size)
- Update the buffer data efficiently if sizes match
The geometry update also correctly updates the Model's vertex count.
666-736: LGTM: Thorough resource cleanup.The
destroy()method correctly cleans up all resources in the proper order with appropriate guards. The extensive buffer cleanup ensures no memory leaks from attribute buffers.
ibgreen
left a comment
ibgreen
left a comment
There was a problem hiding this comment.
- Very excited to see this port. Looks like it was relatively straightforward.
- Of course as a framework provider, it would be great to get feedback on any friction points (bugs, bad docs, unintuitive APIs etc), even if just a short bulleted list.
And of course, a nice side effect is that you now have done about 70-80% of the work of porting cosmos.gl to WebGPU (you still need another set of WGSL shaders, obviously).
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In @src/modules/ForceLink/index.ts:
- Around line 102-108: The code uses a non-null assertion on
this.linkFirstIndicesAndAmountTexture before calling copyImageData, which can
throw if texture creation failed; modify the code to defensively check that
this.linkFirstIndicesAndAmountTexture (the result of device.createTexture) is
defined before calling copyImageData on it, and handle the failure path (e.g.,
log an error via your logger, throw a clear exception, or early-return) so
copyImageData is only invoked when the texture exists; reference the texture
variable this.linkFirstIndicesAndAmountTexture, the createTexture call, and the
copyImageData invocation when making the change.
In @src/modules/Lines/index.ts:
- Around line 404-469: The updatePointsBuffer (and similar
updateColor/updateWidth/updateArrow) logic recreates GPU buffers (pointABuffer,
pointBBuffer, linkIndexBuffer, colorBuffer, widthBuffer, arrowBuffer) but leaves
the drawCurveCommand Model bound to the old destroyed buffers; after any buffer
recreation you must update the Model attributes to reference the new buffers
(use Model.setAttributes() or recreate the Model) so the drawCurveCommand points
to the newly created buffers; locate buffer-recreation branches in
updatePointsBuffer and the other update* methods and call
drawCurveCommand.setAttributes({ a_pointA: {buffer: this.pointABuffer},
a_pointB: {buffer: this.pointBBuffer}, a_linkIndex: {buffer:
this.linkIndexBuffer}, ... }) (include color/width/arrow attributes) or fully
recreate drawCurveCommand immediately after buffer recreation.
🧹 Nitpick comments (4)
src/modules/Clusters/index.ts (2)
98-188: Consider extracting repeated texture recreation logic.The texture recreation pattern (check size change → destroy → create → copy data) is repeated for
clusterTexture,clusterPositionsTexture, andclusterForceCoefficientTexturewith nearly identical logic. This could be refactored into a helper method to reduce duplication and improve maintainability.♻️ Example helper method
private recreateTextureIfNeeded( texture: Texture | undefined, data: Float32Array, width: number, height: number, shouldRecreate: boolean ): Texture { if (!texture || shouldRecreate) { if (texture && !texture.destroyed) { texture.destroy() } texture = this.device.createTexture({ width, height, format: 'rgba32float', usage: Texture.SAMPLE | Texture.RENDER | Texture.COPY_DST, }) } texture.copyImageData({ data, bytesPerRow: getBytesPerRow('rgba32float', width), mipLevel: 0, x: 0, y: 0, }) return texture }
256-260: Duplicate fullscreen quad buffer creation.The same fullscreen quad coordinates
[-1, -1, 1, -1, -1, 1, 1, 1]are created multiple times within this module and across other modules (ForceLink, ForceCenter). Consider creating a shared constant or utility function.♻️ Proposed shared utility
In
src/modules/Shared/buffer.ts, add:export const FULLSCREEN_QUAD_COORDS = new Float32Array([-1, -1, 1, -1, -1, 1, 1, 1]) export function createFullscreenQuadBuffer(device: Device): Buffer { return device.createBuffer({ data: FULLSCREEN_QUAD_COORDS, }) }Then use it across modules:
- this.clearCentermassVertexCoordBuffer = device.createBuffer({ - data: new Float32Array([-1, -1, 1, -1, -1, 1, 1, 1]), - }) + this.clearCentermassVertexCoordBuffer = createFullscreenQuadBuffer(device)Also applies to: 354-358
src/modules/Lines/index.ts (2)
625-640: Optimize: Combine clear and render into single passThe code creates two separate render passes: one to clear (lines 626-630) and another to render (lines 633-640). This is inefficient and creates unnecessary overhead.
♻️ Proposed optimization
Combine into a single pass by setting
clearColorin the render pass:- // Clear and render to index buffer - const clearPass = this.device.beginRenderPass({ - framebuffer: this.linkIndexFbo, - clearColor: [0, 0, 0, 0], - }) - clearPass.end() - // Render to index buffer for picking/hover detection const indexPass = this.device.beginRenderPass({ framebuffer: this.linkIndexFbo, - // Note: We explicitly set clearColor to [0, 0, 0, 0] to prevent re-clearing the framebuffer. - // Without this, luma.gl would use its default clearColor of [0, 0, 0, 1] (opaque black) - clearColor: [0, 0, 0, 0], // or `clearColor: false` + clearColor: [0, 0, 0, 0], // Clear to transparent before rendering }) this.drawCurveCommand.draw(indexPass) indexPass.end()
471-554: Consider: Extract common buffer update patternThe methods
updateColor,updateWidth, andupdateArrowfollow nearly identical logic:
- Prepare data
- Check buffer existence
- Check size match
- Recreate or write
♻️ Possible refactoring to reduce duplication
Extract a common helper method:
private updateBuffer( buffer: Buffer | undefined, data: Float32Array, elementsPerItem: number ): Buffer { const linksNumber = this.data.linksNumber ?? 0 if (!buffer) { return this.device.createBuffer({ data, usage: Buffer.VERTEX | Buffer.COPY_DST, }) } const currentSize = buffer.byteLength / (Float32Array.BYTES_PER_ELEMENT * elementsPerItem) if (currentSize !== linksNumber) { if (!buffer.destroyed) buffer.destroy() return this.device.createBuffer({ data, usage: Buffer.VERTEX | Buffer.COPY_DST, }) } buffer.write(data) return buffer } public updateColor(): void { const linksNumber = this.data.linksNumber ?? 0 const colorData = this.data.linkColors ?? new Float32Array(linksNumber * 4).fill(0) this.colorBuffer = this.updateBuffer(this.colorBuffer, colorData, 4) }Apply similar refactoring to
updateWidthandupdateArrow.Note: This still has the critical issue of not updating Model attributes after buffer recreation.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (8)
package.jsonsrc/modules/Clusters/index.tssrc/modules/ForceCenter/index.tssrc/modules/ForceLink/index.tssrc/modules/ForceManyBody/index.tssrc/modules/Lines/index.tssrc/modules/Points/index.tssrc/modules/Shared/texture-utils.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- package.json
- src/modules/ForceManyBody/index.ts
🧰 Additional context used
🧬 Code graph analysis (4)
src/modules/ForceLink/index.ts (3)
src/modules/core-module.ts (1)
CoreModule(7-28)src/modules/Shared/texture-utils.ts (1)
getBytesPerRow(10-13)src/modules/ForceLink/force-spring.ts (1)
forceFrag(1-89)
src/modules/Clusters/index.ts (3)
src/modules/core-module.ts (1)
CoreModule(7-28)src/modules/Shared/texture-utils.ts (1)
getBytesPerRow(10-13)src/modules/Shared/buffer.ts (1)
createIndexesForBuffer(1-11)
src/modules/ForceCenter/index.ts (3)
src/modules/core-module.ts (1)
CoreModule(7-28)src/modules/Shared/texture-utils.ts (1)
getBytesPerRow(10-13)src/modules/Shared/buffer.ts (1)
createIndexesForBuffer(1-11)
src/modules/Lines/index.ts (4)
src/modules/core-module.ts (1)
CoreModule(7-28)src/modules/Shared/texture-utils.ts (1)
getBytesPerRow(10-13)src/modules/GraphData/index.ts (1)
linksNumber(73-75)src/modules/Lines/geometry.ts (1)
getCurveLineGeometry(4-18)
🔇 Additional comments (14)
src/modules/ForceLink/index.ts (3)
194-222: LGTM! Model configuration follows luma.gl best practices.The Model initialization properly sets up:
- Shader programs with uniform buffer support
- Buffer layout for attributes
- Bindings for uniform buffers and textures
- Appropriate render parameters
225-270: Well-structured run method with proper validation.The implementation includes:
- Comprehensive resource validation
- Size change detection to prevent rendering with mismatched resources
- Proper uniform and binding updates before drawing
- Support for both external and internal RenderPass
272-311: Well-documented resource cleanup with correct destruction order.The destruction sequence properly handles:
- Models (which destroy internal GPU geometry and uniform stores)
- Framebuffers (ForceLink has none, noted in comment)
- Textures
- UniformStores
- Buffers (manually destroyed since Models don't own them)
The explicit checks for
destroyedstate prevent double-free errors.src/modules/Clusters/index.ts (3)
387-424: Robust centermass calculation with proper resource management.The implementation correctly:
- Validates all required resources before rendering
- Updates vertex count dynamically
- Uses UniformStore for uniform management
- Creates a separate RenderPass for the centermass framebuffer
- Properly ends the RenderPass
426-478: Well-structured two-stage rendering pipeline.The run method properly:
- Calls
calculateCentermass()in a separate pass (different framebuffer)- Validates all resources extensively
- Updates uniforms and bindings before drawing
- Supports both external and internal RenderPass for flexibility
480-536: Comprehensive resource cleanup following documented order.The destroy method properly handles:
- 3 Models (clear, calculate, apply)
- 1 Framebuffer
- 4 Textures
- 2 UniformStores
- 3 Buffers
All with appropriate
destroyedchecks to prevent double-free errors.src/modules/ForceCenter/index.ts (4)
39-80: Efficient resource creation with proper reuse.The create method efficiently:
- Reuses the 1x1 centermass texture if it exists and is not destroyed
- Only recreates the pointIndices buffer when the byte length changes
- Properly destroys resources before recreation
- Tracks previous texture size for change detection
82-201: Well-organized program initialization with proper resource setup.The initPrograms method correctly:
- Creates separate vertex buffers for different passes
- Sets up UniformStores for both centermass calculation and force application
- Configures blend parameters for centermass accumulation
- Updates vertex count dynamically when the command already exists (line 172)
Note: The fullscreen quad buffer duplication was already flagged in the Clusters review.
203-253: Efficient two-stage rendering pipeline.The run method implementation:
- Uses
clearColorinbeginRenderPassinstead of a separate clear draw (more efficient)- Properly separates centermass calculation and force application into two passes
- Validates resource availability and size consistency before rendering
- Supports both external and internal RenderPass
255-301: Complete resource cleanup with proper tracking reset.The destroy method properly:
- Follows the documented destruction order
- Destroys all resources with appropriate checks
- Resets tracking variables (line 300)
Consistent implementation across all modules.
src/modules/Shared/texture-utils.ts (1)
10-13: The fallback to 0 is unnecessary—only standard texture formats are used.The codebase exclusively uses
'rgba32float'(16 bytes per pixel) and'rgba8unorm'(4 bytes per pixel), both of which have defined bytesPerPixel values in luma.gl's textureFormatDecoder. The?? 0fallback never triggers in practice. If the function is intended only for these standard formats, the fallback can be removed or replaced with an assertion to catch invalid formats early.src/modules/Lines/index.ts (3)
1-66: LGTM: Clean migration to luma.gl typesThe imports and field declarations properly reflect the migration from Regl to luma.gl. The explicit UniformStore type definitions and split point buffers (pointA/pointB) align with the new rendering architecture.
663-733: LGTM: Well-organized resource cleanup with clear destruction orderThe explicit destruction order (Models → Framebuffers → Textures → UniformStores → Buffers) and detailed comments make the resource lifecycle clear. The defensive
destroyedchecks prevent double-free errors.
351-402: LGTM: Proper resize handling with resource cleanupThis method demonstrates correct resource lifecycle management: detecting size changes, destroying old resources before creating new ones, and tracking state to avoid unnecessary recreation.
There was a problem hiding this comment.
Actionable comments posted: 8
🤖 Fix all issues with AI agents
In @src/modules/ForceCenter/index.ts:
- Around line 110-125: The clearCentermassCommand Model is created in
initPrograms() (using clearFrag/updateVert and clearVertexCoordBuffer) but never
used; remove the dead-code by deleting the clearCentermassCommand instantiation
and any associated buffer allocation/property (clearVertexCoordBuffer) and
related fields on the class, and remove any unused imports or references to
clearFrag/updateVert for this path; if instead the intent was to draw it,
replace the removal by a single call to clearCentermassCommand.draw(...) in the
appropriate render method—choose one fix and keep code/fields consistent.
In @src/modules/ForceGravity/index.ts:
- Around line 50-53: The code binds points.previousPositionTexture using a
non-null assertion which can crash if it's undefined; update the logic in the
block that builds the bindings (the object with keys bindings,
forceGravityUniforms, positionsTexture) to first verify
points.previousPositionTexture exists (or verify points earlier) and either
provide a safe fallback texture or throw a clear error; locate the usage of
points and previousPositionTexture in the method that constructs the bindings
(references: points, previousPositionTexture, bindings, forceGravityUniforms,
uniformStore.getManagedUniformBuffer) and add a guard that handles the undefined
case before creating the GPU binding.
In @src/modules/ForceLink/index.ts:
- Around line 168-171: The initPrograms method currently returns early if points
or store textures are missing but does not guard that
points.previousPositionTexture exists before it's used in model bindings; update
the guard in ForceLink.initPrograms to also check that
points.previousPositionTexture is present (e.g., add
`!points.previousPositionTexture` to the existing early-return condition) so
subsequent code that binds points.previousPositionTexture to the Model (used in
the model binding block) won’t access a null/undefined texture.
In @src/modules/ForceManyBody/index.ts:
- Around line 319-321: The duplicate initialization of forceVertexCoordBuffer
using the ||= pattern is redundant because forceVertexCoordBuffer is already set
earlier with the same device.createBuffer call; remove the second block that
calls device.createBuffer for forceVertexCoordBuffer to eliminate the no-op and
confusion, leaving only the original initialization (keep the earlier code that
sets forceVertexCoordBuffer and delete the later ||= device.createBuffer({...})
statement).
- Around line 449-457: The render pass already clears the framebuffer via
device.beginRenderPass({ clearColor: [...] }), so remove the redundant
full-screen clear draw by deleting the clearLevelsCommand.draw(levelPass) call;
keep calculateLevelsCommand.draw(levelPass) and levelPass.end() as-is (or
alternatively remove the clearColor parameter if you prefer CPU/GPU-side clear
behavior), but ensure you only keep one clear operation (reference:
device.beginRenderPass, clearColor, clearLevelsCommand.draw,
calculateLevelsCommand.draw, levelPass.end).
In @src/modules/Lines/index.ts:
- Around line 174-206: The Model instance drawCurveCommand holds references to
buffer objects that updatePointsBuffer may destroy and recreate, so
updatePointsBuffer must refresh the Model's attribute bindings after buffers
change; locate updatePointsBuffer and after it creates new Buffer objects
(curveLineBuffer, pointABuffer, pointBBuffer, colorBuffer, widthBuffer,
arrowBuffer, linkIndexBuffer) call drawCurveCommand.setAttributes(...) with the
new buffer references (or alternatively recreate drawCurveCommand using the same
Model constructor code) and also refresh any bindings that depend on recreated
buffers so the Model no longer references destroyed buffers.
🧹 Nitpick comments (16)
src/modules/ForceManyBody/index.ts (7)
79-88: Consider reusing zero-filled buffer for texture clearing.Creating a new
Float32Arrayfilled with zeros on everycreate()call for each existing level is allocation-heavy. Consider caching the zero buffer per size or using a clear operation if available.♻️ Suggested optimization
+ private zeroBufferCache = new Map<number, Float32Array>() + + private getZeroBuffer(size: number): Float32Array { + const key = size * size * 4 + let buffer = this.zeroBufferCache.get(key) + if (!buffer) { + buffer = new Float32Array(key).fill(0) + this.zeroBufferCache.set(key, buffer) + } + return buffer + }Then use
this.getZeroBuffer(levelTextureSize)instead ofnew Float32Array(...).fill(0).
181-183: Missingusageflag on vertex buffer.The buffer is created without specifying
usage. For consistency withpointIndicesbuffer and to ensure optimal GPU memory placement, consider addingusage: Buffer.VERTEX.♻️ Suggested fix
this.clearLevelsVertexCoordBuffer ||= device.createBuffer({ data: new Float32Array([-1, -1, 1, -1, -1, 1, 1, 1]), + usage: Buffer.VERTEX, })
268-270: Missingusageflag on vertex buffer.Same issue as above - add
usage: Buffer.VERTEXfor consistency and optimal memory allocation.♻️ Suggested fix
this.forceVertexCoordBuffer ||= device.createBuffer({ data: new Float32Array([-1, -1, 1, -1, -1, 1, 1, 1]), + usage: Buffer.VERTEX, })
356-363: Silent skip may hide issues during debugging.When sizes change and
create()hasn't been called yet,run()silently returns. Consider adding a debug log or returning a boolean to indicate whether the run was actually executed, which would help during development.
500-504: Misleading binding namelevelFbo.The binding is named
levelFbobut the value istarget.texture, not a framebuffer. This naming inconsistency can cause confusion when debugging or reading the shader code. Consider renaming tolevelTexturefor clarity.♻️ Suggested fix
this.forceCommand.setBindings({ forceUniforms: this.forceUniformStore.getManagedUniformBuffer(device, 'forceUniforms'), positionsTexture: points.previousPositionTexture!, - levelFbo: target.texture, + levelTexture: target.texture, })Note: This requires updating the corresponding shader binding name as well.
469-477: Redundant initial binding updates.These bindings are set before the loop but then immediately overwritten inside the loop at lines 500-504 and 517-522. The initial
setBindingscalls forforceCommandandforceFromItsOwnCentermassCommandare effectively no-ops since they don't include thelevelFbobinding that varies per iteration.Consider removing these pre-loop calls or consolidating the binding logic.
517-522: Same misleading binding namelevelFbo.Same issue as above -
levelFboshould be renamed tolevelTexturefor consistency.src/modules/Clusters/index.ts (2)
97-126: Consider extracting texture create-or-update logic to reduce duplication.The pattern of "destroy if exists → create → copyImageData" vs "just copyImageData" is repeated for multiple textures (clusterTexture, clusterPositionsTexture, clusterForceCoefficientTexture). This could be extracted into a helper method.
Example helper pattern
private recreateOrUpdateTexture( texture: Texture | undefined, width: number, height: number, data: Float32Array, recreate: boolean ): Texture { if (recreate) { if (texture && !texture.destroyed) texture.destroy() texture = this.device.createTexture({ width, height, format: 'rgba32float', usage: Texture.SAMPLE | Texture.RENDER | Texture.COPY_DST, }) } texture!.copyImageData({ data, bytesPerRow: getBytesPerRow('rgba32float', width), mipLevel: 0, x: 0, y: 0, }) return texture! }
300-305: Conditional initial bindings may cause shader binding errors.Using spread with conditionals like
...(this.clusterTexture && { clusterTexture: this.clusterTexture })means bindings might be missing at Model creation time if textures aren't ready. SinceinitPrograms()is called aftercreate(), these textures should exist, but the pattern is fragile. The bindings are updated incalculateCentermass()anyway, so consider requiring these textures in the guard.Suggested fix
public initPrograms (): void { const { device, store, data, points } = this // Use same check as create() and run() for consistency if (data.pointsNumber === undefined || (!data.pointClusters && !data.clusterPositions)) return + if (!this.clusterTexture || !points?.previousPositionTexture) returnsrc/modules/ForceLink/index.ts (1)
226-232: Silent skip on size mismatch may hide bugs.When
pointsTextureSizeorlinksTextureSizechanges butcreate()hasn't been called again, the method silently returns. Consider adding a warning log to help with debugging if the simulation appears stuck.Optional: Add debug logging
// Skip if sizes changed and create() wasn't called again if ( store.pointsTextureSize !== this.previousPointsTextureSize || store.linksTextureSize !== this.previousLinksTextureSize ) { + console.warn('ForceLink: size mismatch, call create() to reinitialize') return }src/modules/ForceCenter/index.ts (1)
84-91: Consider sharing the vertex coord buffer between Models.
clearVertexCoordBufferandforceVertexCoordBuffercontain identical data ([-1, -1, 1, -1, -1, 1, 1, 1]). These could be consolidated into a single shared buffer to reduce memory allocation.Suggested consolidation
- private clearVertexCoordBuffer: Buffer | undefined - private forceVertexCoordBuffer: Buffer | undefined + private quadVertexCoordBuffer: Buffer | undefined // In initPrograms(): - this.clearVertexCoordBuffer ||= device.createBuffer({ - data: new Float32Array([-1, -1, 1, -1, -1, 1, 1, 1]), - }) - - this.forceVertexCoordBuffer ||= device.createBuffer({ - data: new Float32Array([-1, -1, 1, -1, -1, 1, 1, 1]), - }) + this.quadVertexCoordBuffer ||= device.createBuffer({ + data: new Float32Array([-1, -1, 1, -1, -1, 1, 1, 1]), + })src/modules/Lines/index.ts (5)
355-361: Consider removing redundant texture initialization.The texture is already created at lines 349-354. Immediately calling
copyImageData()with zeros may be unnecessary since WebGL textures are typically cleared on creation. If explicit clearing is needed, verify whether luma.gl requires this pattern.
412-422:pointBBuffersize check usespointABuffer's computed size.Line 412 reuses
currentSizecomputed frompointABuffer(line 397). While they should always have the same size in practice, this creates an implicit coupling. If the buffers ever get out of sync, this could cause issues.♻️ Suggested improvement
- if (!this.pointBBuffer || currentSize !== data.linksNumber) { + const currentSizeB = (this.pointBBuffer?.byteLength ?? 0) / (Float32Array.BYTES_PER_ELEMENT * 2) + if (!this.pointBBuffer || currentSizeB !== data.linksNumber) {
441-524: Consider extracting buffer update pattern to reduce duplication.The
updateColor(),updateWidth(), andupdateArrow()methods follow an identical pattern: check existence, compare sizes, destroy-and-recreate if needed, or write data. This could be extracted to a helper function.♻️ Example helper pattern
private updateBuffer( buffer: Buffer | undefined, data: Float32Array, elementsPerItem: number ): Buffer { const { device } = this const expectedSize = data.length / elementsPerItem const currentSize = (buffer?.byteLength ?? 0) / (Float32Array.BYTES_PER_ELEMENT * elementsPerItem) if (!buffer || currentSize !== expectedSize) { if (buffer && !buffer.destroyed) buffer.destroy() return device.createBuffer({ data, usage: Buffer.VERTEX | Buffer.COPY_DST, }) } buffer.write(data) return buffer }
596-610: Redundant render passes for clearing and drawing.Two separate render passes are created: one for clearing (lines 596-600) and one for drawing (lines 603-610). This is inefficient. A single render pass with
clearColorset would clear and then allow drawing.Additionally, the comment at line 606-607 is misleading - both passes specify
clearColor: [0, 0, 0, 0], so the framebuffer could potentially be cleared twice.♻️ Combine into single render pass
- // Clear and render to index buffer - const clearPass = this.device.beginRenderPass({ - framebuffer: this.linkIndexFbo, - clearColor: [0, 0, 0, 0], - }) - clearPass.end() - // Render to index buffer for picking/hover detection const indexPass = this.device.beginRenderPass({ framebuffer: this.linkIndexFbo, - // Note: We explicitly set clearColor to [0, 0, 0, 0] to prevent re-clearing the framebuffer. - // Without this, luma.gl would use its default clearColor of [0, 0, 0, 1] (opaque black) - clearColor: [0, 0, 0, 0], // or `clearColor: false` + clearColor: [0, 0, 0, 0], // Clear to transparent before drawing }) this.drawCurveCommand.draw(indexPass) indexPass.end()
279-304: Uniform value construction is duplicated across methods.The uniform values at lines 280-299 (draw), 559-578 (findHoveredLine), and 142-161 (initPrograms defaults) are nearly identical. This creates maintenance burden and risk of inconsistency.
Consider extracting a helper method:
♻️ Extract uniform building helper
private buildDrawLineUniforms(renderMode: number) { const { config, store } = this return { transformationMatrix: store.transformationMatrix4x4, pointsTextureSize: store.pointsTextureSize, widthScale: config.linkWidthScale ?? 1, linkArrowsSizeScale: config.linkArrowsSizeScale ?? 1, spaceSize: store.adjustedSpaceSize ?? 0, screenSize: store.screenSize ?? [0, 0], linkVisibilityDistanceRange: config.linkVisibilityDistanceRange ?? [0, 0], linkVisibilityMinTransparency: config.linkVisibilityMinTransparency ?? 0, linkOpacity: config.linkOpacity ?? 1, greyoutOpacity: config.linkGreyoutOpacity ?? 1, curvedWeight: config.curvedLinkWeight ?? 0, curvedLinkControlPointDistance: config.curvedLinkControlPointDistance ?? 0, curvedLinkSegments: config.curvedLinks ? config.curvedLinkSegments ?? defaultConfigValues.curvedLinkSegments : 1, scaleLinksOnZoom: (config.scaleLinksOnZoom ?? true) ? 1 : 0, maxPointSize: store.maxPointSize ?? 100, renderMode, hoveredLinkIndex: store.hoveredLinkIndex ?? -1, hoveredLinkColor: store.hoveredLinkColor ?? [-1, -1, -1, -1], hoveredLinkWidthIncrease: config.hoveredLinkWidthIncrease ?? 0, } }
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
src/modules/Clusters/index.tssrc/modules/ForceCenter/index.tssrc/modules/ForceGravity/index.tssrc/modules/ForceLink/index.tssrc/modules/ForceManyBody/index.tssrc/modules/ForceMouse/index.tssrc/modules/Lines/index.tssrc/modules/Points/index.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- src/modules/ForceMouse/index.ts
🧰 Additional context used
🧬 Code graph analysis (6)
src/modules/Clusters/index.ts (3)
src/modules/core-module.ts (1)
CoreModule(7-28)src/modules/Shared/texture-utils.ts (1)
getBytesPerRow(10-13)src/modules/Shared/buffer.ts (1)
createIndexesForBuffer(1-11)
src/modules/ForceGravity/index.ts (2)
src/modules/core-module.ts (1)
CoreModule(7-28)src/modules/ForceLink/force-spring.ts (1)
forceFrag(1-89)
src/modules/ForceManyBody/index.ts (4)
src/modules/core-module.ts (1)
CoreModule(7-28)src/modules/Shared/texture-utils.ts (1)
getBytesPerRow(10-13)src/modules/Shared/buffer.ts (1)
createIndexesForBuffer(1-11)src/modules/ForceLink/force-spring.ts (1)
forceFrag(1-89)
src/modules/ForceCenter/index.ts (3)
src/modules/core-module.ts (1)
CoreModule(7-28)src/modules/Shared/texture-utils.ts (1)
getBytesPerRow(10-13)src/modules/Shared/buffer.ts (1)
createIndexesForBuffer(1-11)
src/modules/Lines/index.ts (3)
src/modules/GraphData/index.ts (1)
linksNumber(73-75)src/modules/Shared/texture-utils.ts (1)
getBytesPerRow(10-13)src/modules/Lines/geometry.ts (1)
getCurveLineGeometry(4-18)
src/modules/ForceLink/index.ts (3)
src/modules/core-module.ts (1)
CoreModule(7-28)src/modules/Shared/texture-utils.ts (1)
getBytesPerRow(10-13)src/modules/ForceLink/force-spring.ts (1)
forceFrag(1-89)
🔇 Additional comments (20)
src/modules/ForceManyBody/index.ts (4)
1-16: LGTM!The imports are well-organized, separating core luma.gl primitives from engine components. The
LevelTargettype provides a clean abstraction for managing per-level texture and framebuffer pairs.
18-61: LGTM!The class properties are well-structured with explicit typing. The separation of uniform stores (
calculateLevelsUniformStore,forceUniformStore,forceCenterUniformStore) aligns with the different render passes and provides clear ownership semantics.
159-170: LGTM!Good resource management pattern - the buffer is only recreated when the byte length changes, otherwise existing data is updated in-place with
write(). This avoids unnecessary GPU resource churn.
365-421: LGTM!Excellent resource cleanup implementation. The documented destruction order (Models → FBOs → Textures → UniformStores → Buffers) is correct and prevents use-after-free scenarios. The defensive
.destroyedchecks before callingdestroy()handle edge cases where resources may have already been cleaned up.src/modules/ForceGravity/index.ts (4)
1-6: LGTM - Clean imports for luma.gl migration.The imports are well-organized, pulling core types from
@luma.gl/coreand theModelclass from@luma.gl/engine. The raw shader imports using?rawsuffix are appropriate for Vite/bundler integration.
7-16: LGTM - Well-typed resource declarations.The class fields are properly typed with
| undefinedto support lazy initialization. TheUniformStoregeneric type correctly defines theforceGravityUniformsblock structure matching the shader's expected uniforms.
61-87: LGTM - Robust run implementation with proper resource validation.The method correctly:
- Validates all required resources before proceeding
- Checks for destroyed textures/framebuffers
- Updates uniforms and bindings per-frame
- Handles both provided and internally-created render passes
- Properly ends the pass only when created internally
89-113: LGTM - Well-documented destruction with proper ordering.The destruction follows the documented order (Models → Framebuffers → Textures → UniformStores → Buffers) and includes defensive
destroyedchecks. Setting references toundefinedafter destruction prevents accidental double-destroy scenarios.src/modules/Clusters/index.ts (4)
1-11: LGTM - Comprehensive imports for complex module.The imports cover all necessary luma.gl types and shared utilities. The raw shader imports are consistent with the project pattern.
366-403: LGTM - Well-structured centermass calculation pass.The method properly validates resources, updates uniforms and bindings dynamically, and correctly sequences the clear and calculate commands within a single render pass.
405-457: LGTM - Clean two-stage run implementation.The method correctly orchestrates:
- Centermass calculation into its own framebuffer
- Force application to the velocity framebuffer
The optional
renderPassparameter handling is consistent with the other force modules in this PR.
459-515: LGTM - Comprehensive destruction covering all resources.The destroy method properly cleans up all Models, Framebuffers, Textures, UniformStores, and Buffers in the correct order. The defensive
destroyedchecks prevent double-destruction errors.src/modules/ForceLink/index.ts (3)
1-6: LGTM - Clean imports.Imports are appropriate for the luma.gl migration.
157-161: Good handling of shader rebuild when degree changes.The shader uses
maxPointDegreeas a compile-time constant (MAX_LINKS), so destroying the Model to force a rebuild when this value changes is the correct approach. This ensures the shader loop bounds match the actual data.
266-305: LGTM - Complete resource cleanup.The destroy method properly cleans up all owned resources following the documented destruction order.
src/modules/ForceCenter/index.ts (3)
1-10: LGTM - Proper imports for ForceCenter module.The imports are consistent with other force modules in this PR.
39-76: LGTM - Efficient resource management in create().The 1x1 centermass texture is correctly reused (only data updated), and the pointIndices buffer properly handles resizing by comparing byte lengths. The
write()method for in-place updates is more efficient than recreation when sizes match.
236-282: LGTM - Thorough destruction with proper ordering.The destroy method comprehensively cleans up all resources. Note: if the unused
clearCentermassCommandis removed per the previous comment, the corresponding cleanup code should also be removed.src/modules/Lines/index.ts (2)
269-319: LGTM!The draw method correctly:
- Updates uniforms each frame with current config/store values
- Dynamically updates texture bindings to handle texture changes
- Sets instance count based on current link count
- Properly delegates rendering to the provided render pass
633-703: Well-structured resource cleanup.The destroy method:
- Documents the correct destruction order (Models → Framebuffers → Textures → UniformStores → Buffers)
- Consistently checks
.destroyedbefore calling.destroy()to prevent double-free errors- Sets all references to
undefinedafter destruction to prevent stale reference usage
There was a problem hiding this comment.
Actionable comments posted: 11
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/modules/Clusters/index.ts (1)
54-63: FixclusterCountwhenclusterPositionsexists butpointClustersis missing/partial.Right now
clusterCountcomes from(data.pointClusters ?? [])... + 1(Line 57), so if onlydata.clusterPositionsis provided you’ll end up withclusterCount = 1and silently ignore the rest ofclusterPositions.Proposed fix
- this.clusterCount = (data.pointClusters ?? []).reduce<number>((max, clusterIndex) => { + const clustersFromPointClusters = (data.pointClusters ?? []).reduce<number>((max, clusterIndex) => { if (clusterIndex === undefined || clusterIndex < 0) return max return Math.max(max, clusterIndex) - }, 0) + 1 + }, -1) + 1 + + const clustersFromPositions = + data.clusterPositions ? Math.floor(data.clusterPositions.length / 2) : 0 + + this.clusterCount = Math.max(clustersFromPointClusters, clustersFromPositions)Also applies to: 76-82
🤖 Fix all issues with AI agents
In @src/modules/Clusters/index.ts:
- Around line 410-462: run() currently aborts early for missing/destroyed
velocity FBO even when a renderPass was provided and lacks a guard against
texture/size changes; update run(renderPass?) so checks that depend on the
fallback velocity framebuffer only run when renderPass is not supplied (i.e.,
gate the points.velocityFbo and points.velocityFbo.destroyed check behind
!renderPass), and add a size-stability guard that compares current runtime sizes
(e.g., this.clustersTextureSize, this.clusterPositionsTexture?.width/height or
other size markers used in create()) against stored size state and returns early
if sizes changed and create() has not been rerun; preserve existing checks for
applyForcesCommand, applyForcesUniformStore, clusterTexture, centermassTexture,
clusterPositionsTexture, clusterForceCoefficientTexture and use the same
early-return behavior when those are missing.
In @src/modules/ForceCenter/index.ts:
- Around line 83-88: initPrograms() constructs Models that assume
points.previousPositionTexture exists, which can be undefined and crash luma.gl
v9; add a guard in initPrograms() (alongside the existing checks for
this.centermassFbo/centermassTexture/pointIndices) to return early if
!points.previousPositionTexture before creating the Models that reference
points.previousPositionTexture. Ensure the guard covers both Model creation
blocks (the ones using points.previousPositionTexture! around the sections
corresponding to the earlier and later Model constructors) so bindings are valid
at construction time.
In @src/modules/ForceManyBody/index.ts:
- Around line 90-95: In create() you're destroying existingTarget.texture before
existingTarget.fbo which risks freeing a texture still attached to a
framebuffer; change the destruction order to destroy the framebuffer first and
then the texture so it matches the safe pattern used in destroy() (i.e., call
existingTarget.fbo.destroy() before existingTarget.texture.destroy()), and keep
the same guarded checks (!existingTarget.fbo.destroyed and
!existingTarget.texture.destroyed) around each destroy call.
In @src/modules/Lines/index.ts:
- Around line 269-319: The draw() method can silently return if
pointABuffer/pointBBuffer are missing; modify draw() (in the Lines class) to
attempt recovery by calling this.updatePointsBuffer() when either pointABuffer
or pointBBuffer is undefined, then re-check and if still missing emit a single
error log or throw (use existing logger or throw Error) instead of silently
returning; alternatively, if you prefer strict init order, add an assertion/log
in initPrograms() and reference
initPrograms()/updatePointsBuffer()/pointABuffer/pointBBuffer so callers know
initialization must complete before draw().
- Around line 659-729: In destroy(), after cleaning up buffers and before
returning, explicitly reset this.previousScreenSize = undefined to avoid stale
size comparisons on reuse, and also set this.curveLineGeometry = undefined to
clear CPU-side geometry state; add these assignments (references: destroy(),
previousScreenSize, curveLineGeometry) in the same teardown sequence (e.g.,
after quadBuffer reset).
- Around line 12-66: The UniformStore types allow arbitrary-length number[]
which can hide shader binding bugs; update the drawLineUniformStore and
hoveredLineIndexUniformStore type declarations to use fixed-size tuples rather
than number[] (e.g., change hoveredLinkColor: number[] → hoveredLinkColor:
[number, number, number, number], mousePosition: number[] → mousePosition:
[number, number], linkVisibilityDistanceRange: number[] →
linkVisibilityDistanceRange: [number, number], and any other scalar array
uniforms to the correct tuple lengths) so the types for drawLineUniforms and
hoveredLineIndexUniforms (and the fields referenced in drawLineUniformStore and
hoveredLineIndexUniformStore) exactly match the expected GLSL uniform sizes.
- Around line 374-447: In updatePointsBuffer, guard against
store.pointsTextureSize being 0/undefined before computing fromX/fromY/toX/toY:
if store.pointsTextureSize is falsy (0 or undefined) return early or set a safe
fallback (e.g., 1) and use that value for the modulus/division so you never
produce NaN/Infinity; ensure all uses of store.pointsTextureSize in the loop
reference the validated value, then proceed with creating/updating pointABuffer,
pointBBuffer, linkIndexBuffer and calling drawCurveCommand.setAttributes as
before.
- Around line 448-547: The updateColor/updateWidth/updateArrow methods currently
call buffer.write(...) with external arrays that may be the wrong length;
validate the incoming array lengths against linksNumber (for updateColor check
data.linkColors.length === linksNumber * 4, for updateWidth and updateArrow
check length === linksNumber) and if they mismatch, create a zero-filled
Float32Array of the expected size (or create a padded/copied array of the
correct size) and use that for device.createBuffer or buffer.write; also ensure
the buffer resize branch uses the validated array so write never receives a
mismatched length.
- Around line 548-577: In updateCurveLineGeometry, replace the optional chaining
destroy call (this.curveLineBuffer?.destroy()) with the same explicit guard used
elsewhere: check that this.curveLineBuffer is truthy and that
!this.curveLineBuffer.destroyed before calling this.curveLineBuffer.destroy();
this aligns with how pointABuffer/arrowBuffer are cleaned up and prevents
calling destroy on already-destroyed buffers.
- Around line 578-657: The findHoveredLine method currently creates two render
passes on the same framebuffer (clearPass then indexPass) causing a redundant
clear; remove the empty clearPass and merge its clearColor into the single
indexPass used for drawing: delete the block that begins/ends clearPass, and
ensure the beginRenderPass call used for drawing (the one that calls
this.drawCurveCommand.draw) includes clearColor: [0,0,0,0] so the framebuffer is
cleared once before drawing; update any related comments and keep the subsequent
hoveredLineIndex render pass unchanged.
🧹 Nitpick comments (2)
src/modules/ForceManyBody/index.ts (1)
273-276: Remove duplicateforceVertexCoordBufferinitialization.The second
this.forceVertexCoordBuffer ||= device.createBuffer(...)is redundant.Also applies to: 324-327
src/modules/ForceCenter/index.ts (1)
115-130:clearCentermassCommandlooks unused now.
run()relies onbeginRenderPass({ clearColor })and never drawsclearCentermassCommand; consider deleting the Model +clearVertexCoordBufferif it’s not needed.Also applies to: 199-217
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
src/modules/Clusters/index.tssrc/modules/ForceCenter/index.tssrc/modules/ForceManyBody/index.tssrc/modules/Lines/index.tssrc/modules/Points/index.ts
🧰 Additional context used
🧬 Code graph analysis (3)
src/modules/Clusters/index.ts (3)
src/modules/core-module.ts (1)
CoreModule(7-28)src/modules/Shared/texture-utils.ts (1)
getBytesPerRow(10-13)src/modules/Shared/buffer.ts (1)
createIndexesForBuffer(1-11)
src/modules/ForceCenter/index.ts (3)
src/modules/core-module.ts (1)
CoreModule(7-28)src/modules/Shared/texture-utils.ts (1)
getBytesPerRow(10-13)src/modules/Shared/buffer.ts (1)
createIndexesForBuffer(1-11)
src/modules/ForceManyBody/index.ts (3)
src/modules/core-module.ts (1)
CoreModule(7-28)src/modules/Shared/texture-utils.ts (1)
getBytesPerRow(10-13)src/modules/Shared/buffer.ts (1)
createIndexesForBuffer(1-11)
🔇 Additional comments (6)
src/modules/ForceManyBody/index.ts (1)
159-176: Good: refresh Model attributes after buffer recreation.Updating
calculateLevelsCommandattributes whenpointIndicesis recreated prevents stale VAO/attribute state issues.src/modules/Clusters/index.ts (1)
97-229: Lifecycle handling looks solid (esp. framebuffer-before-texture).The recreation logic and
destroy()ordering are consistent and defensive (destroyed checks + nulling references).Also applies to: 464-520
src/modules/ForceCenter/index.ts (1)
189-239: Good: size-stability guard prevents stale buffer/texture use.The
store.pointsTextureSize !== this.previousPointsTextureSizeearly-return is the right pattern for these modules.src/modules/Lines/index.ts (3)
67-267: Good: “create empty buffers first” avoids Model creation with missing attributes, but watch format/usage assumptions.The pattern of ensuring attribute buffers exist before
new Model(...)is solid (especially given the PR note about WebGL errors when buffers get recreated). Two things to double-check:
Texturecreation withdatainline (Line 73-79) behaves the same across backends; if not, prefercopyImageDataconsistently.renderModeis declared asf32(Line 137-140) but semantically used as a discrete mode; if shaders use integer ops/branches, ensure you’re not hitting float-compare corner cases (recommend== 0.0/1.0only).
321-372: Avoid allocating and uploading a full-screen Float32Array just to zero-initialize the link index texture.The
copyImageData()call with a zero-filled Float32Array (lines 355–361) creates unnecessary CPU allocations and GPU upload bandwidth, especially noticeable on resize and high-DPI displays. Since the framebuffer is already cleared in the render pass, remove this initialization:this.linkIndexTexture = device.createTexture({ width: screenWidth, height: screenHeight, format: 'rgba32float', usage: Texture.SAMPLE | Texture.RENDER | Texture.COPY_DST, }) -this.linkIndexTexture.copyImageData({ - data: new Float32Array(screenWidth * screenHeight * 4).fill(0), - bytesPerRow: getBytesPerRow('rgba32float', screenWidth), - mipLevel: 0, - x: 0, - y: 0, -})Additionally, when
isLinkHoveringEnabledis toggled off at runtime,updateLinkIndexFbo()returns without freeing GPU resources. Add cleanup to prevent memory leaks:if (!this.store.isLinkHoveringEnabled) { + this.linkIndexFbo?.destroy() + this.linkIndexFbo = undefined + this.linkIndexTexture?.destroy() + this.linkIndexTexture = undefined + this.previousScreenSize = undefined return }
1-10: All luma.gl v9.2.5 APIs are correctly used; no breaking changes detected.The code correctly leverages
@luma.gl/core@9.2.5and@luma.gl/engine@9.2.5:
UniformStoreinitialization with nested uniform groups and typed schema is validgetManagedUniformBuffer(device, groupName)calls on the uniform store are correctModel.setAttributes(),setVertexCount(), andsetInstanceCount()are all supported methodsTexture.copyImageData()withbytesPerRowandrgba32floatformat is the correct pattern for float texture uploadsdevice.beginRenderPass()withclearColorparameter is validAll APIs are used with proper patterns and no upstream build issues expected from this file's API usage.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@src/modules/Clusters/index.ts`:
- Around line 250-300: The initPrograms method uses this.pointIndices without
checking it when constructing this.calculateCentermassCommand; add a guard that
verifies pointIndices exists before creating the Model (either early return or
skip creating calculateCentermassCommand), and only reference this.pointIndices
(remove the non-null assertion) inside the creation when present; ensure the
UniformStore creation can still run if desired but avoid passing undefined to
the Model constructor by gating the Model creation on the presence of
this.pointIndices.
♻️ Duplicate comments (11)
src/modules/Lines/index.ts (4)
30-62: Tighten UniformStore TypeScript types to fixed-size tuples.Several uniform types are declared as
number[]which allows passing wrong-length arrays, causing silent shader binding failures. Use fixed-size tuples to catch type errors at compile time.♻️ Suggested type fixes
drawLineUniforms: { transformationMatrix: [number, number, number, number, number, number, number, number, number, number, number, number, number, number, number, number]; pointsTextureSize: number; // ... other fields ... - linkVisibilityDistanceRange: number[]; + linkVisibilityDistanceRange: [number, number]; // ... other fields ... - hoveredLinkColor: number[]; + hoveredLinkColor: [number, number, number, number]; // ... }; }> | undefined private hoveredLineIndexUniformStore: UniformStore<{ hoveredLineIndexUniforms: { - mousePosition: number[]; + mousePosition: [number, number]; screenSize: [number, number]; }; }> | undefined
562-563: Use consistent destroyed check before calling destroy().For consistency with the rest of the module (e.g.,
pointABuffer,arrowBufferdestruction indestroy()), use an explicit guard instead of optional chaining.♻️ Suggested fix
if (!this.curveLineBuffer || this.curveLineBuffer.byteLength !== flatGeometry.byteLength) { - this.curveLineBuffer?.destroy() + if (this.curveLineBuffer && !this.curveLineBuffer.destroyed) { + this.curveLineBuffer.destroy() + } this.curveLineBuffer = device.createBuffer({
657-727: LGTM! Comprehensive destroy() with correct ordering.The destruction follows the documented order and properly checks
destroyedstate before cleanup. All resources (Models, FBOs, Textures, UniformStores, Buffers) are handled.Consider also resetting
previousScreenSizeandcurveLineGeometryfor completeness if the instance might be reused.
377-397: Guard againststore.pointsTextureSize === 0to prevent NaN/Infinity.The modulo and division operations on lines 388-391 will produce invalid values when
pointsTextureSizeis 0 or undefined, potentially corrupting GPU buffer data.🛡️ Proposed fix
public updatePointsBuffer (): void { const { device, data, store } = this if (data.linksNumber === undefined || data.links === undefined) return + if (!store.pointsTextureSize) returnsrc/modules/ForceLink/index.ts (1)
168-171: Add guard forpoints.previousPositionTexturebefore Model creation.While the
run()method properly validatespoints.previousPositionTexture, theinitPrograms()guard at line 170 only checkspointsexistence. IfpreviousPositionTextureis undefined wheninitPrograms()runs, the Model bindings could be set up with invalid state.♻️ Suggested fix
public initPrograms (): void { const { device, store, points } = this - if (!points || !store.pointsTextureSize || !store.linksTextureSize) return + if (!points || !store.pointsTextureSize || !store.linksTextureSize || !points.previousPositionTexture) return if (!this.linkFirstIndicesAndAmountTexture || !this.indicesTexture || !this.biasAndStrengthTexture || !this.randomDistanceTexture) returnsrc/modules/ForceManyBody/index.ts (4)
308-310: Remove redundant buffer initialization.This buffer creation duplicates line 255-257. Since
||=is used, this code has no effect and only adds confusion.♻️ Suggested fix
- this.forceVertexCoordBuffer ||= device.createBuffer({ - data: new Float32Array([-1, -1, 1, -1, -1, 1, 1, 1]), - }) - this.forceFromItsOwnCentermassCommand ||= new Model(device, {
64-68: Guard against non-positiveadjustedSpaceSizebefore calculating levels.
Math.log2(store.adjustedSpaceSize)produces-InfinitywhenadjustedSpaceSizeis 0, andNaNfor negative values. This would break the loop at line 67 and potentially cause issues throughout the module.🛡️ Proposed fix
public create (): void { const { device, store } = this if (!store.pointsTextureSize) return + if (!store.adjustedSpaceSize || store.adjustedSpaceSize <= 0) return - this.levels = Math.log2(store.adjustedSpaceSize) + const rawLevels = Math.log2(store.adjustedSpaceSize) + this.levels = Number.isFinite(rawLevels) ? Math.max(0, Math.floor(rawLevels)) : 0
87-91: Fix destruction order: destroy framebuffer before texture.Lines 89-90 destroy the texture before the framebuffer, but the established pattern (documented in
destroy()at lines 368-385) is to destroy framebuffers first since they reference textures. Destroying a texture while it's still attached to a framebuffer can cause use-after-free issues in luma.gl/WebGPU.🐛 Proposed fix
// Destroy old resources if size changed if (existingTarget) { - if (!existingTarget.texture.destroyed) existingTarget.texture.destroy() - if (!existingTarget.fbo.destroyed) existingTarget.fbo.destroy() + if (!existingTarget.fbo.destroyed) existingTarget.fbo.destroy() + if (!existingTarget.texture.destroyed) existingTarget.texture.destroy() }
178-229: Add explicit null check forpointIndicesbefore non-null assertion.Line 204 uses
this.pointIndices!butinitPrograms()doesn't guard againstpointIndicesbeing undefined. Ifcreate()wasn't called first, this will fail at runtime.🛡️ Proposed fix
public initPrograms (): void { const { device, store, data, points } = this - if (!data.pointsNumber || !points || !store.pointsTextureSize) return + if (!data.pointsNumber || !points || !store.pointsTextureSize || !this.pointIndices) returnsrc/modules/ForceCenter/index.ts (1)
80-85: Consider adding guard forpoints.previousPositionTextureininitPrograms().While the
run()method (line 171) has a guard forpoints.previousPositionTexture, theinitPrograms()method does not. IfinitPrograms()is called before the Points module has created its textures, the Models will be created successfully (since texture bindings are set dynamically inrun()), but the code comment suggests this might be worth guarding for consistency with other modules.The current implementation works because texture bindings are deferred to
run(), but adding a guard would make the initialization requirements more explicit.Optional: Add explicit guard for consistency
public initPrograms (): void { const { device, store, points } = this if (!points || !store.pointsTextureSize) return + if (!points.previousPositionTexture || points.previousPositionTexture.destroyed) return if (!this.centermassFbo || this.centermassFbo.destroyed || !this.centermassTexture || this.centermassTexture.destroyed) return if (!this.pointIndices) returnsrc/modules/Clusters/index.ts (1)
384-401: Add size-stability guard and fixvelocityFbocheck.Two issues:
- Line 401 unconditionally checks
velocityFboeven whenrenderPassis provided, but the fallbackvelocityFbois only needed when norenderPassis supplied- Missing size-stability guard (present in ForceCenter at line 175)
Proposed fix
public run (renderPass?: RenderPass): void { if (!this.data.pointClusters && !this.data.clusterPositions) return + // Skip if sizes changed and create() wasn't called yet + if ( + this.store.pointsTextureSize !== this.previousPointsTextureSize || + this.clustersTextureSize !== this.previousClustersTextureSize || + this.clusterCount !== this.previousClusterCount + ) return + // Calculate centermass (creates its own RenderPass - different framebuffer) this.calculateCentermass() // Add safety check if (!this.applyForcesCommand || !this.applyForcesUniformStore) { return } // Add destroyed checks for resources before use if (!this.clusterTexture || this.clusterTexture.destroyed) return if (!this.centermassTexture || this.centermassTexture.destroyed) return if (!this.clusterPositionsTexture || this.clusterPositionsTexture.destroyed) return if (!this.clusterForceCoefficientTexture || this.clusterForceCoefficientTexture.destroyed) return if (!this.points?.previousPositionTexture || this.points.previousPositionTexture.destroyed) return - if (!this.points?.velocityFbo || this.points.velocityFbo.destroyed) return + if (!renderPass && (!this.points?.velocityFbo || this.points.velocityFbo.destroyed)) return
🧹 Nitpick comments (2)
src/modules/Points/index.ts (2)
206-217: Debug logging retained in production code.The size mismatch check includes
console.errorlogging. While useful for development, this could clutter production logs. Consider using a debug flag or removing after validation.Optional: Add debug flag or use assertion
const expectedBytes = pointsTextureSize * pointsTextureSize * 4 * 4 // width * height * 4 components * 4 bytes const actualBytes = initialState.byteLength if (actualBytes !== expectedBytes) { - console.error('Texture data size mismatch:', { - pointsTextureSize, - expectedBytes, - actualBytes, - textureDataSize, - dataLength: initialState.length, - }) + // This should never happen - indicates a bug in size calculation + console.assert(false, 'Texture data size mismatch', { pointsTextureSize, expectedBytes, actualBytes }) }
1828-1906: Extract duplicate FBO filling logic.
getSampledPointPositionsMap()(lines 1833-1858) andgetSampledPoints()(lines 1879-1905) contain nearly identical code for filling the sampled points FBO. Consider extracting to a private method.Extract common logic
private fillSampledPointsFbo(): boolean { if (!this.fillSampledPointsFboCommand || !this.fillSampledPointsUniformStore || !this.sampledPointsFbo) { return false } if (!this.currentPositionTexture || this.currentPositionTexture.destroyed) return false this.fillSampledPointsFboCommand.setVertexCount(this.data.pointsNumber ?? 0) this.fillSampledPointsUniformStore.setUniforms({ fillSampledPointsUniforms: { pointsTextureSize: this.store.pointsTextureSize ?? 0, transformationMatrix: this.store.transformationMatrix4x4, spaceSize: this.store.adjustedSpaceSize ?? 0, screenSize: this.store.screenSize ?? [0, 0], }, }) this.fillSampledPointsFboCommand.setBindings({ positionsTexture: this.currentPositionTexture, }) const fillPass = this.device.beginRenderPass({ framebuffer: this.sampledPointsFbo, clearColor: [0, 0, 0, 0], }) this.fillSampledPointsFboCommand.draw(fillPass) fillPass.end() return true } public getSampledPointPositionsMap(): Map<number, [number, number]> { const positions = new Map<number, [number, number]>() if (!this.sampledPointsFbo || this.sampledPointsFbo.destroyed) return positions if (!this.fillSampledPointsFbo()) return positions // ... rest of method }
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
src/modules/Clusters/index.tssrc/modules/ForceCenter/index.tssrc/modules/ForceGravity/index.tssrc/modules/ForceLink/index.tssrc/modules/ForceManyBody/index.tssrc/modules/ForceMouse/index.tssrc/modules/Lines/index.tssrc/modules/Points/index.ts
🧰 Additional context used
🧬 Code graph analysis (5)
src/modules/ForceMouse/index.ts (2)
src/modules/core-module.ts (1)
CoreModule(7-28)src/modules/ForceLink/force-spring.ts (1)
forceFrag(1-89)
src/modules/ForceLink/index.ts (3)
src/modules/core-module.ts (1)
CoreModule(7-28)src/modules/Shared/texture-utils.ts (1)
getBytesPerRow(10-13)src/modules/ForceLink/force-spring.ts (1)
forceFrag(1-89)
src/modules/ForceGravity/index.ts (2)
src/modules/core-module.ts (1)
CoreModule(7-28)src/modules/ForceLink/force-spring.ts (1)
forceFrag(1-89)
src/modules/Clusters/index.ts (3)
src/modules/core-module.ts (1)
CoreModule(7-28)src/modules/Shared/texture-utils.ts (1)
getBytesPerRow(10-13)src/modules/Shared/buffer.ts (1)
createIndexesForBuffer(1-11)
src/modules/Points/index.ts (4)
src/modules/core-module.ts (1)
CoreModule(7-28)src/modules/Shared/texture-utils.ts (1)
getBytesPerRow(10-13)src/modules/Shared/buffer.ts (1)
createIndexesForBuffer(1-11)src/helper.ts (1)
readPixels(75-84)
🔇 Additional comments (34)
src/modules/ForceGravity/index.ts (4)
1-16: LGTM! Clean migration to luma.gl primitives.The imports and class structure properly establish the Model-based rendering pattern with typed UniformStore for gravity uniforms.
18-61: LGTM! Proper lazy initialization pattern.The
||=pattern ensures resources are only created once. The Model is correctly configured with:
- Vertex buffer for fullscreen quad
- UniformStore with proper WGSL type declarations
- Texture bindings deferred to run() for dynamic updates
63-89: LGTM! Robust resource validation before rendering.The guards at lines 65-67 properly validate:
pointsandrunCommandexistencepreviousPositionTextureexistence and destruction statevelocityFbowhen no external render pass providedThis addresses the previously flagged concern about non-null assertions on
previousPositionTexture.
91-115: LGTM! Correct destruction order.The documented destruction order (Models → Framebuffers → Textures → UniformStores → Buffers) is followed, and the
destroyedproperty is checked before callingdestroy()on the buffer.src/modules/Lines/index.ts (5)
67-89: LGTM! Proper FBO and texture initialization.The hovered line index texture and framebuffer are correctly initialized with
rgba32floatformat and appropriate usage flags.
174-228: LGTM! Comprehensive Model configuration for curve rendering.The Model is properly configured with:
- Instanced attributes for per-link data (stepMode: 'instance')
- Uniform buffer bindings via UniformStore
- Correct blend parameters for alpha compositing
272-322: LGTM! Proper draw flow with resource validation.The draw method correctly:
- Validates all required resources before rendering
- Updates uniforms via UniformStore
- Dynamically binds textures before draw
- Sets instance count based on link count
442-448: Good: Model attributes updated after buffer recreation.The
setAttributes()call correctly updates the Model's attribute bindings when buffers are recreated due to size changes. This addresses the previously flagged issue about broken Model attribute bindings.
628-634: Good: Consolidated render pass with clearing.The picking pass now uses a single
beginRenderPasswithclearColor: [0, 0, 0, 0]instead of the redundant dual-pass approach previously flagged. This correctly clears and draws in one pass.src/modules/ForceMouse/index.ts (4)
1-15: LGTM! Clean type declarations.The UniformStore type properly uses
[number, number]tuple formousePos, which is consistent with the 2D coordinate nature of the mouse position.
17-59: LGTM! Proper Model initialization.The fullscreen quad Model is correctly configured with vertex buffer, uniform bindings, and appropriate render parameters.
61-86: LGTM! Robust resource validation in run().The guards properly validate all dependencies:
points,runCommand,uniformStoreexistencepreviousPositionTextureexistence and destruction statevelocityFbowhen no external render pass providedThe pattern of creating a local render pass when none is provided, and conditionally ending it, is correct.
88-112: LGTM! Correct destruction order.Follows the established pattern: Models → UniformStores → Buffers. The buffer destruction correctly checks the
destroyedflag before cleanup.src/modules/ForceLink/index.ts (5)
1-38: LGTM! Well-structured state tracking.The class properly tracks previous texture sizes and max point degree to detect when shader/resource rebuilds are needed. The UniformStore type declaration matches the shader's uniform block.
80-108: LGTM! Proper texture lifecycle management.The code correctly:
- Compares current vs required texture dimensions
- Destroys old textures with
destroyedcheck before recreation- Copies data after creation/recreation
The non-null assertion at line 102 is acceptable since the texture was just created on line 95-100 in the same synchronous block.
157-165: Good: Shader rebuild on degree change.Destroying the Model when
maxPointDegreechanges ensures the shader (which embedsMAX_LINKSas a constant) gets regenerated with the new value.
217-261: LGTM! Comprehensive validation in run().The run method properly:
- Validates all required resources and textures
- Skips execution if texture sizes changed without
create()being called- Updates uniforms and bindings before draw
- Handles optional render pass lifecycle
263-302: LGTM! Thorough resource cleanup.The destroy method correctly cleans up all owned resources in the proper order, including the four link-related textures that are unique to this module.
src/modules/ForceManyBody/index.ts (5)
1-59: LGTM! Well-organized type definitions and state.The
LevelTargettype cleanly encapsulates the per-level texture/framebuffer pair. The three separate UniformStores for different render passes maintain clean separation of concerns.
346-353: LGTM! Proper size-change detection.The run method correctly skips execution when texture/space sizes have changed since the last
create()call, preventing stale data issues.
355-405: LGTM! Comprehensive destroy() with correct FBO-before-texture order.The destruction correctly:
- Destroys Models first
- Destroys all level FBOs (lines 369-373)
- Destroys textures (lines 375-386)
- Destroys UniformStores
- Destroys Buffers last
407-441: LGTM! Per-level rendering with dynamic uniform updates.The drawLevels method properly:
- Validates required resources
- Updates uniforms for each level's cell size
- Uses separate render passes per level with clearing
444-503: LGTM! Force calculation with centermass fallback.The implementation correctly:
- Uses a single render pass for all force calculations (additive blending)
- Applies centermass fallback only on the deepest level
- Manages render pass lifecycle based on whether one was provided
src/modules/ForceCenter/index.ts (3)
36-78: LGTM - Resource creation with proper size tracking.The
create()method correctly:
- Uses
||=for texture/framebuffer creation to avoid recreation- Uses
getBytesPerRowfor proper byte calculation- Handles buffer recreation when size changes
- Updates model attributes after buffer recreation
- Tracks
previousPointsTextureSizefor size stability
167-216: LGTM - Well-structured two-stage rendering pipeline.The
run()method correctly:
- Guards all required resources with destroyed checks
- Conditionally checks
velocityFboonly when no externalrenderPassis provided (line 172)- Skips execution if sizes changed (line 175) - important for preventing stale state
- Creates and ends internal render pass for centermass calculation
- Properly handles optional external render pass for velocity updates
218-258: LGTM - Proper destruction order and cleanup.The
destroy()method follows the documented order (Models → Framebuffers → Textures → UniformStores → Buffers) which is correct for luma.gl resource dependencies. All resources are checked for existence and destroyed state before cleanup.src/modules/Clusters/index.ts (3)
47-122: LGTM - Robust size-change handling with conditional texture recreation.The
create()method implements a solid pattern:
- Tracks three dimensions of size change (pointsTextureSize, clustersTextureSize, clusterCount)
- Conditionally recreates textures only when sizes change
- Updates texture data in-place when sizes are stable
- Uses
getBytesPerRowconsistently for proper byte calculations
346-382: LGTM - Proper resource validation and render pass handling.The
calculateCentermass()method has comprehensive guards for all required resources including the dependentpoints.previousPositionTexture, and correctly manages the render pass lifecycle.
437-487: LGTM - Consistent destruction pattern.The
destroy()method follows the established order and comprehensively cleans up all resources including the multiple textures and uniform stores specific to this module.src/modules/Points/index.ts (5)
81-194: Uniform type definitions are well-documented.The UniformStore type definitions include explicit comments about std140 layout ordering requirements. This is critical for correct uniform buffer binding and the documentation helps maintain correctness during future modifications.
448-466: Good defensive initialization pattern.The
initPrograms()method ensures all required resources (atlas, textures, buffers) exist before Model creation. This prevents undefined bindings at construction time.
1330-1447: LGTM - Layered rendering with proper state management.The
draw()method correctly implements layered rendering for selected/unselected points with appropriate uniform and binding updates for each layer. The repetitivesetBindingscalls are necessary since uniforms change between draw calls.
1942-2120: LGTM - Comprehensive resource cleanup.The
destroy()method is thorough, cleaning up all ~50 GPU resources in the correct dependency order. While verbose, this explicit approach ensures no resources are leaked.
2122-2137: LGTM - Safe double-buffering implementation.The
swapFbo()method correctly implements texture/FBO swapping with safety checks to prevent swapping destroyed resources.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@src/index.ts`:
- Around line 675-682: The clear render pass in both the render() and destroy()
methods currently calls device.beginRenderPass(...) and clearPass.end() but
never flushes commands; after the clearPass.end() call in each method (same
pattern as renderFrame()), call this.device.submit() (guarded by the this.device
existence check) so the GPU receives the commands; update the code paths around
device.beginRenderPass / clearPass.end in render() and destroy() to mirror
renderFrame() by invoking this.device.submit() immediately after ending the
pass.
♻️ Duplicate comments (1)
src/index.ts (1)
1415-1423: PreventensureDevicecallbacks after destroy.
ensureDevicestill invokes queued callbacks without checking_isDestroyed, so callbacks can fire after teardown. Please guard before invoking the callback (and optionally before logging) to avoid post-destroy work.🔧 Suggested fix
private ensureDevice (callback: () => void): boolean { if (!this.device) { this.deviceInitPromise .then(() => { + if (this._isDestroyed) return callback() }) .catch(error => { + if (this._isDestroyed) return console.error('Device initialization failed', error) }) return true } return false }
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/index.ts
🧰 Additional context used
🧬 Code graph analysis (1)
src/index.ts (3)
src/config.ts (1)
GraphConfigInterface(20-612)src/helper.ts (2)
getRgbaColor(16-32)readPixels(75-84)src/variables.ts (1)
defaultConfigValues(12-62)
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@src/index.ts`:
- Around line 1446-1454: validateDevice currently only checks for non-null
HTMLCanvasElement context but doesn't ensure the device is WebGL-capable, which
causes runtime failures when later code calls device.readPixelsToArrayWebGL() or
accesses (device as WebGLDevice).gl; update validateDevice to assert the device
is a WebGLDevice by checking for the WebGL-specific surface/GL properties (e.g.,
presence of readPixelsToArrayWebGL method and a gl property or
canvasContext.type indicating 'html-canvas' with WebGL support) and throw a
clear Error if not; reference validateDevice, WebGLDevice,
readPixelsToArrayWebGL, and any uses of (device as WebGLDevice).gl to locate and
change the guard so external devices are validated at runtime.
♻️ Duplicate comments (6)
src/modules/ForceManyBody/index.ts (2)
64-68: Clamplevelsto a non‑negative integer.
Math.log2can yield fractional or non‑finite values (e.g., 0/negative space sizes), which breaks the loop bounds and the deepest‑level centermass fallback. Please floor and guard it.Suggested fix
- this.levels = Math.log2(store.adjustedSpaceSize) + const rawLevels = Math.log2(store.adjustedSpaceSize ?? 0) + this.levels = Number.isFinite(rawLevels) && rawLevels > 0 + ? Math.floor(rawLevels) + : 0
87-90: Destroy the framebuffer before its attached texture.
Destroying the texture first can leave the framebuffer referencing a freed resource.Suggested fix
- if (existingTarget) { - if (!existingTarget.texture.destroyed) existingTarget.texture.destroy() - if (!existingTarget.fbo.destroyed) existingTarget.fbo.destroy() - } + if (existingTarget) { + if (!existingTarget.fbo.destroyed) existingTarget.fbo.destroy() + if (!existingTarget.texture.destroyed) existingTarget.texture.destroy() + }src/index.ts (1)
684-691: Submit after clear passes so commands flush.
Both clear passes end without adevice.submit(), so the clear may never be flushed.Suggested fix
if (this.device) { const clearPass = this.device.beginRenderPass({ clearColor: this.store.backgroundColor, clearDepth: 1, clearStencil: 0, }) clearPass.end() + this.device.submit() }if (this.device) { const clearPass = this.device.beginRenderPass({ clearColor: this.store.backgroundColor, clearDepth: 1, clearStencil: 0, }) clearPass.end() + this.device.submit() this.device.destroy() }Also applies to: 1324-1332
src/modules/Lines/index.ts (2)
377-391: Guard against zeropointsTextureSizebefore modulo/division.
IfpointsTextureSizeis 0/undefined, the index math yields NaN/Infinity and corrupts GPU data.Suggested fix
public updatePointsBuffer (): void { const { device, data, store } = this if (data.linksNumber === undefined || data.links === undefined) return + if (!store.pointsTextureSize) return
451-507: Validate link color/width/arrow array lengths before GPU writes.
linkColors,linkWidths, andlinkArrowscan be mismatched withlinksNumber, which can lead to partial writes or out‑of‑bounds GPU uploads. Consider validating and falling back to zero‑filled arrays.Example fix (apply similarly to width/arrow)
public updateColor (): void { const { device, data } = this const linksNumber = data.linksNumber ?? 0 - const colorData = data.linkColors ?? new Float32Array(linksNumber * 4).fill(0) + const expected = linksNumber * 4 + const colorData = + data.linkColors && data.linkColors.length === expected + ? data.linkColors + : new Float32Array(expected).fill(0)Also applies to: 515-542
src/modules/Clusters/index.ts (1)
396-401: GatevelocityFbocheck on!renderPassfor consistency.Line 401 returns early when
velocityFbois missing/destroyed, but this resource is only used in the fallback branch (lines 427-433) when no externalrenderPassis provided. This unnecessarily blocks rendering when a validrenderPassis supplied.🔧 Suggested fix
if (!this.clusterTexture || this.clusterTexture.destroyed) return if (!this.centermassTexture || this.centermassTexture.destroyed) return if (!this.clusterPositionsTexture || this.clusterPositionsTexture.destroyed) return if (!this.clusterForceCoefficientTexture || this.clusterForceCoefficientTexture.destroyed) return if (!this.points?.previousPositionTexture || this.points.previousPositionTexture.destroyed) return - if (!this.points?.velocityFbo || this.points.velocityFbo.destroyed) return + if (!renderPass && (!this.points?.velocityFbo || this.points.velocityFbo.destroyed)) return
🧹 Nitpick comments (1)
src/modules/Clusters/index.ts (1)
93-184: Consider extracting repetitive texture creation/update logic.The texture handling pattern (check existence → destroy old → create new → copy data OR just copy data) is repeated for
clusterTexture,clusterPositionsTexture, andclusterForceCoefficientTexture. This adds ~90 lines of similar code.♻️ Example helper to reduce duplication
// Could be added to texture-utils.ts function updateOrRecreateTexture( device: Device, texture: Texture | undefined, options: { width: number; height: number; format: TextureFormat }, data: Float32Array, shouldRecreate: boolean ): Texture { if (!texture || shouldRecreate) { if (texture && !texture.destroyed) { texture.destroy() } texture = device.createTexture({ ...options, usage: Texture.SAMPLE | Texture.RENDER | Texture.COPY_DST, }) } texture.copyImageData({ data, bytesPerRow: getBytesPerRow(options.format, options.width), mipLevel: 0, x: 0, y: 0, }) return texture }
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/stories/2. configuration.mdx (1)
82-85: Missing trailing pipe character breaks table formatting.Line 82 is missing the closing
|at the end of the row, which will break the markdown table rendering.📝 Proposed fix
-| simulationRepulsionFromMouse | Repulsion from the mouse pointer force coefficient. The repulsion force is activated by pressing the right mouse button. | 0.0 – 5.0 | `2.0` +| simulationRepulsionFromMouse | Repulsion from the mouse pointer force coefficient. The repulsion force is activated by pressing the right mouse button. | 0.0 – 5.0 | `2.0` |src/index.ts (1)
1266-1276: Adddevice.submit()after running the simulation stepWhen
step()is called manually outside the render loop, GPU commands fromrunSimulationStep(true)are queued but never submitted. Modulerun()methods andpoints.updatePosition()create and end render passes without callingsubmit(). Only theframe()method callsdevice.submit()at the end. For manualstep()calls to execute GPU work, add submission:Fix
public step (): void { if (this._isDestroyed) return if (this.ensureDevice(() => this.step())) return if (!this.config.enableSimulation) return if (!this.store.pointsTextureSize) return // Run one simulation step, forcing execution regardless of isSimulationRunning this.runSimulationStep(true) + this.device?.submit() }
🤖 Fix all issues with AI agents
In `@src/index.ts`:
- Around line 33-35: The constructor currently accepts an optional devicePromise
but the class always destroys the resolved Device; add an ownership flag (e.g.,
this.ownsDevice) set true only when Graph creates the device (the path that
constructs/awaits a new Device) and false when a devicePromise is supplied, then
change the initialization callback that currently destroys the Device when
this._isDestroyed is true (referencing device, deviceInitPromise,
this._isDestroyed) to only call device.destroy() if this.ownsDevice is true, and
similarly update the destroy method (the class destroy function) to
conditionally call device.destroy() only when this.ownsDevice is true so
externally supplied/shared devices are not torn down.
♻️ Duplicate comments (2)
src/index.ts (2)
690-697: Clear passes are never submitted (render/destroy)Line 690-697 and Line 1331-1338 end render passes without
device.submit(). luma.gl requires a submit to flush commands; otherwise the clear may not execute. This was raised earlier and still looks unresolved.✅ Add submit after clear passes
if (this.device) { const clearPass = this.device.beginRenderPass({ clearColor: this.store.backgroundColor, clearDepth: 1, clearStencil: 0, }) clearPass.end() + this.device.submit() }if (this.device) { const clearPass = this.device.beginRenderPass({ clearColor: this.store.backgroundColor, clearDepth: 1, clearStencil: 0, }) clearPass.end() + this.device.submit() this.device.destroy() }Also applies to: 1331-1339
1452-1459: Guard against non-WebGL devicesLine 1452-1457 only validates the canvas context, but the class later uses WebGL-only APIs (e.g.,
readPixelsToArrayWebGL,(device as WebGLDevice).gl). If an external non-WebGL Device is passed, this will crash at runtime. This was raised earlier and still looks unresolved.🛡️ Suggested runtime guard
private validateDevice (device: Device): NonNullable<Device['canvasContext']> { const deviceCanvasContext = device.canvasContext + if (!(device instanceof WebGLDevice)) { + throw new Error('Device must be a WebGLDevice.') + } if (deviceCanvasContext === null || deviceCanvasContext.type === 'offscreen-canvas') { throw new Error('Device must have an HTMLCanvasElement canvas context. OffscreenCanvas and compute-only devices are not supported.') } return deviceCanvasContext }
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/modules/Clusters/index.ts (1)
48-68: GuardpointsTextureSizebefore allocating arrays/textures.
Line 49 usespointsTextureSizefor array sizes and texture dimensions without checking it’s defined; this can throw (NaN sizes) ifcreate()runs before points are initialized.🛠️ Proposed fix
public create (): void { const { device, store, data } = this const { pointsTextureSize } = store + if (!pointsTextureSize) return if (data.pointsNumber === undefined || (!data.pointClusters && !data.clusterPositions)) return
🤖 Fix all issues with AI agents
In `@src/index.ts`:
- Line 729: The code calls readPixels(this.device,
this.points.currentPositionFbo) which uses the WebGL-only API
device.readPixelsToArrayWebGL(); update validateDevice() to explicitly assert
the device is a WebGL-capable device (e.g., check for the presence of
readPixelsToArrayWebGL or a WebGL-specific type flag) and throw a clear error if
not, so functions like readPixels and operations referencing
points.currentPositionFbo fail fast with a descriptive message rather than at
runtime.
♻️ Duplicate comments (7)
src/modules/ForceManyBody/index.ts (1)
87-91: Fix destruction order: destroy framebuffer before texture.Lines 89-90 destroy the texture before the framebuffer, but luma.gl requires framebuffers to be destroyed before their attached textures to avoid use-after-free. The same issue exists at lines 117-118. The
destroy()method correctly documents this order.Proposed fix
// Destroy old resources if size changed if (existingTarget) { - if (!existingTarget.texture.destroyed) existingTarget.texture.destroy() - if (!existingTarget.fbo.destroyed) existingTarget.fbo.destroy() + if (!existingTarget.fbo.destroyed) existingTarget.fbo.destroy() + if (!existingTarget.texture.destroyed) existingTarget.texture.destroy() }Also fix lines 117-118:
for (const [level, target] of Array.from(this.levelTargets.entries())) { if (level >= this.levels) { - if (!target.texture.destroyed) target.texture.destroy() - if (!target.fbo.destroyed) target.fbo.destroy() + if (!target.fbo.destroyed) target.fbo.destroy() + if (!target.texture.destroyed) target.texture.destroy() this.levelTargets.delete(level) } }src/index.ts (5)
1452-1460: Insufficient device type validation.As noted earlier, this validation only checks canvas context type but doesn't verify the device is a
WebGLDevice, which is required for WebGL-specific operations used elsewhere (e.g.,.gl.getParameter(),readPixelsToArrayWebGL()).
690-697: Missingdevice.submit()after clear pass.The luma.gl pipeline requires calling
device.submit()afterrenderPass.end()to flush GPU commands. TherenderFrame()method correctly follows this pattern (line 1654), but this clear pass does not, so the clear may not execute.🔧 Proposed fix
if (this.device) { const clearPass = this.device.beginRenderPass({ clearColor: this.store.backgroundColor, clearDepth: 1, clearStencil: 0, }) clearPass.end() + this.device.submit() }
1331-1340: Missingdevice.submit()beforedevice.destroy().Similar to the
render()method, the clear pass here ends without callingdevice.submit(). While the subsequentdevice.destroy()might implicitly flush pending work, explicitly callingsubmit()ensures the clear operation completes before destruction.🔧 Proposed fix
if (this.device) { // Clears the canvas after particle system is destroyed const clearPass = this.device.beginRenderPass({ clearColor: this.store.backgroundColor, clearDepth: 1, clearStencil: 0, }) clearPass.end() + this.device.submit() this.device.destroy() }
197-198: WebGLDevice cast without runtime validation.The code casts
devicetoWebGLDeviceto access.gl.getParameter(), butvalidateDevice()only checks for HTMLCanvasElement context, not whether the device is actually a WebGLDevice. If an external caller provides a WebGPU device viadevicePromise, this cast will fail at runtime.🛠️ Add WebGLDevice check in validateDevice
private validateDevice (device: Device): NonNullable<Device['canvasContext']> { const deviceCanvasContext = device.canvasContext + if (!(device instanceof WebGLDevice)) { + throw new Error('Cosmos requires a WebGLDevice. WebGPU devices are not supported.') + } // Cosmos requires an HTMLCanvasElement canvas context. // OffscreenCanvas and compute-only devices are not supported. if (deviceCanvasContext === null || deviceCanvasContext.type === 'offscreen-canvas') { throw new Error('Device must have an HTMLCanvasElement canvas context. OffscreenCanvas and compute-only devices are not supported.') } return deviceCanvasContext }
86-104: External device ownership not tracked — may destroy shared devices.The constructor accepts an optional
devicePromise(line 89), but there's no tracking of whether the device was externally supplied. Both the initialization callback (line 102) anddestroy()method (line 1339) unconditionally calldevice.destroy(), which can tear down a shared device the caller still needs.🛠️ Suggested ownership guard
class Graph { private device: Device | undefined private deviceInitPromise: Promise<Device> + private ownsDevice = true public constructor ( div: HTMLDivElement, config?: GraphConfigInterface, devicePromise?: Promise<Device> ) { if (config) this.config.init(config) if (devicePromise) { this.deviceInitPromise = devicePromise + this.ownsDevice = false } else { const canvas = document.createElement('canvas') this.deviceInitPromise = this.createDevice(canvas) } this.deviceInitPromise.then(device => { if (this._isDestroyed) { - device.destroy() + if (this.ownsDevice) device.destroy() return device } // ... }) }Then in
destroy():if (this.device) { const clearPass = this.device.beginRenderPass({ ... }) clearPass.end() - this.device.destroy() + if (this.ownsDevice) this.device.destroy() }src/modules/Clusters/index.ts (1)
384-402: Verify size-stability inrun().
IfpointsTextureSize,clustersTextureSize, orclusterCountcan change betweencreate()andrun(), consider an early-return guard to avoid using stale textures.
🧹 Nitpick comments (2)
src/modules/ForceMouse/index.ts (1)
92-111: Consider consistent destroyed-state checks indestroy().The
vertexCoordBufferdestruction includes a!this.vertexCoordBuffer.destroyedguard (line 108), but this pattern isn't applied torunCommandoruniformStore. For defensive consistency against double-destroy scenarios, consider applying the same pattern to all resources:♻️ Suggested consistent pattern
public destroy (): void { // 1. Destroy Models FIRST (they destroy _gpuGeometry if exists, and _uniformStore) - this.runCommand?.destroy() + if (this.runCommand && !this.runCommand.destroyed) { + this.runCommand.destroy() + } this.runCommand = undefined // ... // 4. Destroy UniformStores (Models already destroyed their managed uniform buffers) - this.uniformStore?.destroy() + if (this.uniformStore && !this.uniformStore.destroyed) { + this.uniformStore.destroy() + } this.uniformStore = undefined // 5. Destroy Buffers (passed via attributes - NOT owned by Models, must destroy manually) if (this.vertexCoordBuffer && !this.vertexCoordBuffer.destroyed) { this.vertexCoordBuffer.destroy() } this.vertexCoordBuffer = undefined }src/modules/ForceManyBody/index.ts (1)
252-254: Consider adding explicit usage flags for consistency.The
forceVertexCoordBufferis created without explicitusageflags, unlikepointIndicesat line 162-165 which specifiesusage: Buffer.VERTEX | Buffer.COPY_DST. While luma.gl may use sensible defaults, adding explicit flags improves consistency and clarity.Suggested change
this.forceVertexCoordBuffer ||= device.createBuffer({ data: new Float32Array([-1, -1, 1, -1, -1, 1, 1, 1]), + usage: Buffer.VERTEX, })
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@package.json`:
- Around line 83-85: Update the three luma.gl dependency versions in
package.json to the published 9.1.9 release: replace "@luma.gl/core": "^9.2.5",
"@luma.gl/engine": "^9.2.5", and "@luma.gl/webgl": "^9.2.5" with
"@luma.gl/core": "^9.1.9", "@luma.gl/engine": "^9.1.9", and "@luma.gl/webgl":
"^9.1.9" respectively so npm can resolve the packages; ensure the changes are
applied to the dependency entries that reference these package names.
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/index.ts (1)
1292-1354: Device cleanup logic is correct, but missingdevice.submit()before destroy.The ownership check with
shouldDestroyDeviceis properly implemented. However, the clear pass at lines 1346-1351 is missing thedevice.submit()call beforedevice.destroy().🔧 Proposed fix
if (this.shouldDestroyDevice) { // Clears the canvas after particle system is destroyed const clearPass = this.device.beginRenderPass({ clearColor: this.store.backgroundColor, clearDepth: 1, clearStencil: 0, }) clearPass.end() + this.device.submit() this.device.destroy() }
🤖 Fix all issues with AI agents
In `@src/stories/beginners/pinned-points/index.ts`:
- Around line 71-76: The destroy function currently destroys the device directly
via devicePromise.then(device => device.destroy()), but must first call
graph.destroy() to let the Graph release GPU resources; update the destroy
function to wait for devicePromise, call graph.destroy() (or graph?.destroy())
before calling device.destroy(), and ensure any promise ordering handles errors
so Graph cleanup runs prior to device.destroy().
♻️ Duplicate comments (2)
src/index.ts (2)
701-708: Missingdevice.submit()after the clear render pass.The luma.gl command pipeline requires calling
device.submit()afterrenderPass.end()to flush commands to the GPU. TherenderFrame()method at line 1669 correctly follows this pattern, but this clear pass inrender()is missing the submit call.🔧 Proposed fix
if (this.device) { const clearPass = this.device.beginRenderPass({ clearColor: this.store.backgroundColor, clearDepth: 1, clearStencil: 0, }) clearPass.end() + this.device.submit() }
1467-1475: Add runtime check to validate devices are WebGL-capable.The
readPixelshelper (insrc/helper.ts) callsdevice.readPixelsToArrayWebGL(), and line 208 casts toWebGLDeviceto access.gl. If an external caller passes a non-WebGL device viadevicePromise, these will fail at runtime. The internalcreateDevice()always creates a WebGL device, but external devices bypass this guarantee.🔧 Suggested fix to add WebGL validation
private validateDevice (device: Device): NonNullable<Device['canvasContext']> { const deviceCanvasContext = device.canvasContext + // Cosmos requires a WebGL device for readPixels and GL parameter queries + if (!(device instanceof WebGLDevice)) { + throw new Error('Device must be a WebGLDevice. WebGPU devices are not currently supported.') + } // Cosmos requires an HTMLCanvasElement canvas context. // OffscreenCanvas and compute-only devices are not supported. if (deviceCanvasContext === null || deviceCanvasContext.type === 'offscreen-canvas') { throw new Error('Device must have an HTMLCanvasElement canvas context. OffscreenCanvas and compute-only devices are not supported.') } return deviceCanvasContext }
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
package.json (1)
83-98: Add missing peer dependency@luma.gl/shadertools.
@luma.gl/engine@9.2.6declares@luma.gl/shadertools@~9.2.0as a peer dependency, but it is not listed inpackage.json. Add"@luma.gl/shadertools": "^9.2.6"to dependencies to satisfy the peer requirement and avoid runtime issues.
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@src/index.ts`:
- Around line 1836-1859: findHoveredItem currently skips detection when the
mouse hasn’t moved, which lets hover state stick while the scene animates;
update findHoveredItem to also detect scene movement and bypass the
MIN_MOUSE_MOVEMENT_THRESHOLD in that case by: add a sceneMoved check (e.g.
compare current scene/viewport transform/scale/translation — introduce/compare
this._viewportTransform or this._sceneTransform with a
this._lastCheckedViewportTransform/_lastCheckedSceneTransform), treat sceneMoved
as equivalent to mouseMoved in the early return condition, and after detection
update the last-checked scene transform fields and reset
this._shouldForceHoverDetection as you already do; ensure
hoveredPoint/hoveredLinkIndex get recomputed when sceneMoved so
mouseover/mouseout callbacks fire correctly.
In `@src/modules/Store/index.ts`:
- Around line 201-217: The adjustSpaceSize method uses webglMaxTextureSize
without validating it, which can produce NaN for this.adjustedSpaceSize; add a
guard in adjustSpaceSize to ensure webglMaxTextureSize is a finite number > 0
(e.g., Number.isFinite(webglMaxTextureSize) && webglMaxTextureSize > 0) and if
not, set a safe fallback (use defaultConfigValues.spaceSize or minSpaceSize)
before applying the WebGL-limits branch; update the branch that computes
this.adjustedSpaceSize (and its warning message) to use the validated/fallback
value so scale calculations downstream cannot become NaN.
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@src/modules/Store/index.ts`:
- Around line 201-224: The webglMaxTextureSize handling in adjustSpaceSize can
be smaller than minSpaceSize (2) causing adjustedSpaceSize to exceed the WebGL
limit; update the validation to treat webglMaxTextureSize < minSpaceSize as
invalid and skip the WebGL-limit branch. In adjustSpaceSize, either change the
existing check for webglMaxTextureSize (currently using Number.isFinite(...) ||
<= 0) to require webglMaxTextureSize >= minSpaceSize, or add an explicit branch
that warns when webglMaxTextureSize < minSpaceSize and sets
this.adjustedSpaceSize = configSpaceSize (and returns) so the later clamp using
webglMaxTextureSize never produces a value larger than the WebGL limit.
- Around line 27-32: The Mat4Array type alias is used as the return type of the
public getter transformationMatrix4x4 but is not exported, causing "private name
in public API" errors; fix this by exporting the type alias (make Mat4Array
exported) so the public API references an exported type (ensure the declaration
export applies to the Mat4Array type alias).
Signed-off-by: Stukova Olya <stukova.o@gmail.com>
Signed-off-by: Stukova Olya <stukova.o@gmail.com>
Signed-off-by: Stukova Olya <stukova.o@gmail.com>
Signed-off-by: Stukova Olya <stukova.o@gmail.com>
Signed-off-by: Stukova Olya <stukova.o@gmail.com>
Signed-off-by: Stukova Olya <stukova.o@gmail.com>
Signed-off-by: Stukova Olya <stukova.o@gmail.com>
Signed-off-by: Stukova Olya <stukova.o@gmail.com>
Signed-off-by: Stukova Olya <stukova.o@gmail.com>
Signed-off-by: Stukova Olya <stukova.o@gmail.com>
Signed-off-by: Stukova Olya <stukova.o@gmail.com>
Signed-off-by: Stukova Olya <stukova.o@gmail.com>
Signed-off-by: Stukova Olya <stukova.o@gmail.com>
Signed-off-by: Stukova Olya <stukova.o@gmail.com>
Signed-off-by: Stukova Olya <stukova.o@gmail.com>
Signed-off-by: Stukova Olya <stukova.o@gmail.com>
…ace size check Signed-off-by: Stukova Olya <stukova.o@gmail.com>
Signed-off-by: Stukova Olya <stukova.o@gmail.com>
Signed-off-by: Stukova Olya <stukova.o@gmail.com>
Signed-off-by: Stukova Olya <stukova.o@gmail.com>
Signed-off-by: Stukova Olya <stukova.o@gmail.com>
There was a problem hiding this comment.
Actionable comments posted: 8
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/stories/2. configuration.mdx (1)
82-82:⚠️ Potential issue | 🟡 MinorMissing trailing
|in Markdown table row — will break table rendering.The
simulationRepulsionFromMouserow is missing the closing pipe character at the end of the line, which can cause the table to render incorrectly in some Markdown parsers (including MDX/Storybook).Proposed fix
-| simulationRepulsionFromMouse | Repulsion from the mouse pointer force coefficient. The repulsion force is activated by pressing the right mouse button. | 0.0 – 5.0 | `2.0` +| simulationRepulsionFromMouse | Repulsion from the mouse pointer force coefficient. The repulsion force is activated by pressing the right mouse button. | 0.0 – 5.0 | `2.0` |
🤖 Fix all issues with AI agents
In `@src/modules/Clusters/index.ts`:
- Around line 321-341: The Model instance assigned to applyForcesCommand lacks
GPU render parameters, so writes to points.velocityFbo won't accumulate with
other forces and depth testing may incorrectly cull writes; update the Model
constructor (the applyForcesCommand creation) to include a parameters object
that explicitly enables additive blending (e.g., blend func/source-destination
set to additive like "one" + "one" or equivalent) and disables depth
testing/writes (depth: { enable: false }), and ensure color write is enabled for
the velocity target; modify the parameters alongside existing bindings/uniforms
in the applyForcesCommand setup so the cluster forces accumulate into
points.velocityFbo and depth is turned off as in other force modules.
In `@src/modules/ForceCenter/index.ts`:
- Around line 211-217: Force modules are currently clearing points.velocityFbo
individually (in ForceCenter.run and ForceManyBody.drawForces) causing
non-additive behaviour; instead clear the velocity FBO once at the start of the
simulation step (main loop) and remove clearColor from per-module
beginRenderPass calls (or pass clearColor: false) so draws accumulate. Also
update ForceCenter.runCommand (the draw command created/used in ForceCenter.run)
to enable additive blending (set appropriate blend parameters) so its output
adds to the existing velocity buffer rather than overwriting it. Ensure other
force modules use additive blending too if they intend to accumulate.
- Around line 138-162: The runCommand Model (created with new Model) is missing
additive blending settings, so its output won't accumulate with other forces;
update the runCommand's parameters object to include additive blend parameters
(e.g., set blendColorSrcFactor: 'one' and blendColorDstFactor: 'one' — mirror
the blending used by calculateCentermassCommand and ForceManyBody) while keeping
depthWriteEnabled and depthCompare as-is; locate the Model construction for
runCommand and add the same blend parameter keys used elsewhere to ensure
velocity contributions accumulate.
In `@src/modules/ForceManyBody/force-level.frag`:
- Around line 7-33: The UBO struct forceUniforms in the fragment shader has
field order level, levels, levelTextureSize, repulsion, alpha,... but the
TypeScript UniformStore/uniformTypes defines the keys as level, levels,
levelTextureSize, alpha, repulsion, spaceSize, theta, causing a std140 mismatch;
fix by reordering the fields in the GLSL struct to exactly match the TS order
(level, levels, levelTextureSize, alpha, repulsion, spaceSize, theta) so the
shader reads the same values the CPU writes (adjust the layout in forceUniforms
and keep the `#define` aliases intact).
In `@src/modules/ForceManyBody/index.ts`:
- Around line 229-250: The UniformStore UBO field order in the ForceManyBody
initializer doesn't match the struct order in force-level.frag; specifically
alpha and repulsion are swapped. In the UniformStore creation (forceUniformStore
/ UniformStore), reorder the keys in both uniformTypes and defaultUniforms to
exactly match the shader struct order used in force-level.frag: level, levels,
levelTextureSize, repulsion, alpha, spaceSize, theta (i.e., swap the positions
of repulsion and alpha in both maps).
In `@src/modules/Points/find-points-on-polygon-selection.frag`:
- Around line 72-78: The code currently casts mat4 to mat3
(mat3(transformationMatrix)) which drops the 2D translation and breaks panning;
fix by applying the full 4×4 transform: compute the 3×3 rotation/scale as mat3
transformMat3 = mat3(transformationMatrix), extract the translation as vec3
translation = transformationMatrix[3].xyz (or apply transformationMatrix *
vec4(p, 1.0) and take .xyz), and then compute vec3 final = transformMat3 * p +
translation so translation is preserved (use the transformationMatrix,
transformMat3, translation, p and final identifiers to locate the change).
In `@src/stories/2`. configuration.mdx:
- Line 78: The table row for the simulationRepulsionTheta property uses the
misspelled word "detalization"; update the description in the MDX so it reads
something like "Decreases / increases the detail level of the Many-Body force
calculations." (or use "granularity") by replacing "detalization" with "detail
level" in the simulationRepulsionTheta table cell.
In `@src/stories/beginners/remove-points/index.ts`:
- Around line 91-93: The pause button's event listener still references graph
after destroy(), causing calls on a destroyed instance; update destroy() (and
the pause button handler) so that you either remove the event listener from the
pause button or disable the button when destroy() is called, or add a guard at
the start of the pause handler that checks the graph's alive state before
calling graph.pause() / graph.start(); modify the function named destroy() to
call pauseButton.removeEventListener(...) or pauseButton.disabled = true (or set
a flag like graph = null) and update the pause handler to return early if graph
is null/destroyed so no methods are invoked on a destroyed graph instance.
🧹 Nitpick comments (12)
src/graph/utils/error-message.ts (1)
1-6: JSDoc is missing the newerrorparameter.The
@param errorentry should be added to keep the doc block in sync with the updated signature.📝 Proposed fix
/** * Creates and appends an error message element to the container * `@param` container The HTML element to append the error message to + * `@param` error The error message describing the unsupported feature(s) * `@returns` The created error div element */src/stories/create-cosmos.ts (1)
18-18:destroyis always provided but typed as optional — consider aligning the type.
destroyis unconditionally assigned and returned on line 71, yet the return type on line 18 marks it asdestroy?: () => void. This won't cause runtime issues but is slightly misleading to consumers.Also note that
create-story.ts(lines 16-19) calls bothd.args.destroy?.()andd.args.graph?.destroy(). Since thisdestroysimply delegates tograph.destroy(), the graph will be destroyed twice. It's safe thanks to the_isDestroyedguard inGraph.destroy(), but worth being aware of as more cleanup logic is added.Also applies to: 67-71
src/modules/ForceManyBody/force-centermass.frag (1)
8-13: Nit: The UBO instance nameforceCentervs block nameforceCenterUniforms— verify naming convention consistency with other shaders in this PR.Other ported shaders in this repo may use a different naming convention for the UBO block and instance names. Worth a quick check for consistency across the module.
#!/bin/bash # Check naming patterns for uniform blocks across the ported shaders rg -n "layout(std140)" --type=glsl -C 2 || rg -n "layout.std140." -g "*.frag" -g "*.vert" -C 2src/modules/Points/draw-highlighted.vert (1)
99-115: The#ifdef USE_UNIFORM_BUFFERS/#elsebranches here are identical—remove the guard.Both branches execute the exact same code (
isDarkenGreyout > 0.0check with identicalmixcalls). The preprocessor split adds no value and is likely a leftover from whenisDarkenGreyoutwas aboolin the non-UBO path.♻️ Suggested simplification
- `#ifdef` USE_UNIFORM_BUFFERS - if (isDarkenGreyout > 0.0) { - // Darken the color - rgbColor = mix(rgbColor, vec3(0.2), blendFactor); - } else { - // Lighten the color - rgbColor = mix(rgbColor, max(backgroundColor.rgb, vec3(0.8)), blendFactor); - } - `#else` if (isDarkenGreyout > 0.0) { // Darken the color rgbColor = mix(rgbColor, vec3(0.2), blendFactor); } else { // Lighten the color rgbColor = mix(rgbColor, max(backgroundColor.rgb, vec3(0.8)), blendFactor); } - `#endif`src/modules/Points/draw-highlighted.frag (1)
6-29: Full UBO block in fragment shader is necessary for binding consistency but could use a shared include.The entire
drawHighlightedUniformsblock is duplicated verbatim between the vertex and fragment shaders, but onlywidthis consumed here. This is correct for std140 block matching, but if the block definition ever changes, both files must be updated in lockstep. Consider extracting the block into a shared GLSL snippet/include if your shader infrastructure supports it.src/modules/Points/find-points-on-area-selection.frag (1)
1-4:#ifdef GL_ESis always true in#version 300 es— the guard is redundant but harmless.With
#version 300 es, the shader is always running in an ES context, soGL_ESis always defined. The#ifdefis a no-op. Not a bug, but could be cleaned up for clarity.src/modules/Points/find-points-on-polygon-selection.frag (1)
20-20:polygonPathLengthstored asfloatand cast toint— consider usingintdirectly.std140 supports
intnatively. Usingfloatand casting withint(...)in the macro works but introduces a subtle precision risk for large polygon lengths (floats lose integer precision above 2²⁴). Anintfield would be more direct, though the 2048 loop cap makes this a non-issue in practice.src/modules/ForceLink/index.ts (1)
261-300: Well-documented destruction with correct ordering.The explicit destruction order (Models → Textures → UniformStores → Buffers) is sound and well-commented. Each resource is guarded with
!resource.destroyedbefore calling.destroy(), and references are set toundefinedafterward to prevent double-destroy.Consider also resetting the tracking fields (
previousMaxPointDegree,previousPointsTextureSize,previousLinksTextureSize) and clearing the CPU-sideFloat32Arrayreferences (linkFirstIndicesAndAmount,indices) to release memory eagerly, in case the instance is held longer than expected after destruction.♻️ Suggested addition at the end of destroy()
this.vertexCoordBuffer = undefined + + // 6. Reset tracking state and CPU-side buffers + this.previousMaxPointDegree = undefined + this.previousPointsTextureSize = undefined + this.previousLinksTextureSize = undefined + this.linkFirstIndicesAndAmount = new Float32Array() + this.indices = new Float32Array() + this.maxPointDegree = 0 }src/modules/Store/index.ts (1)
201-205: Inconsistent use ofisFinitevsNumber.isFinite.Line 202 uses the global
isFinite()while line 214 usesNumber.isFinite(). The global variant coerces its argument (e.g.,isFinite("123")→true), though since the parameter is typed asnumberthis is unlikely to cause an issue in practice. UsingNumber.isFiniteconsistently is marginally safer.Suggested fix
- if (configSpaceSize <= 0 || !isFinite(configSpaceSize)) { + if (configSpaceSize <= 0 || !Number.isFinite(configSpaceSize)) {src/index.ts (1)
1664-1697: Drag operations inside an active render pass may cause issues.
this.points?.drag()andthis.points?.trackPoints()(lines 1689–1692) likely begin their own render passes to off-screen FBOs whiledrawRenderPassis still active. In the WebGL backend this works because render passes are thin wrappers, but it's semantically incorrect and will break if luma.gl enforces stricter render pass semantics (e.g., WebGPU).Consider moving the drag block after
drawRenderPass.end()and beforethis.device.submit():♻️ Suggested reorder
this.points?.draw(drawRenderPass) + drawRenderPass.end() + if (this.dragInstance.isActive) { - // To prevent the dragged point from suddenly jumping, run the drag function twice this.points?.drag() this.points?.drag() - // Update tracked positions after drag, even when simulation is disabled this.points?.trackPoints() } - drawRenderPass.end() this.device.submit()src/modules/Points/draw-points.vert (2)
154-170: Identical code duplicated in#ifdef/#elsebranches.Lines 154–170 contain the exact same
isDarkenGreyoutlogic in both theUSE_UNIFORM_BUFFERSand non-UBO branches. The only difference between the two paths is the uniform declaration style—once insidemain(), both paths reference the same identifiers (via#definemacros in the UBO path). This block can safely live outside the#ifdef.The same applies to the
imageAtlasUVblock at lines 174–202.Simplification for the isDarkenGreyout block
- `#ifdef` USE_UNIFORM_BUFFERS - if (isDarkenGreyout > 0.0) { - // Darken the color - shapeColor.rgb = mix(shapeColor.rgb, vec3(0.2), blendFactor); - } else { - // Lighten the color - shapeColor.rgb = mix(shapeColor.rgb, max(backgroundColor.rgb, vec3(0.8)), blendFactor); - } - `#else` if (isDarkenGreyout > 0.0) { // Darken the color shapeColor.rgb = mix(shapeColor.rgb, vec3(0.2), blendFactor); } else { // Lighten the color shapeColor.rgb = mix(shapeColor.rgb, max(backgroundColor.rgb, vec3(0.8)), blendFactor); } - `#endif`
174-202: Remove redundant#ifdef/#elsearoundimageAtlasUVcalculation.Both branches (lines 175–187 and 189–201) are identical. Since the
#definemacros make UBO fields accessible via the same names as the plain uniforms, the runtime code is the same in both paths.Consolidation
- `#ifdef` USE_UNIFORM_BUFFERS - if (hasImages <= 0.0 || imageIndex < 0.0 || imageIndex >= imageCount) { - imageAtlasUV = vec4(-1.0); - } else { - float atlasCoordIndex = imageIndex; - float texX = mod(atlasCoordIndex, imageAtlasCoordsTextureSize); - float texY = floor(atlasCoordIndex / imageAtlasCoordsTextureSize); - vec2 atlasCoordTexCoord = (vec2(texX, texY) + 0.5) / imageAtlasCoordsTextureSize; - vec4 atlasCoords = texture(imageAtlasCoords, atlasCoordTexCoord); - imageAtlasUV = atlasCoords; - } - `#else` if (hasImages <= 0.0 || imageIndex < 0.0 || imageIndex >= imageCount) { imageAtlasUV = vec4(-1.0); } else { float atlasCoordIndex = imageIndex; float texX = mod(atlasCoordIndex, imageAtlasCoordsTextureSize); float texY = floor(atlasCoordIndex / imageAtlasCoordsTextureSize); vec2 atlasCoordTexCoord = (vec2(texX, texY) + 0.5) / imageAtlasCoordsTextureSize; vec4 atlasCoords = texture(imageAtlasCoords, atlasCoordTexCoord); imageAtlasUV = atlasCoords; } - `#endif`
Signed-off-by: Stukova Olya <stukova.o@gmail.com>
Signed-off-by: Stukova Olya <stukova.o@gmail.com>
|
Successfully ported to luma.gl — ready to use. The rendering stack has been moved to luma.gl and WebGL 2. All GPU resources now use luma.gl Framebuffers, Textures, and Buffers, and the engine runs on WebGL 2 (no longer WebGL 1). Shaders have been updated for WebGL 2 and GLSL ES 3. The constructor also supports an external luma.gl device via an optional Migration note: Cosmos no longer depends on the OES_texture_float extension. In WebGL 2, float textures are part of the core spec, so the graph runs on any WebGL 2–capable device (including modern Android) without that limitation. One breaking change: The quadtree many-body module and its config options New beta published: 3.0.0-beta.0 |
3 participants
Changes
reglwithluma.gl/coreandluma.gl/engineregl.Framebuffer2D, Texture2D, Bufferwith luma.glFramebuffer, Texture, BufferModelOf course nothing builds at this point, additional passes will be needed.
Summary by CodeRabbit
New Features
Improvements
Bug Fixes
Removed