fix(measurements): The image stack sync tool fails to work on non-FOR instances and hangs the browser #642
Conversation
✅ Deploy Preview for cornerstone-3d-docs ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
wayfarer3130
changed the title
| } | ||
| } catch (ex) { | ||
| console.warn(`Synchronizer, for: ${this._eventName}`, ex); | ||
| } finally { | ||
| this._ignoreFiredEvents = false; | ||
| if (promises.length) { |
There was a problem hiding this comment.
@wayfarer3130 and I reviewed this on a call; this is causing issues in the TVM mode. This is also an important fix that was making the browser hang so we want to keep it.
Mentioning here just so we know we need to fix it before merging
There was a problem hiding this comment.
Use the viewport status instead of making this async
There was a problem hiding this comment.
Seems to be ok in the TVM mode now, at least I can't reproduce any hangs. I believe the issue was caused by multiple initial renders causing a loop, and the existing OHIF changes to prevent extra renders resolved the base issue.
There was a problem hiding this comment.
@mateusfreira can you try again, It seems to work with tmtv now
There was a problem hiding this comment.
@mateusfreira - you have to try it with the merge update branch of FlexView, as it was changes outside of CS3D that fixed the hang issue.
There was a problem hiding this comment.
@mateusfreira can you try and let me know?
There was a problem hiding this comment.
@sedghi and @wayfarer3130 I confirmed it is working as expected
packages/core/src/utilities/calculateViewportsSpatialRegistration.ts
Outdated
Show resolved
Hide resolved
| @@ -211,13 +212,20 @@ class Synchronizer { | |||
| if (targetIsSource) { | |||
| continue; | |||
| } | |||
|
|
|||
| this._eventHandler(this, sourceViewport, targetViewport, sourceEvent); | |||
| promises.push( | |||
There was a problem hiding this comment.
I don't know how to avoid this synchronization - the basic synchronizer design relies on being able to turn off secondary notifications so that only a single change applies, and if this doesn't use async, then you get an event from the target viewport, which changs the source viewport, which sends an event etc.
There was a problem hiding this comment.
The changes in this file fix the browser hanging issue.
| if (!registrationMatrixMat4) { | ||
| throw new Error( | ||
| `No registration matrix found for sourceViewport: ${sourceViewport.viewportId} and targetViewport: ${targetViewport.viewportId}, viewports with different frameOfReferenceUIDs must have a registration matrix in the registrationMetadataProvider. Use calculateViewportsRegistrationMatrix to calculate the matrix.` | ||
| console.log( | ||
| "Viewports still don't have registration:", | ||
| sourceViewport.viewportId, | ||
| targetViewport.viewportId | ||
| ); | ||
| return; | ||
| } |
There was a problem hiding this comment.
No, there are legitimate times this won't return a value, and you can't set it up to simply deny the display in the viewprot - for example, simple CR images won't have that, and may still want to default the viewport to have the stack sync on for when the HP is used for another image set.
packages/tools/src/synchronizers/callbacks/stackImageSyncCallback.ts
Outdated
Show resolved
Hide resolved
|
can you do a |
…x/image-stack-sync
…into fix/image-stack-sync
…into fix/image-stack-sync
Context
Synchronizers hang the browser if they do asynchronous actions.
The imageStackSync uses two algorithms, of which only one actually works correctly.
Changed to use a single algorithm, just different starting conditions depending on options.
Changes & Results
Made the synchronizers thread safe
Made the imageStackSync work with non same FOR images, and same FOR using current positions when configured to do so.
Testing
Display 4 up (2x2 mode) with the same FOR series in all frames, turn on image stack synchronizer - depending on the series selected, it may hang the browser.
Display 2+up mode with different FOR but coplanar series. Activate image stack sync (without doing anything special - just activate it, eg in the hanging protocol). Ensure that image stacks navigate together for coplanar series in stack mode.
Checklist
PR
semantic-release format and guidelines.
Code
[] My code has been well-documented (function documentation, inline comments,
etc.)
[] I have run the
yarn build:update-apito update the API documentation, and havecommitted the changes to this PR. (Read more here https://www.cornerstonejs.org/docs/contribute/update-api)
Public Documentation Updates
additions or removals.
Tested Environment