Link sampling #195
Merged
Merged
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds GPU-backed link sampling: new config option and default, shader modules and FBO pipeline in Lines, Graph APIs to read sampled link midpoints/angles, story/demo assets (data, labels, CSS), and a devDependency bump. Changes
Sequence Diagram(s)sequenceDiagram
participant User as User/App
participant Graph as Graph API
participant Lines as Lines Module
participant GPU as GPU/Framebuffer
participant Parser as Readback Parser
User->>Graph: graph.getSampledLinks()
Graph->>Lines: getSampledLinks()
Note over Lines: guard destroyed & lines present
Lines->>GPU: bind sampledLinksFbo & run fillSampledLinksFboCommand
GPU->>GPU: vertex shader fetch A,B, compute mid (curve opt.), transform
GPU->>GPU: fragment shader write RGBA (x,y,angle,index)
GPU->>Lines: readPixels(sampledLinksFbo)
Lines->>Parser: parse pixels -> indices, positions, angles
Parser-->>Graph: return sampled arrays
Graph-->>User: deliver sampled link data
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (4)
src/modules/Lines/fill-sampled-links.frag (1)
2-4:#ifdef GL_ESguard is redundant and inconsistent with other shaders in this codebase.
GL_ESis always defined in any GLSL ES context, so the guard is a no-op. Other shaders (e.g.,force-spring.ts'sforceFrag) declareprecision highp float;unconditionally. The shader is functionally correct either way.♻️ Proposed cleanup
`#version` 300 es -#ifdef GL_ES precision highp float; -#endif🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/modules/Lines/fill-sampled-links.frag` around lines 2 - 4, Remove the redundant GL_ES guard in the fragment shader and declare precision unconditionally: delete the "#ifdef GL_ES" and matching "#endif" lines and leave a single "precision highp float;" line at the top of the fill-sampled-links.frag shader so it matches other shaders like forceFrag; ensure no other conditional directives remain around the precision statement.src/stories/beginners/link-sampling/index.ts (1)
26-45: Labels are not updated on initial render — they only appear after the first zoom/drag event.The
onZoom/onDragEndcallbacks will update labels, but there's no explicitlinkLabels.update(graph)aftergraph.render(). SincefitViewOnInit(default: true) triggers a zoom transition, labels will appear after that animation completes (~500ms). If the user disablesfitViewOnInitor setsinitialZoomLevel, labels would remain invisible until a manual zoom/drag.Consider adding an explicit initial update, e.g. via the
onSimulationTickoronZoomEndcallback, or simply callinglinkLabels.update(graph)after render once the graph is ready. Since this is a demo story, it's a minor polish item.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/stories/beginners/link-sampling/index.ts` around lines 26 - 45, Labels aren't updated on initial render because only onZoom/onDragEnd call linkLabels.update(graph); ensure labels are updated immediately after the graph is ready by invoking linkLabels.update(graph) once after graph.render() (or attach it to a post-render callback such as onZoomEnd/onSimulationTick if available) so that label positions are correct regardless of fitViewOnInit or initialZoomLevel; reference the existing graph.render(), linkLabels.update(graph), and the config onZoom/onDragEnd handlers when making the change.src/modules/Lines/index.ts (1)
688-780: Significant duplication betweengetSampledLinkPositionsMapandgetSampledLinks.Lines 694-717 and 741-764 are nearly identical — both set uniforms, set bindings, run the render pass, clear with
[-1,-1,-1,-1], draw, end, and read pixels. The only difference is how the pixel data is consumed afterward.Extract the shared GPU work into a private helper that returns the raw pixel array, and have both public methods just interpret the results differently.
♻️ Proposed refactor
+ private fillAndReadSampledLinks (): Float32Array | null { + if (!this.sampledLinksFbo || this.sampledLinksFbo.destroyed) return null + const points = this.points + if (!points?.currentPositionTexture || points.currentPositionTexture.destroyed) return null + if (!this.fillSampledLinksFboCommand || !this.fillSampledLinksUniformStore) return null + + this.fillSampledLinksFboCommand.setVertexCount(this.data.linksNumber ?? 0) + this.fillSampledLinksUniformStore.setUniforms({ + fillSampledLinksUniforms: { + pointsTextureSize: this.store.pointsTextureSize ?? 0, + transformationMatrix: this.store.transformationMatrix4x4, + spaceSize: this.store.adjustedSpaceSize ?? 0, + screenSize: ensureVec2(this.store.screenSize, [0, 0]), + curvedWeight: this.config.curvedLinkWeight ?? 0, + curvedLinkControlPointDistance: this.config.curvedLinkControlPointDistance ?? 0, + curvedLinkSegments: this.config.curvedLinks ? this.config.curvedLinkSegments ?? defaultConfigValues.curvedLinkSegments : 1, + }, + }) + this.fillSampledLinksFboCommand.setBindings({ + positionsTexture: points.currentPositionTexture, + }) + + const fillPass = this.device.beginRenderPass({ + framebuffer: this.sampledLinksFbo, + clearColor: [-1, -1, -1, -1], + }) + this.fillSampledLinksFboCommand.draw(fillPass) + fillPass.end() + + return readPixels(this.device, this.sampledLinksFbo) + } + public getSampledLinkPositionsMap (): Map<number, [number, number, number]> { const positions = new Map<number, [number, number, number]>() - if (!this.sampledLinksFbo || this.sampledLinksFbo.destroyed) return positions - const points = this.points - if (!points?.currentPositionTexture || points.currentPositionTexture.destroyed) return positions - - if (this.fillSampledLinksFboCommand && this.fillSampledLinksUniformStore && this.sampledLinksFbo) { - // ... ~25 lines of GPU work ... - } - - const pixels = readPixels(this.device, this.sampledLinksFbo) + const pixels = this.fillAndReadSampledLinks() + if (!pixels) return positions for (let i = 0; i < pixels.length / 4; i++) { // ... existing pixel interpretation ... } return positions } public getSampledLinks (): { indices: number[]; positions: number[]; angles: number[] } { const indices: number[] = [] const positions: number[] = [] const angles: number[] = [] - if (!this.sampledLinksFbo || this.sampledLinksFbo.destroyed) return { indices, positions, angles } - const points = this.points - if (!points?.currentPositionTexture || points.currentPositionTexture.destroyed) return { indices, positions, angles } - - if (this.fillSampledLinksFboCommand && this.fillSampledLinksUniformStore && this.sampledLinksFbo) { - // ... ~25 lines of GPU work (identical) ... - } - - const pixels = readPixels(this.device, this.sampledLinksFbo) + const pixels = this.fillAndReadSampledLinks() + if (!pixels) return { indices, positions, angles } for (let i = 0; i < pixels.length / 4; i++) { // ... existing pixel interpretation ... } return { indices, positions, angles } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/modules/Lines/index.ts` around lines 688 - 780, Both getSampledLinkPositionsMap and getSampledLinks duplicate the GPU setup/draw/read logic; extract that into a private helper (e.g., private readSampledLinksPixels(): Uint8Array) that performs the shared work: check sampledLinksFbo and points/currentPositionTexture, call fillSampledLinksFboCommand.setVertexCount(...), set uniforms on fillSampledLinksUniformStore, setBindings(positionsTexture), beginRenderPass with framebuffer sampledLinksFbo and clearColor [-1,-1,-1,-1], draw and end, then call readPixels(this.device, this.sampledLinksFbo) and return the pixel array; update getSampledLinkPositionsMap and getSampledLinks to call this helper and only contain the respective pixel-parsing logic (using indices, x, y, angle), leaving all GPU setup in the new helper.src/stories/beginners/link-sampling/labels.ts (1)
40-43: Accessinggraph.graph.linkscouples to internal API.
graph.graphexposes the internalGraphDatainstance. For a demo/story this is fine, but note that ifGraphDatais restructured later, this will break. If the library intends to support this use case more broadly, consider exposing a public getter for the raw links array on Graph.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/stories/beginners/link-sampling/labels.ts` around lines 40 - 43, The update method is directly accessing the internal GraphData via graph.graph.links which couples to internals; add or use a public accessor on Graph (e.g., getLinks() or getRawLinks()) that returns the links array and change the update method to call that instead of graph.graph.links. Locate the update(graph: Graph) method in labels.ts and replace the direct internal access with graph.getLinks() (or the existing public getter if present), and if no getter exists add a small public method on the Graph class that returns the raw links array to keep encapsulation.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/modules/Lines/conic-curve-module.ts`:
- Line 9: The variable name `divident` in the conic curve computation is
misspelled; rename it to `dividend` wherever it is declared and used (the
expression "vec2 divident = (1.0 - t) * (1.0 - t) * A + 2.0 * (1.0 - t) * t * w
* ControlPoint + t * t * B;" and any subsequent references) so all references
match the corrected identifier `dividend`; ensure you update imports/exports or
other functions that may reference that identifier to avoid undefined symbol
errors.
In `@src/modules/Lines/index.ts`:
- Around line 453-470: The updateSampledLinksGrid method can call
device.createFramebuffer with width or height 0 when screenSize is [0,0]; add an
early guard at the start of updateSampledLinksGrid to return (no-op) if computed
w === 0 || h === 0 to avoid creating a zero-sized sampledLinksFbo, leaving any
existing sampledLinksFbo intact (and still destroying it only when replacing
with a valid-sized FBO); apply the same guard change to
Points.updateSampledPointsGrid to prevent zero-sized framebuffer creation there
too.
In `@src/stories/beginners.stories.ts`:
- Line 137: The linkSampling story never updates link labels on initial render
because linkLabels.update(graph) is only bound to onZoom and onDragEnd; after
calling graph.render() (in the linkSampling function in
src/stories/beginners/link-sampling/index.ts) add an initial invocation of
linkLabels.update(graph) so labels are rendered immediately when
enableSimulation is false rather than waiting for user interaction.
---
Nitpick comments:
In `@src/modules/Lines/fill-sampled-links.frag`:
- Around line 2-4: Remove the redundant GL_ES guard in the fragment shader and
declare precision unconditionally: delete the "#ifdef GL_ES" and matching
"#endif" lines and leave a single "precision highp float;" line at the top of
the fill-sampled-links.frag shader so it matches other shaders like forceFrag;
ensure no other conditional directives remain around the precision statement.
In `@src/modules/Lines/index.ts`:
- Around line 688-780: Both getSampledLinkPositionsMap and getSampledLinks
duplicate the GPU setup/draw/read logic; extract that into a private helper
(e.g., private readSampledLinksPixels(): Uint8Array) that performs the shared
work: check sampledLinksFbo and points/currentPositionTexture, call
fillSampledLinksFboCommand.setVertexCount(...), set uniforms on
fillSampledLinksUniformStore, setBindings(positionsTexture), beginRenderPass
with framebuffer sampledLinksFbo and clearColor [-1,-1,-1,-1], draw and end,
then call readPixels(this.device, this.sampledLinksFbo) and return the pixel
array; update getSampledLinkPositionsMap and getSampledLinks to call this helper
and only contain the respective pixel-parsing logic (using indices, x, y,
angle), leaving all GPU setup in the new helper.
In `@src/stories/beginners/link-sampling/index.ts`:
- Around line 26-45: Labels aren't updated on initial render because only
onZoom/onDragEnd call linkLabels.update(graph); ensure labels are updated
immediately after the graph is ready by invoking linkLabels.update(graph) once
after graph.render() (or attach it to a post-render callback such as
onZoomEnd/onSimulationTick if available) so that label positions are correct
regardless of fitViewOnInit or initialZoomLevel; reference the existing
graph.render(), linkLabels.update(graph), and the config onZoom/onDragEnd
handlers when making the change.
In `@src/stories/beginners/link-sampling/labels.ts`:
- Around line 40-43: The update method is directly accessing the internal
GraphData via graph.graph.links which couples to internals; add or use a public
accessor on Graph (e.g., getLinks() or getRawLinks()) that returns the links
array and change the update method to call that instead of graph.graph.links.
Locate the update(graph: Graph) method in labels.ts and replace the direct
internal access with graph.getLinks() (or the existing public getter if
present), and if no getter exists add a small public method on the Graph class
that returns the raw links array to keep encapsulation.
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/modules/Lines/index.ts (1)
689-781: Extract duplicated render-and-readback logic into a private helper.
getSampledLinkPositionsMapandgetSampledLinksshare ~30 identical lines for setting uniforms, bindings, and executing the render pass. Only the post-readback loop differs. Extracting the shared render+readback into a private method (e.g.,renderAndReadSampledLinks) would eliminate the duplication and make both methods clearer.The same pattern exists in
Points(getSampledPointPositionsMap/getSampledPoints), so this is consistent with the codebase, but worth cleaning up.♻️ Sketch of the refactor
+ private renderAndReadSampledLinks (): Float32Array | undefined { + if (!this.sampledLinksFbo || this.sampledLinksFbo.destroyed) return undefined + const points = this.points + if (!points?.currentPositionTexture || points.currentPositionTexture.destroyed) return undefined + if (!this.fillSampledLinksFboCommand || !this.fillSampledLinksUniformStore) return undefined + + this.fillSampledLinksFboCommand.setVertexCount(this.data.linksNumber ?? 0) + this.fillSampledLinksUniformStore.setUniforms({ + fillSampledLinksUniforms: { + pointsTextureSize: this.store.pointsTextureSize ?? 0, + transformationMatrix: this.store.transformationMatrix4x4, + spaceSize: this.store.adjustedSpaceSize ?? 0, + screenSize: ensureVec2(this.store.screenSize, [0, 0]), + curvedWeight: this.config.curvedLinkWeight ?? 0, + curvedLinkControlPointDistance: this.config.curvedLinkControlPointDistance ?? 0, + curvedLinkSegments: this.config.curvedLinks ? this.config.curvedLinkSegments ?? defaultConfigValues.curvedLinkSegments : 1, + }, + }) + this.fillSampledLinksFboCommand.setBindings({ + positionsTexture: points.currentPositionTexture, + }) + + const fillPass = this.device.beginRenderPass({ + framebuffer: this.sampledLinksFbo, + clearColor: [-1, -1, -1, -1], + }) + this.fillSampledLinksFboCommand.draw(fillPass) + fillPass.end() + + return readPixels(this.device, this.sampledLinksFbo) + } public getSampledLinkPositionsMap (): Map<number, [number, number, number]> { const positions = new Map<number, [number, number, number]>() - if (!this.sampledLinksFbo || this.sampledLinksFbo.destroyed) return positions - const points = this.points - if (!points?.currentPositionTexture || points.currentPositionTexture.destroyed) return positions - - if (this.fillSampledLinksFboCommand && this.fillSampledLinksUniformStore && this.sampledLinksFbo) { - // ... ~20 lines of render logic ... - } - - const pixels = readPixels(this.device, this.sampledLinksFbo) + const pixels = this.renderAndReadSampledLinks() + if (!pixels) return positions for (let i = 0; i < pixels.length / 4; i++) { // ... parse loop unchanged ... } return positions }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/modules/Lines/index.ts` around lines 689 - 781, getSampledLinkPositionsMap and getSampledLinks duplicate the same uniform/binding/render-pass/readPixels sequence; extract that logic into a private helper (e.g., renderAndReadSampledLinks) that performs the early-return checks (sampledLinksFbo, points.currentPositionTexture), sets uniforms via fillSampledLinksUniformStore, sets bindings, executes the render pass, calls readPixels(this.device, this.sampledLinksFbo) and returns the pixel buffer (or null/empty). Replace the duplicated block in both public methods with a call to renderAndReadSampledLinks and keep only their distinct post-readback loops (mapping into Map vs pushing into indices/positions/angles). Ensure the helper uses the same this.fillSampledLinksFboCommand, this.fillSampledLinksUniformStore, and this.store/config fields and preserves vertexCount setup and return types.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@src/modules/Lines/index.ts`:
- Around line 453-471: The updateSampledLinksGrid change should also guard
against an empty screenSize array to avoid Math.min(...) throwing: in
updateSampledLinksGrid, compute dist using screenSize.length > 0 ?
Math.min(...screenSize) / 2 : defaultConfigValues.linkSamplingDistance (still
fallback when dist === 0), keep the early return when w === 0 || h === 0, and
preserve the existing sampledLinksFbo destruction and recreation logic
(sampledLinksFbo, device.createFramebuffer,
defaultConfigValues.linkSamplingDistance).
---
Nitpick comments:
In `@src/modules/Lines/index.ts`:
- Around line 689-781: getSampledLinkPositionsMap and getSampledLinks duplicate
the same uniform/binding/render-pass/readPixels sequence; extract that logic
into a private helper (e.g., renderAndReadSampledLinks) that performs the
early-return checks (sampledLinksFbo, points.currentPositionTexture), sets
uniforms via fillSampledLinksUniformStore, sets bindings, executes the render
pass, calls readPixels(this.device, this.sampledLinksFbo) and returns the pixel
buffer (or null/empty). Replace the duplicated block in both public methods with
a call to renderAndReadSampledLinks and keep only their distinct post-readback
loops (mapping into Map vs pushing into indices/positions/angles). Ensure the
helper uses the same this.fillSampledLinksFboCommand,
this.fillSampledLinksUniformStore, and this.store/config fields and preserves
vertexCount setup and return types.
Signed-off-by: Stukova Olya <stukova.o@gmail.com>
- Bump @interacta/css-labels to ^0.5.0 for rotation support 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>
Stukova
force-pushed
the
f/link-sampling
branch
from
6c8f793 to
3983768
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
src/stories/beginners/link-sampling/labels.ts (1)
36-36: Accessing internalgraph.graph.linkscouples story code to GraphData internals.
graph.graphis a publicGraphDatainstance, but there is no dedicated public getter likegetLinks()on theGraphclass. For a demo story this is pragmatic, but if the internal structure changes, this will silently break. Consider whether a publicgetLinks()accessor onGraphwould be worthwhile.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/stories/beginners/link-sampling/labels.ts` at line 36, The story is directly reading the internal structure via graph.graph.links which couples it to GraphData internals; add a public accessor on the Graph class (e.g., getLinks(): LinkType[] or similar) that returns the underlying links from the internal GraphData, then update the story code to call graph.getLinks() instead of accessing graph.graph.links so callers use the stable public API (locate the Graph class and implement getLinks, and update usages in labels.ts to use graph.getLinks()).src/modules/Lines/index.ts (1)
689-781: Extract shared FBO render+readback logic to reduce duplication betweengetSampledLinkPositionsMapandgetSampledLinks.Lines 695–718 and 742–765 are near-identical: both set vertex count, push the same uniforms, bind the same texture, create a render pass, draw, end, and call
readPixels. Only the post-read iteration differs. Extracting the common render+read step into a private helper (e.g.,private renderAndReadSampledLinks(): Float32Array | null) would eliminate ~25 duplicated lines and ensure both methods stay in sync when uniforms or bindings change.♻️ Sketch of proposed refactor
+ private renderSampledLinkPixels (): Float32Array | null { + if (!this.sampledLinksFbo || this.sampledLinksFbo.destroyed) return null + const points = this.points + if (!points?.currentPositionTexture || points.currentPositionTexture.destroyed) return null + + if (this.fillSampledLinksFboCommand && this.fillSampledLinksUniformStore && this.sampledLinksFbo) { + this.fillSampledLinksFboCommand.setVertexCount(this.data.linksNumber ?? 0) + this.fillSampledLinksUniformStore.setUniforms({ + fillSampledLinksUniforms: { + pointsTextureSize: this.store.pointsTextureSize ?? 0, + transformationMatrix: this.store.transformationMatrix4x4, + spaceSize: this.store.adjustedSpaceSize ?? 0, + screenSize: ensureVec2(this.store.screenSize, [0, 0]), + curvedWeight: this.config.curvedLinkWeight ?? 0, + curvedLinkControlPointDistance: this.config.curvedLinkControlPointDistance ?? 0, + curvedLinkSegments: this.config.curvedLinks ? this.config.curvedLinkSegments ?? defaultConfigValues.curvedLinkSegments : 1, + }, + }) + this.fillSampledLinksFboCommand.setBindings({ + positionsTexture: points.currentPositionTexture, + }) + + const fillPass = this.device.beginRenderPass({ + framebuffer: this.sampledLinksFbo, + clearColor: [-1, -1, -1, -1], + }) + this.fillSampledLinksFboCommand.draw(fillPass) + fillPass.end() + } + + return readPixels(this.device, this.sampledLinksFbo) + } + public getSampledLinkPositionsMap (): Map<number, [number, number, number]> { const positions = new Map<number, [number, number, number]>() - if (!this.sampledLinksFbo || this.sampledLinksFbo.destroyed) return positions - const points = this.points - if (!points?.currentPositionTexture || points.currentPositionTexture.destroyed) return positions - // ... ~25 lines of render logic ... - const pixels = readPixels(this.device, this.sampledLinksFbo) + const pixels = this.renderSampledLinkPixels() + if (!pixels) return positions for (let i = 0; i < pixels.length / 4; i++) { // ... existing iteration ... } return positions } public getSampledLinks (): { indices: number[]; positions: number[]; angles: number[] } { const indices: number[] = [] const positions: number[] = [] const angles: number[] = [] - if (!this.sampledLinksFbo || this.sampledLinksFbo.destroyed) return { indices, positions, angles } - const points = this.points - if (!points?.currentPositionTexture || points.currentPositionTexture.destroyed) return { indices, positions, angles } - // ... ~25 lines of render logic ... - const pixels = readPixels(this.device, this.sampledLinksFbo) + const pixels = this.renderSampledLinkPixels() + if (!pixels) return { indices, positions, angles } for (let i = 0; i < pixels.length / 4; i++) { // ... existing iteration ... } return { indices, positions, angles } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/modules/Lines/index.ts` around lines 689 - 781, Both getSampledLinkPositionsMap and getSampledLinks duplicate the same FBO render and readback logic (setVertexCount, fillSampledLinksUniformStore.setUniforms, setBindings, beginRenderPass/draw/end, readPixels); extract that block into a private helper (e.g., private renderAndReadSampledLinks(): Float32Array | null) that performs the checks on sampledLinksFbo and points.currentPositionTexture, invokes this.fillSampledLinksFboCommand.setVertexCount, this.fillSampledLinksUniformStore.setUniforms, this.fillSampledLinksFboCommand.setBindings, runs the render pass (beginRenderPass/draw/end) and returns the pixels from readPixels, then update getSampledLinkPositionsMap and getSampledLinks to call renderAndReadSampledLinks() and only handle the post-read iteration (parsing index/x/y/angle) locally.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/stories/2`. configuration.mdx:
- Line 61: Documentation shows linkSamplingDistance and pointSamplingDistance
defaulting to 150 but the actual defaults in src/variables.ts are 100; update
the table entries for both pointSamplingDistance and linkSamplingDistance in the
MDX (the rows around the existing `150` values) to `100` so the docs match the
constants in src/variables.ts and verify both table cells are changed.
---
Duplicate comments:
In `@src/modules/Lines/conic-curve-module.ts`:
- Around line 7-13: Rename the misspelled local variable "divident" to
"dividend" inside the conicParametricCurve function: update the declaration
(vec2 divident = ...) and any subsequent references (return divident / divisor;)
to use "dividend" so the identifier is consistent and correctly spelled in
conicParametricCurve.
In `@src/modules/Lines/index.ts`:
- Around line 453-471: The guard in updateSampledLinksGrid currently checks for
w === 0 || h === 0 but should use a non-positive check to be robust; change the
condition to w <= 0 || h <= 0 to prevent creating a framebuffer when computed
dimensions are zero or negative (refer to variables w and h in the
updateSampledLinksGrid method), and keep the rest of the early return behavior
the same.
---
Nitpick comments:
In `@src/modules/Lines/index.ts`:
- Around line 689-781: Both getSampledLinkPositionsMap and getSampledLinks
duplicate the same FBO render and readback logic (setVertexCount,
fillSampledLinksUniformStore.setUniforms, setBindings, beginRenderPass/draw/end,
readPixels); extract that block into a private helper (e.g., private
renderAndReadSampledLinks(): Float32Array | null) that performs the checks on
sampledLinksFbo and points.currentPositionTexture, invokes
this.fillSampledLinksFboCommand.setVertexCount,
this.fillSampledLinksUniformStore.setUniforms,
this.fillSampledLinksFboCommand.setBindings, runs the render pass
(beginRenderPass/draw/end) and returns the pixels from readPixels, then update
getSampledLinkPositionsMap and getSampledLinks to call
renderAndReadSampledLinks() and only handle the post-read iteration (parsing
index/x/y/angle) locally.
In `@src/stories/beginners/link-sampling/labels.ts`:
- Line 36: The story is directly reading the internal structure via
graph.graph.links which couples it to GraphData internals; add a public accessor
on the Graph class (e.g., getLinks(): LinkType[] or similar) that returns the
underlying links from the internal GraphData, then update the story code to call
graph.getLinks() instead of accessing graph.graph.links so callers use the
stable public API (locate the Graph class and implement getLinks, and update
usages in labels.ts to use graph.getLinks()).
… 150) Signed-off-by: Stukova Olya <stukova.o@gmail.com>
Signed-off-by: Stukova Olya <stukova.o@gmail.com>
rokotyan
approved these changes
Mar 2, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Adds link sampling: an API that returns a sample of the links currently visible on screen — link indices, midpoint positions, and angle per link.
getSampledLinkPositionsMap()returns a Map from link index to[x, y, angle];getSampledLinks()returns the same data as arrays:indices,positions,angles.linkSamplingDistance(default 150) controls density — larger value = fewer sampled links.Summary by CodeRabbit
New Features
Bug Fixes
Documentation
Chores