WIP Add GPU transitions#228
Open
Stukova wants to merge 18 commits into
Open
Conversation
|
Important Review skippedThis PR was authored by the user configured for CodeRabbit reviews. CodeRabbit does not review PRs authored by this user. It's recommended to use a dedicated user account to post CodeRabbit review feedback. ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthroughAdds a GPU-driven Transition system integrated into Graph, Points, Lines, and Drag; introduces per-property queued transitions with duration/easing and lifecycle callbacks; updates shaders/attributes to source/target pairs for interpolation; makes Changes
Sequence Diagram(s)sequenceDiagram
participant App as Application
participant Graph as Graph
participant Transition as Transition
participant Points as Points
participant Lines as Lines
participant GPU as GPU/Canvas
Note over App,Graph: App enqueues updates via setters
App->>Graph: setPointPositions/Colors/Sizes/Link*(...)
Graph->>Transition: queue(...) (properties)
App->>Graph: render()
Graph->>Transition: start()
Transition->>Transition: onTransitionStart()
loop per frame
Graph->>Transition: step(now)
Transition-->>Graph: progress (eased)
Graph->>Points: setTransitionProgress(progress, animateFlags)
Graph->>Lines: setTransitionProgress(progress, animateFlags)
alt positions transition active
Graph->>Points: interpolatePosition(progress)
Points->>GPU: GPU interpolation pass (interpolate-position.frag)
end
Points->>GPU: draw points (draw-points.vert mixes attrs)
Lines->>GPU: draw lines (draw-curve-line.vert mixes attrs)
GPU->>Graph: frameRendered
end
Transition->>Transition: onTransitionEnd(interrupted=false)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 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: 6
🧹 Nitpick comments (3)
src/index.ts (1)
255-257: Prefer constructor injection over post-construction field mutation.
this.points.transition = this.transitionat line 256 sets a public field onPointsimmediately after it's constructed, instead of passingtransitionthrough the constructor likeDragdoes (line 61). This pattern leaks a two-step initialization contract (consumers must remember to settransitionor the transition-aware code paths inPointssilently no-op) and forcestransitionto be a mutable public field. Passingtransitioninto thePointsconstructor alongsidedevice/config/store/graphwould be more consistent and safer.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/index.ts` around lines 255 - 257, The code mutates this.points.transition after construction which leaks a two-step init; update the Points class constructor to accept a Transition parameter (same type as this.transition), update the instantiation site here to pass this.transition into new Points(device, this.config, this.store, this.graph, this.transition) and remove the post-construction assignment (this.points.transition = this.transition); also update any other call sites and the Points type signature accordingly (follow the same pattern used by Drag) so transition is provided via constructor injection and kept immutable.src/modules/Lines/index.ts (2)
986-998: Mid-transition interruption causes a brief visual jump.When
updateAttributeBuffersis called withsameCountwhile a previous transition is still in flight, the swap makes the newsourceequal to the prior cycle's fully-populated target (i.e., the end state of the previous animation), not the currently on-screen interpolated value. Sincetransition.start()then resetsprogressto 0, the first frame after the setter snaps from the current interpolated color/width to the prior target before animating toward the new target.This is likely acceptable as an initial implementation, but worth a doc note on
setLinkColors/setLinkWidthsbehavior under rapid re-entries, or alternatively reading back the currently displayed values to seed the newsourceso the animation starts from where the eye actually is.🤖 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 986 - 998, The swap logic in updateAttributeBuffers currently reuses the prior target as the new source causing a visual snap when a previous transition is mid-flight; detect an active transition (e.g., inspect the transition object used by setLinkColors/setLinkWidths and check transition.progress is between 0 and 1) and instead seed the new source with the currently displayed/interpolated values by computing a lerp between the existing sourceBuffer data and targetBuffer data using transition.progress (or read back the currently-displayed attribute if available), then write that interpolated array into nextSource before returning so the new transition starts from the on-screen state rather than the prior target.
132-139: Pre-allocatedcolorBuffer/widthBufferare now dead and leaked.Since the
drawCurveCommandattribute bindings switched tosourceColor/targetColorandsourceWidth/targetWidth(lines 221–224), thesecolorBuffer/widthBufferpreallocations are no longer bound to any shader attribute. They have two remaining side effects, both undesirable:
- The lazy-init guards in
draw()(lines 375–376,if (!this.colorBuffer) this.updateColor()andif (!this.widthBuffer) this.updateWidth()) are now effectively dead —colorBuffer/widthBufferare always defined byinitPrograms, so these guards never trigger. Initial population relies entirely on theisLinkColor/WidthUpdateNeededflags set bysetLinks/setLinkColors/setLinkWidthsviacreate().- When
updateColor()/updateWidth()eventually run, they reassignthis.colorBuffer = target/this.widthBuffer = target(lines 596, 620) without destroying the previously-preallocated buffer, leaking one GPU buffer allocation per field until device destruction.Consider either removing the preallocation entirely and repurposing the guards to call
updateColor()/updateWidth()to initialize the source/target buffers, or droppingcolorBuffer/widthBufferfrom the class now that they're just shadow aliases oftargetColorBuffer/targetWidthBuffer.🤖 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 132 - 139, Preallocated this.colorBuffer/this.widthBuffer in initPrograms are unused and leak; remove their creation and the class-level aliases so the code only uses targetColorBuffer/targetWidthBuffer (and the shader bindings drawCurveCommand/sourceColor/targetColor and sourceWidth/targetWidth). Update draw() to stop checking the dead guards (if (!this.colorBuffer) / if (!this.widthBuffer)) and instead rely on the existing isLinkColor/WidthUpdateNeeded flags and updateColor()/updateWidth() to initialize or refresh targetColorBuffer/targetWidthBuffer (ensure create()/setLinks/setLinkColors/setLinkWidths still set those flags), and delete any assignments that reassign colorBuffer/widthBuffer without destroying the previous buffer; if you prefer keeping aliases, ensure updateColor()/updateWidth() destroy the old GPU buffer before reassigning to avoid leaks.
🤖 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/index.ts`:
- Around line 740-742: The guard currently calls transition.abort() when
this.transition.isPending && !this.points?.currentPositionTexture, but abort()
clears the whole queue (dropping unrelated pending updates); change this to only
remove or skip pending Positions updates: add/use a narrow Transition API (e.g.,
Transition.isPendingFor('Positions') or Transition.dequeue('Positions')) or
implement logic in the caller to inspect the pending queue and remove only the
Positions entry when currentPositionTexture is missing; update the check around
this.transition.isPending and this.points?.currentPositionTexture to call the
new targeted method instead of transition.abort(), and adjust Transition
(methods like isPendingFor, dequeue, or removePendingByType) to support
selective removal without clearing other pending/active cycles.
In `@src/modules/Drag/index.ts`:
- Around line 12-14: Update the subject((event) => ...) drag gate to also block
when a transition is pending: in the callback currently checking
this.transition.isActive, extend the check to include the transition's pending
flag (e.g., this.transition.isPending or this.transition.pending depending on
the Transition API you use) so the early-return becomes something like if
(this.transition.isActive || this.transition.isPending) return undefined; keep
the existing checks for this.store.hoveredPoint and
!this.store.isSpaceKeyPressed unchanged.
In `@src/modules/Points/index.ts`:
- Around line 2348-2369: interpolatePosition writes updated GPU positions into
currentPositionFbo but does not invalidate CPU/consumer caches, so after the
render pass you must mark position-dependent caches stale; after
renderPass.end() in interpolatePosition(), call cache invalidation methods such
as this.invalidateTrackedPositionsCache() and
this.invalidateClusterCentroidsCache() (or set flags like
this.trackedPositionsStale = true and this.clusterCentroidsStale = true if you
use boolean invalidation), ensuring any consumers/readbacks check those
methods/flags and recompute from this.currentPositionFbo when stale; add the
corresponding methods/flag checks elsewhere (e.g., where tracked positions or
centroids are read) to trigger a refresh.
- Around line 939-950: The code currently falls back to zero-filled arrays
causing transparent/zero-size points; change the fallbacks in the color and size
preparation to use the module's configured defaults instead of zeros: when
building colorData use data.pointColors ?? new Float32Array((data.pointsNumber
?? 0) * 4).fill(<use module default RGBA, e.g. this.pointColor or
this.config.pointColor>) and when building sizeData (the block around
updateAttributeBuffers for sizes at ~1047) use data.pointSizes ?? new
Float32Array(data.pointsNumber ?? 0).fill(<use module default size, e.g.
this.pointSize or this.config.pointSize>); keep using updateAttributeBuffers,
sourceColorBuffer, targetColorBuffer, previousColorData and colorBuffer (and the
analogous size buffers) but replace the zero-fills with the canonical defaults
so missing attributes render with visible defaults.
- Around line 2190-2213: The resize/recreate branches in
createOrUpdateVelocityTexture(), createOrUpdateSimulationResources(), and
createOrUpdatePositionTextures() currently destroy textures before their
attached framebuffers (e.g., velocityTexture then velocityFbo), which violates
the documented destruction order; update each branch to destroy the framebuffer
(e.g., velocityFbo) first if it exists and is not destroyed, then destroy the
texture (e.g., velocityTexture) to match "Framebuffers → Textures" ordering, and
apply the same swap for all similarly named pairs (positionFbo/positionTexture,
any simulationFbo/simulationTexture) throughout the function bodies.
In `@src/stories/3`. api-reference.mdx:
- Around line 59-61: The sentence about enableSimulation is ambiguous about
whether it is part of the "reset to defaults when set to undefined" rule; reword
the paragraph to clarify contrast with setConfig-init-only fields by explicitly
stating that properties set to undefined are reset to defaults, and separately
note that enableSimulation can be toggled at runtime without recreating the
graph instance (reference enableSimulation and setConfig to locate the text).
---
Nitpick comments:
In `@src/index.ts`:
- Around line 255-257: The code mutates this.points.transition after
construction which leaks a two-step init; update the Points class constructor to
accept a Transition parameter (same type as this.transition), update the
instantiation site here to pass this.transition into new Points(device,
this.config, this.store, this.graph, this.transition) and remove the
post-construction assignment (this.points.transition = this.transition); also
update any other call sites and the Points type signature accordingly (follow
the same pattern used by Drag) so transition is provided via constructor
injection and kept immutable.
In `@src/modules/Lines/index.ts`:
- Around line 986-998: The swap logic in updateAttributeBuffers currently reuses
the prior target as the new source causing a visual snap when a previous
transition is mid-flight; detect an active transition (e.g., inspect the
transition object used by setLinkColors/setLinkWidths and check
transition.progress is between 0 and 1) and instead seed the new source with the
currently displayed/interpolated values by computing a lerp between the existing
sourceBuffer data and targetBuffer data using transition.progress (or read back
the currently-displayed attribute if available), then write that interpolated
array into nextSource before returning so the new transition starts from the
on-screen state rather than the prior target.
- Around line 132-139: Preallocated this.colorBuffer/this.widthBuffer in
initPrograms are unused and leak; remove their creation and the class-level
aliases so the code only uses targetColorBuffer/targetWidthBuffer (and the
shader bindings drawCurveCommand/sourceColor/targetColor and
sourceWidth/targetWidth). Update draw() to stop checking the dead guards (if
(!this.colorBuffer) / if (!this.widthBuffer)) and instead rely on the existing
isLinkColor/WidthUpdateNeeded flags and updateColor()/updateWidth() to
initialize or refresh targetColorBuffer/targetWidthBuffer (ensure
create()/setLinks/setLinkColors/setLinkWidths still set those flags), and delete
any assignments that reassign colorBuffer/widthBuffer without destroying the
previous buffer; if you prefer keeping aliases, ensure
updateColor()/updateWidth() destroy the old GPU buffer before reassigning to
avoid leaks.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 0c779d55-49ee-40ad-8636-766d2d8b7f05
📒 Files selected for processing (14)
src/config.tssrc/index.tssrc/modules/Drag/index.tssrc/modules/GraphData/index.tssrc/modules/Lines/draw-curve-line.vertsrc/modules/Lines/index.tssrc/modules/Points/draw-points.vertsrc/modules/Points/index.tssrc/modules/Points/interpolate-position.fragsrc/modules/Points/position-utils.tssrc/modules/Transition/index.tssrc/stories/2. configuration.mdxsrc/stories/3. api-reference.mdxsrc/variables.ts
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
src/stories/transition/point-data.ts (1)
9-16: Consider settingcrossOriginfor user-supplied image URLs.
loadPointData(imageUrl)is exported and accepts an arbitrary URL. If a caller passes a cross-origin URL without proper CORS headers, the canvas will be tainted andctx.getImageDatainsampleImageToPointColorswill throw aSecurityError. The bundled default image is same-origin so the demo works today, but settingimg.crossOrigin = 'anonymous'before assigningsrcmakes the helper safe for external URLs (and is a no-op for same-origin).🛡️ Proposed change
function loadImage (url: string): Promise<HTMLImageElement> { return new Promise((resolve, reject) => { const img = new Image() + img.crossOrigin = 'anonymous' img.onload = (): void => resolve(img) img.onerror = (): void => reject(new Error(`Failed to load image: ${url}`)) img.src = url }) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/stories/transition/point-data.ts` around lines 9 - 16, The loadImage helper can cause canvas tainting for cross-origin images; modify the loadImage function to set img.crossOrigin = 'anonymous' before assigning img.src so external URLs can be fetched without CORS tainting (it is a no-op for same-origin); this protects downstream code such as sampleImageToPointColors and any exported loadPointData that relies on ctx.getImageData.
🤖 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/transition/point-transition.stories.ts`:
- Around line 16-18: The story's meta object (meta: Meta<CosmosStoryProps>) uses
title: 'Examples/Beginners' which miscategorizes this transition story; update
the title string to a dedicated transitions category such as
'Examples/Transitions/Point Position Transition' (or similar) so the story
appears under Examples → Transitions → Point Position Transition in Storybook
and does not collide with the existing Beginners section.
In `@src/stories/transition/point-transition.ts`:
- Around line 61-64: The button label is set to 'Pause' prematurely causing a
possible double-start; change the initial label for pausePlayAction to 'Play' to
match the initial not-playing state, and update the label only when the loop
actually transitions to playing (e.g., in the click handler when you call
startLoop() and in the requestAnimationFrame callback when you confirm the loop
started); also ensure startLoop() is guarded by the playing flag in both the
click handler and the rAF callback so startLoop() cannot be invoked twice.
- Around line 113-132: The rAF callback unconditionally calls startLoop(), which
can overwrite an earlier interval started by the pausePlayAction click handler
and leak timers; modify the rAF callback to only call startLoop() when the loop
is not already running (check loopIntervalId === undefined or use a dedicated
boolean like isLoopStarted), and make startLoop() itself idempotent by setting
that flag (or only assigning loopIntervalId when it is currently undefined) so
repeated calls from the click handler or rAF cannot double-schedule intervals;
ensure stopLoopTimer() clears the flag and loopIntervalId when stopping.
---
Nitpick comments:
In `@src/stories/transition/point-data.ts`:
- Around line 9-16: The loadImage helper can cause canvas tainting for
cross-origin images; modify the loadImage function to set img.crossOrigin =
'anonymous' before assigning img.src so external URLs can be fetched without
CORS tainting (it is a no-op for same-origin); this protects downstream code
such as sampleImageToPointColors and any exported loadPointData that relies on
ctx.getImageData.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: b1679608-c291-4990-bcb8-fef2c39b27c4
⛔ Files ignored due to path filters (1)
src/stories/transition/horsewoman-by-bryullov-1832.jpgis excluded by!**/*.jpg
📒 Files selected for processing (8)
README.mdmigration-notes.mdsrc/declaration.d.tssrc/stories/transition/point-data.tssrc/stories/transition/point-transition.stories.tssrc/stories/transition/point-transition.tssrc/stories/transition/transition-helpers.tssrc/stories/transition/transition.css
✅ Files skipped from review due to trivial changes (3)
- src/declaration.d.ts
- src/stories/transition/transition.css
- migration-notes.md
Signed-off-by: Stukova Olya <stukova.o@gmail.com>
Signed-off-by: Stukova Olya <stukova.o@gmail.com>
- Add history/ folder with README, LLM prompt, and first entry covering GPU transitions and runtime simulation toggle (commits e2af399, a9272fd) - Add /history skill to auto-draft entries from git context - Update CONTRIBUTING.md to point contributors to history/ for non-trivial changes Signed-off-by: Stukova Olya <stukova.o@gmail.com>
Stukova
force-pushed
the
feature/gpu-transitions
branch
from
d3529ff to
6b540da
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/modules/Lines/index.ts (1)
138-145:⚠️ Potential issue | 🟠 MajorLeaked/obsolete initial
colorBufferandwidthBufferallocations.The model no longer binds
color/widthattributes directly — it usessourceColor/targetColorandsourceWidth/targetWidth(see lines 235–249). The standalone buffers created here are therefore never attached to any draw pipeline, and the first call toupdateColor()/updateWidth()overwritesthis.colorBuffer/this.widthBufferwith the new target buffer via alias assignment (lines 617, 641) without destroying the original — a GPU buffer leak per module init.Two consequences worth addressing together:
- Drop the pre-allocation (it's dead).
- Reconsider whether keeping
colorBuffer/widthBufferas aliases is necessary at all; if they're only used for theif (!this.colorBuffer) this.updateColor()guards indraw()/findHoveredLine(), switching those to checkthis.targetColorBuffer/this.targetWidthBufferremoves the aliasing (and the leak vector) entirely.Proposed fix
- this.colorBuffer ||= device.createBuffer({ - data: new Float32Array(linksNumber * 4), - usage: Buffer.VERTEX | Buffer.COPY_DST, - }) - this.widthBuffer ||= device.createBuffer({ - data: new Float32Array(linksNumber), - usage: Buffer.VERTEX | Buffer.COPY_DST, - })And in
draw()/findHoveredLine()(and wherevercolorBuffer/widthBufferare read):- if (!this.colorBuffer) this.updateColor() - if (!this.widthBuffer) this.updateWidth() + if (!this.targetColorBuffer) this.updateColor() + if (!this.targetWidthBuffer) this.updateWidth()Then
this.colorBuffer/this.widthBuffer(and their destroy blocks) can be removed.🤖 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 138 - 145, The pre-allocated this.colorBuffer and this.widthBuffer are dead and leak because attributes now use sourceColor/targetColor and sourceWidth/targetWidth; remove the initial allocations for colorBuffer/widthBuffer in the constructor/init code, stop aliasing them in updateColor()/updateWidth(), and update guards in draw() and findHoveredLine() (and any other checks) to test this.targetColorBuffer and this.targetWidthBuffer instead of this.colorBuffer/this.widthBuffer; finally remove any remaining uses/destroys of the old this.colorBuffer/this.widthBuffer fields so only the target/source buffers are created and destroyed by their existing update/destroy logic.
♻️ Duplicate comments (4)
src/modules/Points/index.ts (3)
2209-2233:⚠️ Potential issue | 🟠 MajorDestroy attached framebuffers before replacing their textures.
These recreate paths still destroy textures before framebuffers that reference them, which conflicts with the documented lifecycle order and can leave framebuffer attachments dangling during resize/reallocation.
🛠️ Proposed ordering pattern
- if (this.velocityTexture && !this.velocityTexture.destroyed) { - this.velocityTexture.destroy() - } if (this.velocityFbo && !this.velocityFbo.destroyed) { this.velocityFbo.destroy() } + if (this.velocityTexture && !this.velocityTexture.destroyed) { + this.velocityTexture.destroy() + }Apply the same ordering to
sourcePositionFbo/sourcePositionTexture,targetPositionFbo/targetPositionTexture,currentPositionFbo/currentPositionTexture, andpreviousPositionFbo/previousPositionTexture.Also applies to: 2252-2297, 2482-2529
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/modules/Points/index.ts` around lines 2209 - 2233, The teardown/recreate sequence destroys textures before their attached framebuffers causing dangling attachments; change the order so you destroy each framebuffer (e.g., velocityFbo, sourcePositionFbo, targetPositionFbo, currentPositionFbo, previousPositionFbo) before destroying or replacing its corresponding texture (velocityTexture, sourcePositionTexture, targetPositionTexture, currentPositionTexture, previousPositionTexture), and apply the same ordering in the recreate blocks (the createTexture/copyImageData/createFramebuffer sequences) found around the velocity and the other position texture sections.
2368-2389:⚠️ Potential issue | 🟠 MajorInvalidate position caches after interpolation.
interpolatePosition()writes new positions intocurrentPositionFbo, but cached tracked positions and cluster centroids remain marked fresh, so readback APIs can return stale positions during/after a transition.🐛 Proposed cache invalidation
const renderPass = this.device.beginRenderPass({ framebuffer: this.currentPositionFbo, }) this.interpolatePositionCommand.draw(renderPass) renderPass.end() + + this.areClusterCentroidsUpToDate = false + this.isPositionsUpToDate = false🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/modules/Points/index.ts` around lines 2368 - 2389, interpolatePosition() updates currentPositionFbo but doesn't mark cached readback data stale; after the renderPass.end() call in interpolatePosition (the method named interpolatePosition), invalidate the tracked positions and cluster centroids caches so subsequent readback APIs won't return stale data — e.g. call the cache invalidation methods or set the dirty flags associated with trackedPositionsCache and clusterCentroidsCache (or set this.trackedPositionsDirty = true / this.clusterCentroidsDirty = true) immediately after renderPass.end().
940-955:⚠️ Potential issue | 🟠 MajorUse configured defaults instead of zero transition fallbacks.
Missing color/size arrays currently become transparent colors and zero-size points, so transition buffers can make default-rendered points disappear.
🐛 Proposed fallback adjustment
-import { readPixels } from '@/graph/helper' +import { getRgbaColor, readPixels } from '@/graph/helper'- const colorData = data.pointColors ?? new Float32Array((data.pointsNumber ?? 0) * 4).fill(0) + const defaultColor = getRgbaColor(this.config.pointDefaultColor) + const colorData = data.pointColors ?? Float32Array.from( + { length: (data.pointsNumber ?? 0) * 4 }, + (_, i) => defaultColor[i % 4] as number + )- const sizeData = data.pointSizes ?? new Float32Array(data.pointsNumber).fill(0) + const sizeData = data.pointSizes ?? new Float32Array(data.pointsNumber).fill(this.config.pointDefaultSize)Also applies to: 1059-1088
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/modules/Points/index.ts` around lines 940 - 955, The fallback for missing color/size data currently fills arrays with zeros causing transparent colors and zero-size points; change updateColor to build the fallback Float32Array from the configured defaults in this.store (use the actual store property for default point color, e.g. a default RGBA tuple on this.store or this.store.defaults) instead of new Float32Array(...).fill(0), and repeat that RGBA per-vertex; likewise update the analogous size fallback (lines noted 1059-1088) to use the configured default point size from this.store when data.pointSizes is missing so transition buffers don’t collapse visuals; keep the existing calls to updateAttributeBuffers and the same buffer variables (sourceColorBuffer, targetColorBuffer, previousColorData, colorBuffer).src/index.ts (1)
735-745:⚠️ Potential issue | 🟠 MajorScope transition pausing/aborting to position transitions only.
Line 735 pauses simulation for any pending transition, so color/size/width-only updates can unexpectedly stop the force simulation. Line 743 also aborts the entire queue when only
Positionsneeds a current position texture, dropping unrelated pending transitions.🐛 Proposed scoping fix
- if (this.transition.isPending && + if (this.transition.isPendingFor(TransitionProperty.Positions) && this.store.isSimulationRunning && this.config.transitionDuration > 0 && !this._isFirstRenderAfterInit) { this.store.isSimulationRunning = false this.config.onSimulationPause?.() } - if (this.transition.isPending && !this.points?.currentPositionTexture) { - this.transition.abort() + if (this.transition.isPendingFor(TransitionProperty.Positions) && !this.points?.currentPositionTexture) { + this.transition.dequeue(TransitionProperty.Positions) }Supporting API in
Transition:+ public isPendingFor (property: TransitionProperty): boolean { + return this.pendingProperties.has(property) + } + + public dequeue (property: TransitionProperty): void { + this.pendingProperties.delete(property) + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/index.ts` around lines 735 - 745, The pause and abort logic is currently applied for any pending transition (using this.transition.isPending) which incorrectly affects color/size/width-only updates; restrict these checks to position-updating transitions only by adding/using a predicate on Transition (e.g., a method or flag like affectsPositions or needsPositionTexture) and update both conditionals: only pause the simulation and call this.config.onSimulationPause when this.transition.isPending && this.transition.affectsPositions && this.store.isSimulationRunning && this.config.transitionDuration > 0 && !this._isFirstRenderAfterInit, and only call this.transition.abort when this.transition.isPending && this.transition.affectsPositions && !this.points?.currentPositionTexture so unrelated transitions are not paused/aborted.
🤖 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/index.ts`:
- Around line 1223-1225: When resuming the simulation, ending a transition with
this.transition.end(true) only clears state but doesn’t write the transition’s
target positions into currentPositionTexture, so forces resume from an
interpolated frame; after detecting an active transition
(this.transition.isActive) call this.transition.end(true), then immediately
copy/commit the transition’s target positions into currentPositionTexture (e.g.,
blit or shader pass from the transition’s targetPositionTexture into
currentPositionTexture or invoke a helper like transition.applyFinalPositions)
so the simulation starts from the final positions; apply this exact change at
both places where you currently call this.transition.end(true) (the instance
around the shown diff and the similar block at the other location).
In `@src/modules/Points/index.ts`:
- Around line 2428-2480: The bug is that updateAttributeBuffers currently reuses
the previous GPU buffers (swapping source/target) when counts match, which snaps
the attribute to the old target instead of continuing the in-flight
interpolation; change updateAttributeBuffers (and its callers) to accept the
prior source and prior target CPU Float32Array snapshots and the current
transitionProgress, and when sameCount is true build the next source CPU array
by linearly interpolating each component between priorSource and priorTarget at
transitionProgress (instead of reusing targetBuffer as next source), then create
new GPU buffers (via this.device.createBuffer) from that interpolated source and
the new targetData (or reuse/destroy old buffers appropriately); keep
previousData only for backwards compatibility but prefer priorSource/priorTarget
+ transitionProgress to resume animations smoothly.
---
Outside diff comments:
In `@src/modules/Lines/index.ts`:
- Around line 138-145: The pre-allocated this.colorBuffer and this.widthBuffer
are dead and leak because attributes now use sourceColor/targetColor and
sourceWidth/targetWidth; remove the initial allocations for
colorBuffer/widthBuffer in the constructor/init code, stop aliasing them in
updateColor()/updateWidth(), and update guards in draw() and findHoveredLine()
(and any other checks) to test this.targetColorBuffer and this.targetWidthBuffer
instead of this.colorBuffer/this.widthBuffer; finally remove any remaining
uses/destroys of the old this.colorBuffer/this.widthBuffer fields so only the
target/source buffers are created and destroyed by their existing update/destroy
logic.
---
Duplicate comments:
In `@src/index.ts`:
- Around line 735-745: The pause and abort logic is currently applied for any
pending transition (using this.transition.isPending) which incorrectly affects
color/size/width-only updates; restrict these checks to position-updating
transitions only by adding/using a predicate on Transition (e.g., a method or
flag like affectsPositions or needsPositionTexture) and update both
conditionals: only pause the simulation and call this.config.onSimulationPause
when this.transition.isPending && this.transition.affectsPositions &&
this.store.isSimulationRunning && this.config.transitionDuration > 0 &&
!this._isFirstRenderAfterInit, and only call this.transition.abort when
this.transition.isPending && this.transition.affectsPositions &&
!this.points?.currentPositionTexture so unrelated transitions are not
paused/aborted.
In `@src/modules/Points/index.ts`:
- Around line 2209-2233: The teardown/recreate sequence destroys textures before
their attached framebuffers causing dangling attachments; change the order so
you destroy each framebuffer (e.g., velocityFbo, sourcePositionFbo,
targetPositionFbo, currentPositionFbo, previousPositionFbo) before destroying or
replacing its corresponding texture (velocityTexture, sourcePositionTexture,
targetPositionTexture, currentPositionTexture, previousPositionTexture), and
apply the same ordering in the recreate blocks (the
createTexture/copyImageData/createFramebuffer sequences) found around the
velocity and the other position texture sections.
- Around line 2368-2389: interpolatePosition() updates currentPositionFbo but
doesn't mark cached readback data stale; after the renderPass.end() call in
interpolatePosition (the method named interpolatePosition), invalidate the
tracked positions and cluster centroids caches so subsequent readback APIs won't
return stale data — e.g. call the cache invalidation methods or set the dirty
flags associated with trackedPositionsCache and clusterCentroidsCache (or set
this.trackedPositionsDirty = true / this.clusterCentroidsDirty = true)
immediately after renderPass.end().
- Around line 940-955: The fallback for missing color/size data currently fills
arrays with zeros causing transparent colors and zero-size points; change
updateColor to build the fallback Float32Array from the configured defaults in
this.store (use the actual store property for default point color, e.g. a
default RGBA tuple on this.store or this.store.defaults) instead of new
Float32Array(...).fill(0), and repeat that RGBA per-vertex; likewise update the
analogous size fallback (lines noted 1059-1088) to use the configured default
point size from this.store when data.pointSizes is missing so transition buffers
don’t collapse visuals; keep the existing calls to updateAttributeBuffers and
the same buffer variables (sourceColorBuffer, targetColorBuffer,
previousColorData, colorBuffer).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 5e628bce-c6c4-4484-b686-1e92aeb51dde
⛔ Files ignored due to path filters (1)
src/stories/transition/horsewoman-by-bryullov-1832.jpgis excluded by!**/*.jpg
📒 Files selected for processing (27)
.claude/skills/history/SKILL.mdCONTRIBUTING.mdREADME.mdhistory/2026/2026-04-22-gpu-transitions.mdhistory/PROMPT.mdhistory/README.mdmigration-notes.mdsrc/config.tssrc/declaration.d.tssrc/index.tssrc/modules/Drag/index.tssrc/modules/GraphData/index.tssrc/modules/Lines/draw-curve-line.vertsrc/modules/Lines/index.tssrc/modules/Points/draw-points.vertsrc/modules/Points/index.tssrc/modules/Points/interpolate-position.fragsrc/modules/Points/position-utils.tssrc/modules/Transition/index.tssrc/stories/2. configuration.mdxsrc/stories/3. api-reference.mdxsrc/stories/transition/point-data.tssrc/stories/transition/point-transition.stories.tssrc/stories/transition/point-transition.tssrc/stories/transition/transition-helpers.tssrc/stories/transition/transition.csssrc/variables.ts
✅ Files skipped from review due to trivial changes (9)
- src/declaration.d.ts
- CONTRIBUTING.md
- history/README.md
- .claude/skills/history/SKILL.md
- history/PROMPT.md
- history/2026/2026-04-22-gpu-transitions.md
- migration-notes.md
- src/stories/transition/point-transition.stories.ts
- src/stories/3. api-reference.mdx
🚧 Files skipped from review as they are similar to previous changes (7)
- src/stories/transition/transition.css
- README.md
- src/modules/Points/interpolate-position.frag
- src/variables.ts
- src/stories/2. configuration.mdx
- src/modules/Lines/draw-curve-line.vert
- src/modules/Points/draw-points.vert
Color/size transitions don't compete with force updates, so they shouldn't pause the simulation. Add Transition.isPendingFor(property) and gate the simulation pause on a pending Positions transition only. Also tidy the point-transition story: drop the color-clustered scatter reveal (and its helper), snap directly to the picture layout, and move it under Examples/Transitions. Set transitionDuration: 0 on the remove-points and moscow-metro-stations stories to avoid unintended load-time animation. Signed-off-by: Stukova Olya <stukova.o@gmail.com>
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)
1174-1182:⚠️ Potential issue | 🟠 MajorUse a position-only transition guard for simulation resume paths.
start()can turn physics back on while a position transition is active, whileunpause()and enabling simulation calltransition.end(true)for any active transition, including color/size/link-only animations that do not conflict with physics. Gate this onTransitionProperty.Positionsconsistently.🐛 Suggested guard sketch
+ private interruptPositionTransitionForSimulation (): void { + if (!this.transition.isActiveFor(TransitionProperty.Positions)) return + this.transition.end(true) + } + public start (alpha = 1): void { @@ if (!this.config.enableSimulation) return if (!this.graph.pointsNumber) return if (this.store.isSimulationRunning) return + this.interruptPositionTransitionForSimulation() @@ - if (this.transition.isActive) { - this.transition.end(true) - } + this.interruptPositionTransitionForSimulation() @@ if (this.config.enableSimulation) { - this.transition.end(true) + this.interruptPositionTransitionForSimulation() this.ensureSimulationModules()If mixed position + color/link cycles must preserve non-position animations, this should become a scoped
TransitionAPI instead of ending the whole active cycle.Also applies to: 1222-1224, 1532-1534
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/index.ts` around lines 1174 - 1182, The simulation start/resume path currently calls transition.end(true) and thus ends all active transitions (color/size/link) even when only positions must resume; update start(), unpause(), and the code path that enables simulation to only terminate position transitions by using a position-only guard (e.g., call transition.end(true, TransitionProperty.Positions) or check the active Transition's properties and only end if it includes Positions) so non-position animations remain running; adjust any helper that currently calls transition.end(true) (referenced by start, unpause, and the enable-simulation branch) to use the Positions-scoped end call or a check against TransitionProperty.Positions.
🧹 Nitpick comments (1)
src/stories/transition/point-transition.ts (1)
70-83: Cache generated scatter layouts instead of reallocating every tick.Line 76 regenerates a ~1.6 MB
Float32Arrayfor each 200k-point scatter step on every interval. SinceLOOP_STEPSonly uses fixed tile counts, cache bystepto reduce GC pressure during the long-running demo.♻️ Suggested cache sketch
const picturePositions = createPicturePositions(cols, rows, spaceSize, aspect) + const tilePositionsByStep = new Map<number, Float32Array>() + const getTilePositions = (step: number): Float32Array => { + let positions = tilePositionsByStep.get(step) + if (!positions) { + positions = createTileScatterPositions(cols, rows, spaceSize, aspect, step) + tilePositionsByStep.set(step, positions) + } + return positions + } @@ if (step === undefined) { graph.setPointPositions(picturePositions) } else { - const tilePositions = createTileScatterPositions( - cols, - rows, - spaceSize, - aspect, - step - ) - graph.setPointPositions(tilePositions) + graph.setPointPositions(getTilePositions(step)) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/stories/transition/point-transition.ts` around lines 70 - 83, The interval handler in startLoop regenerates large Float32Array layouts each tick (via createTileScatterPositions) for the same fixed LOOP_STEPS; introduce a cache (e.g., Map<number, Float32Array> or Record keyed by step) inside the module/scope containing startLoop, compute and store tilePositions the first time createTileScatterPositions(cols, rows, spaceSize, aspect, step) is called for a given step, and on subsequent ticks reuse the cached array when loopStepIndex yields that step before calling graph.setPointPositions; also ensure the cache is invalidated or rebuilt if cols/rows/spaceSize/aspect change.
🤖 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/index.ts`:
- Around line 734-740: The pause logic currently pauses simulation when a
position transition is pending but never resumes it; introduce a boolean flag
(e.g., this._pausedByPositionTransition) and set it to true where the code
currently pauses (the block checking
this.transition.isPendingFor(TransitionProperty.Positions) &&
this.store.isSimulationRunning), then call this.store.isSimulationRunning =
false and this.config.onSimulationPause?.() as before. In the
transition-complete handler (the method that runs when the position transition
ends—referenced near the existing end-of-transition code around where
setPointPositions()/transition completion is handled), check if
this._pausedByPositionTransition is true and if so set
this.store.isSimulationRunning = true, call this.config.onSimulationResume?.()
if applicable, and clear this._pausedByPositionTransition. Also clear
this._pausedByPositionTransition in explicit user pause()/stop() paths so manual
pauses override auto-resume.
---
Outside diff comments:
In `@src/index.ts`:
- Around line 1174-1182: The simulation start/resume path currently calls
transition.end(true) and thus ends all active transitions (color/size/link) even
when only positions must resume; update start(), unpause(), and the code path
that enables simulation to only terminate position transitions by using a
position-only guard (e.g., call transition.end(true,
TransitionProperty.Positions) or check the active Transition's properties and
only end if it includes Positions) so non-position animations remain running;
adjust any helper that currently calls transition.end(true) (referenced by
start, unpause, and the enable-simulation branch) to use the Positions-scoped
end call or a check against TransitionProperty.Positions.
---
Nitpick comments:
In `@src/stories/transition/point-transition.ts`:
- Around line 70-83: The interval handler in startLoop regenerates large
Float32Array layouts each tick (via createTileScatterPositions) for the same
fixed LOOP_STEPS; introduce a cache (e.g., Map<number, Float32Array> or Record
keyed by step) inside the module/scope containing startLoop, compute and store
tilePositions the first time createTileScatterPositions(cols, rows, spaceSize,
aspect, step) is called for a given step, and on subsequent ticks reuse the
cached array when loopStepIndex yields that step before calling
graph.setPointPositions; also ensure the cache is invalidated or rebuilt if
cols/rows/spaceSize/aspect change.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: b3d3e7ec-992c-472c-99ab-bf3812fd3bf6
📒 Files selected for processing (7)
src/index.tssrc/modules/Transition/index.tssrc/stories/beginners/remove-points/config.tssrc/stories/geospatial/moscow-metro-stations/index.tssrc/stories/transition/point-transition.stories.tssrc/stories/transition/point-transition.tssrc/stories/transition/transition-helpers.ts
✅ Files skipped from review due to trivial changes (3)
- src/stories/geospatial/moscow-metro-stations/index.ts
- src/stories/beginners/remove-points/config.ts
- src/stories/transition/point-transition.stories.ts
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
src/index.ts (2)
1217-1227:⚠️ Potential issue | 🟠 Major
unpause()starts forces from an interpolated frame, not the target positions.
this.transition.end(true)on Line 1223 only clearsTransitionstate (active set, progress, startTime) — it does not commit the transition's target positions intocurrentPositionTexture. If a Positions transition is mid-flight when the user unpauses, the force simulation resumes from wherever the interpolated frame happened to be, not from the positions the caller ofsetPointPositions()requested.The same concern applies in
applyEnableSimulationConfigChangeat Line 1533 wheretransition.end(true)is called right beforeensureSimulationModules()/create().🐛 Suggested helper
+ private finishActivePositionTransitionForSimulation (): void { + if (this.transition.isActiveFor(TransitionProperty.Positions)) { + this.points?.interpolatePosition(1) + } + this.transition.end(true) + }Then call
finishActivePositionTransitionForSimulation()at both sites instead oftransition.end(true).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/index.ts` around lines 1217 - 1227, The unpause() method currently calls transition.end(true) which only clears Transition state but does not commit the transition's target positions into currentPositionTexture, causing the simulation to resume from an interpolated frame; replace the transition.end(true) call in unpause() with a call to finishActivePositionTransitionForSimulation() so active Positions transitions commit their target positions before resuming, and make the same replacement in applyEnableSimulationConfigChange() right before ensureSimulationModules()/create(); reference transition.end(true), finishActivePositionTransitionForSimulation(), unpause(), applyEnableSimulationConfigChange(), setPointPositions(), currentPositionTexture and Transition when locating and updating the code.
734-744:⚠️ Potential issue | 🟠 MajorSimulation paused by a pending position transition is never resumed.
When
setPointPositions()is called on a running simulation withtransitionDuration > 0, Lines 734–740 pause the simulation for the duration of the position transition, but nothing re-enables it after the transition ends. A graph that was simulating beforesetPointPositions()stays paused indefinitely once the animation completes.Track the "paused-for-transition" state (e.g. a
shouldResumeSimulationAfterPositionTransitionflag set here and cleared inrenderFrameoncetransition.isActiveFor(TransitionProperty.Positions)flips from true → false), then flipisSimulationRunningback on and fireonSimulationUnpause?.(). Also clear the flag in explicitpause()/stop()/destroy()paths so manual user actions win over auto-resume.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/index.ts` around lines 734 - 744, The code pauses the running simulation when a positions transition starts but never resumes it; add a boolean flag (e.g. shouldResumeSimulationAfterPositionTransition) and set it where you currently set store.isSimulationRunning = false (inside the block that checks transition.isPendingFor(TransitionProperty.Positions), config.transitionDuration > 0, and !_isFirstRenderAfterInit), then in renderFrame detect when transition.isActiveFor(TransitionProperty.Positions) flips from true to false and if the flag is set re-enable store.isSimulationRunning, call config.onSimulationUnpause?.(), and clear the flag; also ensure the flag is cleared whenever pause(), stop(), or destroy() are invoked so manual actions override auto-resume; leave the existing transition.abort logic (transition.isPending && !points?.currentPositionTexture) unchanged.
🤖 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/index.ts`:
- Around line 1549-1556: The onSimulationEnd callback is being invoked
unconditionally when disabling simulation; mirror the logic used in stop() to
avoid spurious end events: before mutating
this.store.isSimulationRunning/alpha/simulationProgress, compute a
wasSimulationActive boolean (e.g. const wasSimulationActive =
this.store.isSimulationRunning || this.store.alpha > 0 ||
this.store.simulationProgress > 0) and only call this.config.onSimulationEnd?.()
if wasSimulationActive is true; leave the rest of the cleanup
(this._shouldForceHoverDetection = true and this.destroySimulationModules())
unchanged.
---
Duplicate comments:
In `@src/index.ts`:
- Around line 1217-1227: The unpause() method currently calls
transition.end(true) which only clears Transition state but does not commit the
transition's target positions into currentPositionTexture, causing the
simulation to resume from an interpolated frame; replace the
transition.end(true) call in unpause() with a call to
finishActivePositionTransitionForSimulation() so active Positions transitions
commit their target positions before resuming, and make the same replacement in
applyEnableSimulationConfigChange() right before
ensureSimulationModules()/create(); reference transition.end(true),
finishActivePositionTransitionForSimulation(), unpause(),
applyEnableSimulationConfigChange(), setPointPositions(), currentPositionTexture
and Transition when locating and updating the code.
- Around line 734-744: The code pauses the running simulation when a positions
transition starts but never resumes it; add a boolean flag (e.g.
shouldResumeSimulationAfterPositionTransition) and set it where you currently
set store.isSimulationRunning = false (inside the block that checks
transition.isPendingFor(TransitionProperty.Positions), config.transitionDuration
> 0, and !_isFirstRenderAfterInit), then in renderFrame detect when
transition.isActiveFor(TransitionProperty.Positions) flips from true to false
and if the flag is set re-enable store.isSimulationRunning, call
config.onSimulationUnpause?.(), and clear the flag; also ensure the flag is
cleared whenever pause(), stop(), or destroy() are invoked so manual actions
override auto-resume; leave the existing transition.abort logic
(transition.isPending && !points?.currentPositionTexture) unchanged.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 427e7fc7-6af9-4af2-a157-d7793e45b2ad
📒 Files selected for processing (7)
src/index.tssrc/modules/Transition/index.tssrc/stories/beginners/remove-points/config.tssrc/stories/geospatial/moscow-metro-stations/index.tssrc/stories/transition/point-transition.stories.tssrc/stories/transition/point-transition.tssrc/stories/transition/transition-helpers.ts
✅ Files skipped from review due to trivial changes (1)
- src/stories/geospatial/moscow-metro-stations/index.ts
🚧 Files skipped from review as they are similar to previous changes (4)
- src/stories/beginners/remove-points/config.ts
- src/stories/transition/point-transition.stories.ts
- src/stories/transition/transition-helpers.ts
- src/stories/transition/point-transition.ts
…ases - Invalidate position/centroid caches after interpolatePosition so trackedPositions and centroid getters don't return stale data during a position transition with simulation inactive - Skip hover detection while transitions are active (hover shaders read target buffers and would mismatch the visible interpolated geometry) - Guard onSimulationEnd in applyEnableSimulationConfigChange against spurious firing when disabling a never-running simulation - Drop dead colorBuffer/sizeBuffer aliases in Points and colorBuffer/ widthBuffer in Lines; Lines init allocations were leaking on first update - Match Framebuffer→Texture destroy order in resize branches with destroy() - Replace zero-fill fallbacks in Points updateColor/updateSize with \`as Float32Array\` + invariant comment - Document that setPointPositions intentionally keeps simulation paused Signed-off-by: Stukova Olya <stukova.o@gmail.com>
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
src/modules/Points/index.ts (1)
2483-2528:⚠️ Potential issue | 🟠 MajorDestroy order in
createOrUpdatePositionTexturesviolates the documented Framebuffer→Texture pattern.The other recreate paths in this PR (
ensureSimulationResources,createTransitionResources,destroyTransitionResources,destroySimulationResources, anddestroy()) all follow the documented order "Models → Framebuffers → Textures → UniformStores → Buffers". This method still destroys the texture before the framebuffer that holds it as a color attachment, leaving the FBO momentarily attached to a destroyed texture. Mirror the same fix here for bothcurrentPosition*andpreviousPosition*.🛠️ Proposed ordering fix
if (!this.currentPositionTexture || this.currentPositionTexture.width !== pointsTextureSize || this.currentPositionTexture.height !== pointsTextureSize) { - if (this.currentPositionTexture && !this.currentPositionTexture.destroyed) { - this.currentPositionTexture.destroy() - } if (this.currentPositionFbo && !this.currentPositionFbo.destroyed) { this.currentPositionFbo.destroy() } + if (this.currentPositionTexture && !this.currentPositionTexture.destroyed) { + this.currentPositionTexture.destroy() + } this.currentPositionTexture = this.device.createTexture({if (!this.previousPositionTexture || this.previousPositionTexture.width !== pointsTextureSize || this.previousPositionTexture.height !== pointsTextureSize) { - if (this.previousPositionTexture && !this.previousPositionTexture.destroyed) { - this.previousPositionTexture.destroy() - } if (this.previousPositionFbo && !this.previousPositionFbo.destroyed) { this.previousPositionFbo.destroy() } + if (this.previousPositionTexture && !this.previousPositionTexture.destroyed) { + this.previousPositionTexture.destroy() + } this.previousPositionTexture = this.device.createTexture({🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/modules/Points/index.ts` around lines 2483 - 2528, In createOrUpdatePositionTextures, the destroy order currently destroys currentPositionTexture (and previousPositionTexture) before their associated framebuffers, violating the Framebuffer→Texture pattern; update both blocks so you first destroy this.currentPositionFbo (if exists and not destroyed) then destroy this.currentPositionTexture, and likewise destroy this.previousPositionFbo before this.previousPositionTexture, keeping the same existence checks (e.g., this.currentPositionFbo && !this.currentPositionFbo.destroyed) and preserving the subsequent createTexture/createFramebuffer calls and copyImageData usage.
🧹 Nitpick comments (1)
src/index.ts (1)
256-256: Ad-hoc field assignment couplesPointstoTransitionafter construction.Setting
this.points.transition = this.transitionoutside thePointsconstructor relies onPointsexposing a public mutabletransitionfield; it also leaves a brief window where a freshly-constructedPointshas no transition reference (anything that reads it during the same tick would crash/no-op). Threadingtransitionthrough thePointsconstructor (likeDragalready does) would make the dependency explicit and remove the ordering trap.♻️ Sketch
- this.points = new Points(device, this.config, this.store, this.graph) - this.points.transition = this.transition + this.points = new Points(device, this.config, this.store, this.graph, this.transition)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/index.ts` at line 256, The Points instance is being coupled to Transition after construction via the ad-hoc assignment this.points.transition = this.transition; instead pass the Transition into the Points constructor (mirroring how Drag is constructed) so Points receives and stores the reference during construction, remove the external assignment, and update the Points class signature (constructor) and all call sites where new Points(...) is created to accept and store the Transition dependency (ensure any public API used elsewhere is preserved or exposed via constructor-set fields/methods).
🤖 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/index.ts`:
- Around line 380-382: The guard that queues position transitions only checks
this.points?.currentPositionTexture for truthiness but must also ensure the
texture isn't destroyed; update both the pre-queue check (around the current
transition.queue(TransitionProperty.Positions) call) and the abort() fallback
guard to verify the texture is not destroyed (e.g., ensure
this.points.currentPositionTexture &&
!this.points.currentPositionTexture.destroyed) before calling transition.queue,
transition.start, or points.interpolatePosition so no operations run on a
destroyed GPU resource.
In `@src/modules/Lines/index.ts`:
- Around line 1066-1118: updateAttributeBuffers is reusing buffers in the
sameCount fast path (setting nextSource = targetBuffer) which causes a visual
snap because it doesn't capture the currently interpolated frame; instead, when
queuing a new transition mid-flight capture the actual interpolated attribute
values and seed sourceData with them before swapping buffers. Modify
updateAttributeBuffers (and the fast-path branch that uses
nextSource/nextTarget) to either read back the GPU-generated interpolated values
(e.g., via the same read-back used in Points) or compute CPU-side interpolation
from previousData and targetData using the current transitionProgress, then
write those interpolated values into the buffer you will use as the new source
(rather than using raw targetBuffer contents), ensuring source/previous reflect
the visible frame so the next transition interpolates smoothly.
---
Duplicate comments:
In `@src/modules/Points/index.ts`:
- Around line 2483-2528: In createOrUpdatePositionTextures, the destroy order
currently destroys currentPositionTexture (and previousPositionTexture) before
their associated framebuffers, violating the Framebuffer→Texture pattern; update
both blocks so you first destroy this.currentPositionFbo (if exists and not
destroyed) then destroy this.currentPositionTexture, and likewise destroy
this.previousPositionFbo before this.previousPositionTexture, keeping the same
existence checks (e.g., this.currentPositionFbo &&
!this.currentPositionFbo.destroyed) and preserving the subsequent
createTexture/createFramebuffer calls and copyImageData usage.
---
Nitpick comments:
In `@src/index.ts`:
- Line 256: The Points instance is being coupled to Transition after
construction via the ad-hoc assignment this.points.transition = this.transition;
instead pass the Transition into the Points constructor (mirroring how Drag is
constructed) so Points receives and stores the reference during construction,
remove the external assignment, and update the Points class signature
(constructor) and all call sites where new Points(...) is created to accept and
store the Transition dependency (ensure any public API used elsewhere is
preserved or exposed via constructor-set fields/methods).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: f66eaf3a-890c-4bcc-bfc6-50a03c5f3eb9
📒 Files selected for processing (4)
src/config.tssrc/index.tssrc/modules/Lines/index.tssrc/modules/Points/index.ts
- Update 2026-04-22-gpu-transitions.md to reflect post-merge state and follow-up fixes (c5fd30e, 75b1a15, 74e0567): auto-pause is now position-only, hover skipped during transitions, position/centroid caches invalidated, behavior matrix split into position vs color/size rows, Storybook path fixed - Add --update mode to the /history skill so follow-up commits extending an existing topic revise the latest entry in place without re-asking for a "why" - Mention /history --update in CONTRIBUTING.md and history/README.md Signed-off-by: Stukova Olya <stukova.o@gmail.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@history/2026/2026-04-22-gpu-transitions.md`:
- Line 68: Update the sentence that ends with “and stay paused afterwards” to
use US spelling by replacing “afterwards” with “afterward”; locate the line
mentioning the new `transitionDuration` default and the sentence that reads
"Call `graph.unpause()` to resume forces." and change it to "...and stay paused
afterward. Call `graph.unpause()` to resume forces."
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: c599ee9c-12ee-41e2-8164-14b774ed285a
📒 Files selected for processing (4)
.claude/skills/history/SKILL.mdCONTRIBUTING.mdhistory/2026/2026-04-22-gpu-transitions.mdhistory/README.md
✅ Files skipped from review due to trivial changes (3)
- CONTRIBUTING.md
- .claude/skills/history/SKILL.md
- history/README.md
- src/index.ts: skip pending position transition queue/abort when currentPositionTexture is destroyed.
- src/modules/{Lines,Points}/index.ts: add TODO for rare mid-animation attribute-update smoothing edge case.
Signed-off-by: Stukova Olya <stukova.o@gmail.com>
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (3)
src/modules/Points/index.ts (2)
2484-2530:⚠️ Potential issue | 🟡 MinorDestroy the FBO before its attached texture in recreate paths.
createOrUpdatePositionTextures()still tears downcurrentPositionTexture/previousPositionTexturebefore the framebuffers that reference them. The rest of this module already followsFramebuffers -> Textures; this branch should do the same.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/modules/Points/index.ts` around lines 2484 - 2530, In createOrUpdatePositionTextures(), the teardown destroys currentPositionTexture / previousPositionTexture before their framebuffers; reverse the order so you destroy currentPositionFbo before currentPositionTexture and previousPositionFbo before previousPositionTexture (check and respect the destroyed checks), ensuring you call destroy() on the framebuffer objects (currentPositionFbo and previousPositionFbo) prior to destroying their attached textures (currentPositionTexture and previousPositionTexture) and keep the existing recreate logic that creates the texture then the framebuffer.
2438-2450:⚠️ Potential issue | 🟠 MajorChain point attribute transitions from the interpolated value.
The same-count fast path still reuses the previous target buffer as the next source. A second
setPointColors()orsetPointSizes()during an active transition will visibly snap before animating again.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/modules/Points/index.ts` around lines 2438 - 2450, The fast-path is reusing the prior target buffer as the next source so a new setPointColors()/setPointSizes() during an active transition snaps because previous is set to the raw new targetData; instead capture the current interpolated values from the buffer that will become the source (targetBuffer / nextSource) and use those as the previous array. Update the branch where nextSource = targetBuffer and nextTarget = sourceBuffer so that you read/copy the current contents of nextSource (the buffer being reused as the new source) into previous (e.g., clone its current attribute data) and keep nextTarget.write(targetData) for the new target; reference sameCount, sourceBuffer, targetBuffer, nextSource, nextTarget, and previous to locate the change.src/modules/Lines/index.ts (1)
1077-1089:⚠️ Potential issue | 🟠 MajorChain link attribute transitions from the displayed frame.
This fast path still promotes the old target buffer to the next source. If
setLinkColors()orsetLinkWidths()lands mid-transition, the next animation snaps to the previous target before continuing.🤖 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 1077 - 1089, The fast-path promotes the old target buffer to the next source which causes new setLinkColors()/setLinkWidths() calls mid-transition to snap to the previous target; fix the swap in the sameCount branch by ensuring the returned previous array reflects the currently displayed attributes (the buffer being shown, i.e., nextSource) instead of blindly using targetData—when you set nextSource = targetBuffer and nextTarget = sourceBuffer, build previous from the nextSource's current attribute contents (read or clone its backing Float32Array) so subsequent transitions chain smoothly from the displayed frame.
🤖 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/Points/index.ts`:
- Around line 268-271: The shouldAnimate gate currently uses
this.transition?.isPending and can be triggered by unrelated queued transitions;
narrow it to only pending Positions transitions by checking this.transition is
pending AND is for Positions (e.g., inspect this.transition.type or
this.transition.key === 'Positions') and also ensure the texture is alive (check
this.currentPositionTexture exists and is not destroyed) before setting
shouldAnimate so updatePositions() only follows the position-transition path;
update the shouldAnimate expression accordingly (referencing shouldAnimate,
this.transition, this.config.transitionDuration, this.currentPositionTexture,
and updatePositions()).
---
Duplicate comments:
In `@src/modules/Lines/index.ts`:
- Around line 1077-1089: The fast-path promotes the old target buffer to the
next source which causes new setLinkColors()/setLinkWidths() calls
mid-transition to snap to the previous target; fix the swap in the sameCount
branch by ensuring the returned previous array reflects the currently displayed
attributes (the buffer being shown, i.e., nextSource) instead of blindly using
targetData—when you set nextSource = targetBuffer and nextTarget = sourceBuffer,
build previous from the nextSource's current attribute contents (read or clone
its backing Float32Array) so subsequent transitions chain smoothly from the
displayed frame.
In `@src/modules/Points/index.ts`:
- Around line 2484-2530: In createOrUpdatePositionTextures(), the teardown
destroys currentPositionTexture / previousPositionTexture before their
framebuffers; reverse the order so you destroy currentPositionFbo before
currentPositionTexture and previousPositionFbo before previousPositionTexture
(check and respect the destroyed checks), ensuring you call destroy() on the
framebuffer objects (currentPositionFbo and previousPositionFbo) prior to
destroying their attached textures (currentPositionTexture and
previousPositionTexture) and keep the existing recreate logic that creates the
texture then the framebuffer.
- Around line 2438-2450: The fast-path is reusing the prior target buffer as the
next source so a new setPointColors()/setPointSizes() during an active
transition snaps because previous is set to the raw new targetData; instead
capture the current interpolated values from the buffer that will become the
source (targetBuffer / nextSource) and use those as the previous array. Update
the branch where nextSource = targetBuffer and nextTarget = sourceBuffer so that
you read/copy the current contents of nextSource (the buffer being reused as the
new source) into previous (e.g., clone its current attribute data) and keep
nextTarget.write(targetData) for the new target; reference sameCount,
sourceBuffer, targetBuffer, nextSource, nextTarget, and previous to locate the
change.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: ea85d3ed-122b-4a0e-af9f-85ebb8f2cb31
📒 Files selected for processing (3)
src/index.tssrc/modules/Lines/index.tssrc/modules/Points/index.ts
Signed-off-by: Stukova Olya <stukova.o@gmail.com>
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/modules/Points/index.ts (2)
1449-1473:⚠️ Potential issue | 🟡 MinorAnimate hover/focus ring size with the point.
The ring pass still sizes from
data.pointSizes/data.pointImageSizes, so the outline jumps to the final radius while the underlying point is still interpolating. Use the same source/target size pair plustransitionProgresshere whenshouldAnimatePointSizesis active.Also applies to: 1483-1507
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/modules/Points/index.ts` around lines 1449 - 1473, The ring sizing uses data.pointSizes/data.pointImageSizes directly causing a jump; update the logic inside drawHighlightedUniformStore.setUniforms (the block that reads pointSize = data.pointSizes?.[store.hoveredPoint.index] ...) to check shouldAnimatePointSizes and the hovered point's transitionProgress (e.g., hoveredPoint.transitionProgress or similar fields used elsewhere) and compute an interpolated size from the transition's source/target size pair instead of the static arrays; use that interpolated value for both pointSize and imageSize (and for the other similar block around lines 1483-1507) so the ring animates in sync with the point.
1063-1119:⚠️ Potential issue | 🟠 MajorKeep hit-testing and selection on the same size curve as rendering.
updateSize()now publishes only the final size state (targetSizeBuffer/sizeTexture). The draw shader interpolates point size, but hover picking and rect/polygon queries consume the snapped target immediately, so points can be hovered or selected outside their visible radius during a size transition.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/modules/Points/index.ts` around lines 1063 - 1119, updateSize() currently writes the snapped final sizes into sizeTexture (used by hit-testing) from data.pointSizes, causing hover/selection to use the target size during transitions; change it to use the currently uploaded source/previous buffer values for the sizeTexture so hit-testing matches the visible interpolated size. Specifically, after calling updateAttributeBuffers(...) use the returned source (or previous if source is not present) buffer/content instead of data.pointSizes to populate initialState (i.e., derive per-point size from the source/previous buffer values you just got), and then copy that initialState into sizeTexture (and when updating sizeTexture on subsequent frames) so sizeTexture reflects the current visible size rather than the snapped target; keep drawCommand attribute updates for sourceSize/targetSize as-is.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@src/modules/Points/index.ts`:
- Around line 1449-1473: The ring sizing uses
data.pointSizes/data.pointImageSizes directly causing a jump; update the logic
inside drawHighlightedUniformStore.setUniforms (the block that reads pointSize =
data.pointSizes?.[store.hoveredPoint.index] ...) to check
shouldAnimatePointSizes and the hovered point's transitionProgress (e.g.,
hoveredPoint.transitionProgress or similar fields used elsewhere) and compute an
interpolated size from the transition's source/target size pair instead of the
static arrays; use that interpolated value for both pointSize and imageSize (and
for the other similar block around lines 1483-1507) so the ring animates in sync
with the point.
- Around line 1063-1119: updateSize() currently writes the snapped final sizes
into sizeTexture (used by hit-testing) from data.pointSizes, causing
hover/selection to use the target size during transitions; change it to use the
currently uploaded source/previous buffer values for the sizeTexture so
hit-testing matches the visible interpolated size. Specifically, after calling
updateAttributeBuffers(...) use the returned source (or previous if source is
not present) buffer/content instead of data.pointSizes to populate initialState
(i.e., derive per-point size from the source/previous buffer values you just
got), and then copy that initialState into sizeTexture (and when updating
sizeTexture on subsequent frames) so sizeTexture reflects the current visible
size rather than the snapped target; keep drawCommand attribute updates for
sourceSize/targetSize as-is.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: ea19cd6d-9931-4401-b11d-441f703ca231
📒 Files selected for processing (1)
src/modules/Points/index.ts
Signed-off-by: Stukova Olya <stukova.o@gmail.com>
There was a problem hiding this comment.
🧹 Nitpick comments (1)
history/2026/2026-04-22-gpu-transitions.md (1)
56-64: Clarify matrix wording to explicitly include link transitions in the non-pausing path.The current labels say “color/size transition,” but this PR also transitions link color/width. Expanding wording avoids ambiguity for readers.
✏️ Suggested doc tweak
-Each row is the state when `render()` fires. The last two columns show what happens if you flip each config via `setConfigPartial` from that state. Auto-pause applies to **position** transitions only — color/size transitions never pause the simulation. +Each row is the state when `render()` fires. The last two columns show what happens if you flip each config via `setConfigPartial` from that state. Auto-pause applies to **position** transitions only — point/link attribute transitions (point color/size, link color/width) never pause the simulation. -| `true` + `>0` (simulation, **color/size** transition) | Animate. Simulation keeps running — color/size transitions don't compete with forces. | `→ false`: stops simulation, destroys simulation-only resources, fires `onSimulationEnd`. | `→ ≤0`: next `start()` snaps. A running transition ends with `onTransitionEnd(true)` on the next step. | +| `true` + `>0` (simulation, **attribute** transition: point color/size or link color/width) | Animate. Simulation keeps running — attribute transitions don't compete with forces. | `→ false`: stops simulation, destroys simulation-only resources, fires `onSimulationEnd`. | `→ ≤0`: next `start()` snaps. A running transition ends with `onTransitionEnd(true)` on the next step. |🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@history/2026/2026-04-22-gpu-transitions.md` around lines 56 - 64, The wording in the transition matrix is ambiguous about which transitions do not auto-pause the simulation; update the "color/size transition" entries to explicitly include link transitions so readers know link color/width changes don't pause simulation. Edit the table rows that currently read "color/size transition" to say something like "non-positional transitions (color, size, link color/width)" and keep references to render(), setConfigPartial, enableSimulation, transitionDuration, onSimulationPause, unpause(), and onTransitionEnd(true) intact so the behavior description and signals remain precise.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@history/2026/2026-04-22-gpu-transitions.md`:
- Around line 56-64: The wording in the transition matrix is ambiguous about
which transitions do not auto-pause the simulation; update the "color/size
transition" entries to explicitly include link transitions so readers know link
color/width changes don't pause simulation. Edit the table rows that currently
read "color/size transition" to say something like "non-positional transitions
(color, size, link color/width)" and keep references to render(),
setConfigPartial, enableSimulation, transitionDuration, onSimulationPause,
unpause(), and onTransitionEnd(true) intact so the behavior description and
signals remain precise.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 859acddc-8377-462b-b7c0-7d1db727bff8
📒 Files selected for processing (3)
.agents/skills/history/SKILL.mdCONTRIBUTING.mdhistory/2026/2026-04-22-gpu-transitions.md
✅ Files skipped from review due to trivial changes (2)
- .agents/skills/history/SKILL.md
- CONTRIBUTING.md
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@history/2026/2026-04-22-gpu-transitions.md`:
- Around line 61-64: Replace the misleading phrase "next `start()` snaps" with
render-cycle wording: explain that changing duration to ≤0 causes the next
update + render cycle to snap immediately rather than `start()` triggering the
snap, and note that `start()` only changes simulation state (see start() and
render() in src/index.ts); keep references to transition lifecycle callbacks
like onTransitionEnd(true) and onSimulationEnd so integrators understand when
the snap/cleanup occurs relative to those events.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 670f16c6-48b7-4229-a36f-be1929a885dc
📒 Files selected for processing (3)
.agents/skills/history/SKILL.mdCONTRIBUTING.mdhistory/2026/2026-04-22-gpu-transitions.md
✅ Files skipped from review due to trivial changes (2)
- CONTRIBUTING.md
- .agents/skills/history/SKILL.md
…isely Signed-off-by: Stukova Olya <stukova.o@gmail.com>
There was a problem hiding this comment.
🧹 Nitpick comments (1)
history/2026/2026-04-22-gpu-transitions.md (1)
30-30: Broaden “color/size” wording to explicitly include link transitions.Line 30 and Line 64 currently read as if only point color/size transitions are non-pausing. Since this PR also transitions link color/width, a small wording tweak would prevent ambiguity.
✏️ Suggested wording update
-**Auto-pause (position transitions only).** When `render()` sees a pending **position** transition with `transitionDuration > 0` and a running simulation (and it's not the first render), the simulation pauses before the transition starts and `onSimulationPause` fires. The simulation **stays paused after the transition ends** — `setPointPositions()` signals the user wants to explore a specific layout, not have forces immediately pull nodes away from it. Call `unpause()` to resume explicitly. Color and size transitions don't compete with force updates and never pause the simulation. +**Auto-pause (position transitions only).** When `render()` sees a pending **position** transition with `transitionDuration > 0` and a running simulation (and it's not the first render), the simulation pauses before the transition starts and `onSimulationPause` fires. The simulation **stays paused after the transition ends** — `setPointPositions()` signals the user wants to explore a specific layout, not have forces immediately pull nodes away from it. Call `unpause()` to resume explicitly. Point color/size and link color/width transitions don't compete with force updates and never pause the simulation.-| `true` + `>0` (simulation, **color/size** transition) | Animate. Simulation keeps running — color/size transitions don't compete with forces. | `→ false`: stops simulation, destroys simulation-only resources, fires `onSimulationEnd`. | `→ ≤0`: the next update + render cycle snaps immediately instead of animating. `start()` only changes simulation state. If a transition is already running, the next render frame interrupts it with `onTransitionEnd(true)`; `onSimulationEnd` still only fires if the simulation is later stopped or finishes. | +| `true` + `>0` (simulation, **color/size/link color/width** transition) | Animate. Simulation keeps running — these transitions don't compete with forces. | `→ false`: stops simulation, destroys simulation-only resources, fires `onSimulationEnd`. | `→ ≤0`: the next update + render cycle snaps immediately instead of animating. `start()` only changes simulation state. If a transition is already running, the next render frame interrupts it with `onTransitionEnd(true)`; `onSimulationEnd` still only fires if the simulation is later stopped or finishes. |Also applies to: 64-64
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@history/2026/2026-04-22-gpu-transitions.md` at line 30, The paragraph around render(), transitionDuration, setPointPositions(), onSimulationPause and unpause should be reworded to explicitly state that non-pausing transitions include point color/size and link color/width (or link color/width/size as applicable) so readers don’t assume only point visuals are exempt; update the sentence that currently reads “Color and size transitions don't compete with force updates and never pause the simulation” to something like “Color and size transitions (and link color/width transitions) don't compete with force updates and never pause the simulation,” and apply the same wording change at the other occurrence referenced (line ~64).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@history/2026/2026-04-22-gpu-transitions.md`:
- Line 30: The paragraph around render(), transitionDuration,
setPointPositions(), onSimulationPause and unpause should be reworded to
explicitly state that non-pausing transitions include point color/size and link
color/width (or link color/width/size as applicable) so readers don’t assume
only point visuals are exempt; update the sentence that currently reads “Color
and size transitions don't compete with force updates and never pause the
simulation” to something like “Color and size transitions (and link color/width
transitions) don't compete with force updates and never pause the simulation,”
and apply the same wording change at the other occurrence referenced (line ~64).
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: e344e4b9-1c43-473c-a669-ee6d3482b33d
📒 Files selected for processing (1)
history/2026/2026-04-22-gpu-transitions.md
Bring back repeated start(alpha) calls as a reheat mechanism while keeping onSimulationStart limited to stopped/paused -> running. Signed-off-by: Stukova Olya <stukova.o@gmail.com>
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
src/index.ts (1)
740-750:⚠️ Potential issue | 🟠 MajorDrop only the invalid position transition before auto-pausing.
Line 750 calls
abort(), which clears the entire pending queue and any active cycle. If the invalid piece is onlyTransitionProperty.Positions, this also discards unrelated point/link color or size/width transitions. Because Lines 740-745 pause first, it can also leave the simulation paused even though no transition survives.🔧 Narrow the fallback to positions and validate before pausing
- if (this.transition.isPendingFor(TransitionProperty.Positions) && - this.store.isSimulationRunning && - this.config.transitionDuration > 0 && - !this._isFirstRenderAfterInit) { - this.store.isSimulationRunning = false - this.config.onSimulationPause?.() - } - const currentPositionTexture = this.points?.currentPositionTexture - if (this.transition.isPending && (!currentPositionTexture || currentPositionTexture.destroyed)) { - this.transition.abort() + const hasValidPositionTexture = !!currentPositionTexture && !currentPositionTexture.destroyed + if (this.transition.isPendingFor(TransitionProperty.Positions) && !hasValidPositionTexture) { + this.transition.dequeue?.(TransitionProperty.Positions) + } + + if (this.transition.isPendingFor(TransitionProperty.Positions) && + hasValidPositionTexture && + this.store.isSimulationRunning && + this.config.transitionDuration > 0 && + !this._isFirstRenderAfterInit) { + this.store.isSimulationRunning = false + this.config.onSimulationPause?.() }Based on learnings, the pause itself is intentional for valid position transitions; the issue here is the broad abort/ordering, not the lack of auto-resume.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/index.ts` around lines 740 - 750, The code currently calls this.transition.abort() which clears all pending and active transitions (including non-position ones) and can leave the simulation paused incorrectly; instead drop only invalid position transitions and validate before pausing: locate the checks using this.transition.isPendingFor(TransitionProperty.Positions) and currentPositionTexture, replace the global this.transition.abort() call with a targeted removal of only pending position transitions (e.g., a method like dropPendingFor or abortPendingFor with TransitionProperty.Positions), then re-check that a position transition still exists before setting this.store.isSimulationRunning = false and calling this.config.onSimulationPause?.().
🤖 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/index.ts`:
- Around line 2017-2020: The current hover gate unconditionally returns when
this.transition.isActive, which also blocks hover during color-only transitions;
change the condition to only block when the active transition actually affects
geometry (e.g. points/links sizes/positions). Replace the unconditional check
using this.transition.isActive with a predicate that checks geometry-affecting
transitions (for example: if (this.transition.isActive &&
this.transition.affectsGeometry) return), or if your Transition type uses a
kind/flags field use that (e.g. this.transition.kind === 'geometry' or
this.transition.hasFlag('geometry')). If such an indicator doesn't exist add a
boolean like affectsGeometry to the Transition class/instance and set it when
starting transitions that change vertex geometry so hover/cursor updates remain
enabled for color-only transitions.
---
Duplicate comments:
In `@src/index.ts`:
- Around line 740-750: The code currently calls this.transition.abort() which
clears all pending and active transitions (including non-position ones) and can
leave the simulation paused incorrectly; instead drop only invalid position
transitions and validate before pausing: locate the checks using
this.transition.isPendingFor(TransitionProperty.Positions) and
currentPositionTexture, replace the global this.transition.abort() call with a
targeted removal of only pending position transitions (e.g., a method like
dropPendingFor or abortPendingFor with TransitionProperty.Positions), then
re-check that a position transition still exists before setting
this.store.isSimulationRunning = false and calling
this.config.onSimulationPause?.().
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 3dfff0bf-9c95-434e-9a0e-e48d4f118930
📒 Files selected for processing (2)
src/index.tssrc/stories/3. api-reference.mdx
🚧 Files skipped from review as they are similar to previous changes (1)
- src/stories/3. api-reference.mdx
Only block hover for point size transitions. Block drag start for position and point size transitions, and update docs to match the current behavior. Signed-off-by: Stukova Olya <stukova.o@gmail.com>
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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/index.ts`:
- Around line 1543-1560: When enabling simulation (the block that calls
this.transition.end(true)), also remove any queued "Positions" transitions that
were enqueued by setPointPositions so they don't resume on the next render;
locate the Transition instance (this.transition) and either call its specific
dequeue/abort API for position-type entries (e.g., abort('positions') or
abortPendingOfType('Positions')) or filter its pending queue (e.g., remove
entries whose type/command equals Positions) so only non-position pending
transitions survive; this prevents a queued Positions transition from running
after setConfigPartial({ enableSimulation: true }) and causing an immediate
auto-pause in render().
- Around line 1184-1190: Before re-enabling physics in start(), ensure any
active position transitions are finished to avoid GPU interpolation overwriting
simulation steps: before setting this.store.isSimulationRunning = true (i.e., at
the top of the shown start() snippet), check for and end active position
transitions for TransitionProperty.Positions—mirror the unpause() guard by
calling the same transition termination path (e.g.,
this.transitions.end(TransitionProperty.Positions) or the existing isActive/stop
helper used elsewhere) so the force step runs against final simulation
coordinates.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: ed07619a-6e54-460d-83fb-b1d1fe989bdb
📒 Files selected for processing (5)
history/2026/2026-04-22-gpu-transitions.mdsrc/index.tssrc/modules/Drag/index.tssrc/stories/2. configuration.mdxsrc/stories/3. api-reference.mdx
✅ Files skipped from review due to trivial changes (1)
- src/stories/2. configuration.mdx
🚧 Files skipped from review as they are similar to previous changes (1)
- src/modules/Drag/index.ts
start() and unpause() now end transitions only when positions are animating. Enabling simulation also drops any queued position transition to prevent immediate auto-pause on next render(). Signed-off-by: Stukova Olya <stukova.o@gmail.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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/Transition/index.ts`:
- Around line 155-156: The progress calculation can go negative when nowMs <
this.startTime, causing invalid inputs to this.applyEasing; change the
computation of linear in Transition to clamp the lower bound to 0 (e.g., use
Math.max or a clamp) so linear = clamp((nowMs - this.startTime) /
transitionDuration, 0, 1) before calling this.applyEasing; update the expression
near the existing linear/ eased lines to ensure transitionDuration is used and
avoid division by zero if needed.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: bc9546c7-d67c-431f-bbe5-a504b989bd32
📒 Files selected for processing (2)
src/index.tssrc/modules/Transition/index.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- src/index.ts
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>
Member
Author
Upgrade prompt with migration guide:
|
Signed-off-by: Stukova Olya <stukova.o@gmail.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
migration-notes.md (1)
206-207: 💤 Low valueConsider using American English "afterward" instead of "afterwards".
The static analysis tool flagged this as a locale preference. "Afterward" is standard in American English documentation.
📝 Suggested fix
-**Auto-pause.** When a position transition runs while the simulation is on, the simulation auto-pauses for the transition and **stays paused afterwards**. Call `graph.unpause()` to resume forces. Set `transitionDuration: 0` to keep the v2 snap-and-keep-running behavior. +**Auto-pause.** When a position transition runs while the simulation is on, the simulation auto-pauses for the transition and **stays paused afterward**. Call `graph.unpause()` to resume forces. Set `transitionDuration: 0` to keep the v2 snap-and-keep-running behavior.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@migration-notes.md` around lines 206 - 207, Update the phrasing in migration-notes.md to use American English by replacing "afterwards" with "afterward" in the Auto-pause note; keep the rest of the sentence and retain references to graph.unpause() and transitionDuration: 0 so the instruction still reads: call graph.unpause() to resume forces and set transitionDuration: 0 to preserve v2 snap-and-keep-running behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/stories/2`. configuration.mdx:
- Line 110: The example for onTransitionEnd is misleading about
enableSimulation; update the sentence so it only cites enabling simulation
mid-flight (not toggling either direction) as an interruption example — e.g.,
"enabling enableSimulation mid-flight" — and keep transitionDuration being set
to 0 as a separate interruption example; modify the text around onTransitionEnd
to explicitly reference enableSimulation and transitionDuration to avoid
implying turning simulation off will interrupt transitions.
---
Nitpick comments:
In `@migration-notes.md`:
- Around line 206-207: Update the phrasing in migration-notes.md to use American
English by replacing "afterwards" with "afterward" in the Auto-pause note; keep
the rest of the sentence and retain references to graph.unpause() and
transitionDuration: 0 so the instruction still reads: call graph.unpause() to
resume forces and set transitionDuration: 0 to preserve v2 snap-and-keep-running
behavior.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 2bc1f3d6-3016-4518-af11-c47dfca1c521
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (12)
.agents/skills/history/SKILL.md.claude/skills/history/SKILL.mdhistory/2026/2026-04-22-gpu-transitions.mdmigration-notes.mdpackage.jsonsrc/index.tssrc/modules/Lines/index.tssrc/modules/Points/index.tssrc/modules/Shared/buffer.tssrc/modules/Transition/index.tssrc/stories/2. configuration.mdxsrc/stories/3. api-reference.mdx
✅ Files skipped from review due to trivial changes (3)
- package.json
- .claude/skills/history/SKILL.md
- history/2026/2026-04-22-gpu-transitions.md
| | onSimulationUnpause | Called when simulation unpauses | | ||
| | onTransitionStart | Called when an animated transition starts (see [Transitions](#transitions)) | | ||
| | onTransition | Called on every transition frame with the eased `progress` value in the `[0, 1]` range: `(progress: number) => void` | | ||
| | onTransitionEnd | Called when a transition ends. `interrupted` is `true` when the transition was replaced or ended early (e.g. by setting `transitionDuration` to `0` mid-flight, or toggling `enableSimulation` mid-flight): `(interrupted: boolean) => void` | |
There was a problem hiding this comment.
Clarify the enableSimulation interruption example.
Line 110 currently reads as if either toggle direction interrupts transitions, but Line 149 says turning simulation off does not interrupt. Narrow the example to enabling simulation mid-flight.
📝 Suggested wording
-| onTransitionEnd | Called when a transition ends. `interrupted` is `true` when the transition was replaced or ended early (e.g. by setting `transitionDuration` to `0` mid-flight, or toggling `enableSimulation` mid-flight): `(interrupted: boolean) => void` |
+| onTransitionEnd | Called when a transition ends. `interrupted` is `true` when the transition was replaced or ended early (e.g. by setting `transitionDuration` to `0` mid-flight, or turning `enableSimulation` on mid-flight): `(interrupted: boolean) => void` |📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| | onTransitionEnd | Called when a transition ends. `interrupted` is `true` when the transition was replaced or ended early (e.g. by setting `transitionDuration` to `0` mid-flight, or toggling `enableSimulation` mid-flight): `(interrupted: boolean) => void` | | |
| | onTransitionEnd | Called when a transition ends. `interrupted` is `true` when the transition was replaced or ended early (e.g. by setting `transitionDuration` to `0` mid-flight, or turning `enableSimulation` on mid-flight): `(interrupted: boolean) => void` | |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/stories/2`. configuration.mdx at line 110, The example for
onTransitionEnd is misleading about enableSimulation; update the sentence so it
only cites enabling simulation mid-flight (not toggling either direction) as an
interruption example — e.g., "enabling enableSimulation mid-flight" — and keep
transitionDuration being set to 0 as a separate interruption example; modify the
text around onTransitionEnd to explicitly reference enableSimulation and
transitionDuration to avoid implying turning simulation off will interrupt
transitions.
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
1 participant
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 GPU-driven transitions for point positions, colors, sizes, and link colors/widths. Also makes
enableSimulationruntime-switchable so simulation can be toggled without recreating the graph.Animated transitions
When you update graph data and call
render(), the change now animates instead of snapping. Position transitions interpolate on the GPU; color/size/width transitions blend in the draw shader.All updates queued between two
render()calls animate together as one synchronized cycle.Notes
fitViewduring a position transition frames the target positions (the latestsetPointPositionsargument), not the interpolated ones currently on screen. Same forfitViewByPointIndices.start()andunpause()interrupt active position transitions (onTransitionEnd(true)fires); color/size/link transitions keep playing.TransitionEasingis exported from the package.Lifecycle callbacks
Runtime simulation toggle
enableSimulationis no longer init-only — flip it at any time:New story
src/stories/transition/— a 200k-point cloud sampled from a painting, auto-looping between the picture layout and a sequence of tile scatters.Breaking changes
setPointPositions(...) + render()now animates over 800ms instead of snapping immediately. To keep the old snap behavior, initialize withtransitionDuration: 0, or disable it for a single update:Position transitions auto-pause the simulation. When
setPointPositions()triggers a transition while the simulation is running, the simulation pauses for the transition and stays paused afterwards — callingsetPointPositions()is treated as intent to explore a specific layout, not feed the physics. Callgraph.unpause()to resume.enableSimulationis no longer init-only. Previously listed as init-only and ignored bysetConfig(). Now applied at runtime. Code that relied on it being a no-op after init will see real behavior changes.Contributor tooling:
history/and/historyThis PR also introduces a lightweight place to capture the why behind non-trivial changes, separate from commit messages:
history/folder withREADME.md(conventions) andPROMPT.md(LLM template). The first entry,history/2026/2026-04-22-gpu-transitions.md, documents this PR./historyClaude Code slash command at.claude/skills/history/SKILL.md:/history <why>— drafts a new entry from recent commits./history --update— revises the latest entry in place when follow-up commits extend the same topic, without re-asking for a "why".CONTRIBUTING.mdpoints to all of the above.Summary by CodeRabbit
New Features
Behavior Changes
Documentation