fix(layers): stop the layer cache from disposing the source image - #956
Merged
Merged
Conversation
ensureSameSpace returned the candidate itself when the grids already
matched, so the layer cached under `${parent}::${source}` and the source
dataset referenced one vtkImageData. The cache disposes what it owns, so
removing the layer or deleting the parent called delete() on an image the
source still held, leaving a wiped model reachable from the cache.
Return a geometry copy that shares the scalar array. vtk.js delete() only
clears the object's own model, so the array survives either owner and the
voxels are not duplicated. Both return paths now hand back an owned
object, so the ownership rule sits with the borrow instead of at each
call site.
A deleted vtkImageData keeps its closures but loses its model, so getPointData() returns undefined rather than throwing on access. The existing optional chain guarded the wrapper, not the model, so any image disposed without being evicted threw on every read.
addError builds its bug report from application state that the error may have just corrupted, and it does so before the message exists. A throw there costs the user the message and leaves the global handler reporting the reporter's own failure.
Layers two images that share a grid, so building the layer reuses the source image rather than resampling it. Removing the layer and building it again reads the source image a second time, which fails when removing the layer released the image the source dataset still owns. Adds the dataset menu test ids the DICOM browser already carries, and moves the MetaImage fixture writer next to the other manifest helpers so the zero spacing spec can share it.
✅ Deploy Preview for volview-dev ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
reallocateImage deleted the existing image before allocating its replacement, so a throw from allocateImageFromChunks left the chunk image holding a deleted vtkImageData. A re-import reaches this with the image already in the cache and nothing evicts it on failure, leaving every later read of that volume to find a wiped model. Allocate first and release the old buffer once the replacement exists, matching the order updateVTKImageData already uses.
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.
Problem
ensureSameSpacereturned its input unchanged when two images already shared anindex grid.
_addLayerhanded that object toaddVTKImageDataunder the layerid
${parent}::${source}, so two cache entries referenced onevtkImageData.The cache disposes what it owns, so removing the layer or deleting the parent
called
delete()on an image the source dataset still held. vtk.jsdelete()clears the model but keeps the closures, so the source's cached image became an
object whose every getter returns
undefined, and it stayed reachable from thecache for the rest of the session. Every later read of that dataset throws:
Cannot read properties of undefined (reading 'getScalars'),r.getPointData() is undefined, ort.extent is not iterable.To reproduce: load two images that share a grid, add one as a layer of the
other, remove the layer, then select the image that was the layer source.
addErrorbuilds a bug report from the image cache before it creates themessage, so reporting the first error threw and the message was never created.
The user saw no notification at all.
reallocateImagereaches the same end state by a second route: it released theexisting volume before allocating its replacement, so a throw from
allocateImageFromChunksleft a released image behind. A re-import reaches thiswith the image already in the cache and nothing evicts it on failure.
Change
ensureSameSpacereturns a copy that shares the candidate's scalar array, socallers own what they get back. vtk.js
delete()is not recursive, so thearray outlives either owner and the voxel buffer is not duplicated.
any other path degrades to a blank view rather than a crash.
addErrorfalls back to a placeholder bug report rather than throwing.reallocateImageallocates the replacement volume before releasing the oldbuffer, matching the order
updateVTKImageDataalready uses.