webgl: redraw when the SVG overlay finishes its async texture bake#644
Conversation
Toggling an overlay layer (ROIs / sulci / labels) re-bakes the overlay into a GPU texture asynchronously; SVGOverlay dispatches "update" with the new texture, and the surface swaps it into uniforms.overlay.value and re-dispatches "update" on itself (mriview_surface.js). But addSurf never wired the surface's "update" to the viewer's redraw, so the swap landed with no schedule() call. The menu schedules a redraw immediately on toggle — which races (and usually beats) the async bake — so the new overlay was not drawn until the next interaction. The switch appeared not to work. Wire surf "update" -> viewer.schedule() in addSurf so the overlay redraws as soon as the rebaked texture is committed. Complements the generation guard in #643 (which fixed out-of-order bakes); together a single toggle now deterministically lands on the current switch state. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
There was a problem hiding this comment.
Code Review
This pull request adds an event listener for the 'update' event on the surface object in mriview.js to trigger a redraw when the SVG overlay asynchronously re-bakes its texture. The review feedback points out that using this.schedule.bind(this) inline creates a new function instance each time, which can lead to memory leaks as it cannot be easily removed. It is recommended to cache the bound function reference so it can be properly cleaned up later.
| // The SVG overlay (ROIs/sulci/labels) re-bakes its texture asynchronously, then | ||
| // dispatches "update" on the surface once the new texture is swapped in. Redraw when | ||
| // that lands, otherwise a toggle isn't visible until the next interaction. | ||
| surf.addEventListener("update", this.schedule.bind(this)); |
There was a problem hiding this comment.
Using this.schedule.bind(this) directly in addEventListener creates a new function instance every time addSurf is called. This prevents the event listener from being properly removed later (e.g., in rmSurf), which can lead to memory leaks.
To fix this, cache the bound function on this._schedule so the same reference can be reused and cleaned up. Note that you should also add this.surfs[i].removeEventListener("update", this._schedule); to rmSurf to ensure proper cleanup.
| surf.addEventListener("update", this.schedule.bind(this)); | |
| this._schedule = this._schedule || this.schedule.bind(this); | |
| surf.addEventListener("update", this._schedule); |
Problem
Follow-up to #643. Even with the out-of-order race fixed, a single ROI/sulci/label toggle often still doesn't appear until you next move the brain.
SVGOverlay.update()re-bakes the overlay texture asynchronously; on completion the surface swaps it intouniforms.overlay.valueand dispatches"update"on itself (mriview_surface.js:295). ButaddSurfonly wires the surface'smix/allowTiltand the dataview'supdate— never the surface's own"update"toschedule(). So the texture swap lands with no redraw. The menu callsschedule()immediately on toggle, which races and usually beats the async bake, drawing the old texture; the corrected texture then sits unused until the next interaction.Fix
in
addSurf, so the overlay redraws the moment its rebaked texture is committed. Together with #643's generation guard, a single toggle now deterministically settles on the current switch state. One line;node --checkpasses.🤖 Generated with Claude Code