Fix Crosshair Update on scroll/pan/zoom. #84

JamesAPetts · 2019-12-13T13:38:57Z

This PR makes scroll/pan/zoom update the crosshairs correctly when using either vtkInteractorStyleMPRSlice or vtkInteractorStyleMPRCrosshairs.

Updates for vtkInteractorStyleMPRRotate will come in the future or during a large refactor of the iStyle framework we have in place.

Closes: OHIF#49

dannyrb · 2019-12-13T16:17:41Z

examples/VTKCrosshairsExample.js


      renderWindow.render();
+
+      // Its up to the layout manager of an app to know how many viewports are being created.


I think its the only reasonable way to do it. They all have no knowledge of each other. You need a management layer for synrchronised viewports which is out of the scope of the viewport component itself. We could eventually formalise this layer in react-vtkjs-viewport itself, but I'm wondering if it'd be too app-specific.

We cheat in cornerstone-tools by tracking enabledElements and exposing a configuration option for them to sync, or through methods that apply to "all" tracked enabledElements when used. This layer of abstraction doesn't exist here.

That's my round-about way of saying I agree with you.

dannyrb · 2019-12-13T16:19:59Z

examples/VTKCrosshairsExample.js

+
+        const api = apis[0];
+
+        api.svgWidgets.crosshairsWidget.resetCrosshairs(apis, 0);


I wonder what 0 is for 🤔
continues reading

Which viewport to as source of truth for the reset.

dannyrb · 2019-12-13T16:34:57Z

src/VTKViewport/vtkInteractorStyleMPRCrosshairs.js

  vtkInteractorStyleMPRSlice.extend(publicAPI, model, initialValues);

-  macro.setGet(publicAPI, model, ['callback']);
+  macro.setGet(publicAPI, model, ['callback', 'apis', 'apiIndex', 'onScroll']);


It feels weird to use apis in the interactorStyle. It feels like we're creating a synchronizer with apis as "viewport properties"

Yeah, its difficult, because we have to do a different thing depending on each API index. We have to have add some layer like this to the iStyle so it has any knowledge other viewports exist. We could try and normalise this approach in the future though.

dannyrb · 2019-12-13T16:37:36Z

src/VTKViewport/vtkInteractorStyleMPRSlice.js

+  function handleButtonPress() {
+    const { apis, apiIndex } = model;
+
+    if (apis && apis[apiIndex] && apis[apiIndex].type === 'VIEW2D') {


This makes sense, but I hadn't thought of it. We'll need some way to change interaction based on this. Likely creating tools that only support one or the other?

I hadn't thought of it either, pretty sure putting crosshairs on a View3D would just go kaput currently, View3D has recieved little love from the community as of yet anyway.

dannyrb · 2019-12-13T16:51:16Z

src/VTKViewport/vtkInteractorStyleMPRSlice.js

+
+      const api = apis[apiIndex];
+
+      api.svgWidgets.crosshairsWidget.updateCrosshairForApi(api);


Could use events or obersavables and have crosshairs monitor apis so it can self-update? Not pushing for that now, just adding ideas to the idea pile.

dannyrb · 2019-12-13T19:25:19Z

src/VTKViewport/vtkInteractorStyleMPRSlice.js

+      api.svgWidgets.crosshairsWidget.updateCrosshairForApi(api);
+    }
+
+    superButtonRelease();


super if calling an inherited constructor, but callback if passed as an arg and called here?

Yeah super is internal to the way the vtkjs classes are constructed. Callbacks we pass in when we set the interactor style allow us to hook into vtkjs from the app level but I don't like it haha.

dannyrb · 2019-12-13T19:27:00Z

src/VTKViewport/vtkInteractorStyleMPRSlice.js

-    if (onScroll) onScroll(slicePoint);
+    if (onScroll) {
+      onScroll({
+        slicePoint,


This isn't used by the model.onScroll below. Do we expect it to be passed for other onScroll implementations?

I left it as it already existed, I'm not sure of the original intention, we can review its existance later.

dannyrb · 2019-12-13T19:28:14Z

src/VTKViewport/vtkSVGCrosshairsWidget.js

-      `;
+
+    if (model.display) {
+      node.innerHTML = `


This SVG is odd

This PR didn't touch it, feel free to clean it up! :)

dannyrb

Fine to merge, but raises some interesting questions

ohif-bot · 2019-12-16T16:22:18Z

🎉 This PR is included in version 0.7.2 🎉

The release is available on:

Your semantic-release bot 📦🚀

JamesAPetts added 5 commits December 12, 2019 16:39

Refactoring crosshairs, but has slight drift.

f6fe74d

Fix drifiting issue.

5d26cc4

Working crosshair sync whilst panning.

dab8f27

fix: 🐛 Using mpr slice tools updates crosshairs

c1c8572

Closes: OHIF#49

Merge branch 'master' into crosshairUpdates

2093b42

dannyrb reviewed Dec 13, 2019

View reviewed changes

dannyrb approved these changes Dec 13, 2019

View reviewed changes

JamesAPetts merged commit 74c4656 into OHIF:master Dec 16, 2019

JamesAPetts added a commit that referenced this pull request Dec 16, 2019

fix: 🐛 Using mpr slice tools updates crosshairs (#84)

8051ae0

JamesAPetts mentioned this pull request Dec 16, 2019

fix: 🐛 Using mpr slice tools updates crosshairs (#84) #85

Merged

JamesAPetts added a commit that referenced this pull request Dec 16, 2019

fix: 🐛 Using mpr slice tools updates crosshairs (#84) (#85)

1ba78ef

ohif-bot added the released label Dec 16, 2019


		renderWindow.render();

		// Its up to the layout manager of an app to know how many viewports are being created.


		const api = apis[0];

		api.svgWidgets.crosshairsWidget.resetCrosshairs(apis, 0);


		const api = apis[apiIndex];

		api.svgWidgets.crosshairsWidget.updateCrosshairForApi(api);

Fix Crosshair Update on scroll/pan/zoom. #84

Fix Crosshair Update on scroll/pan/zoom. #84

Uh oh!

Conversation

JamesAPetts commented Dec 13, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dannyrb left a comment

Choose a reason for hiding this comment

Uh oh!

ohif-bot commented Dec 16, 2019

Uh oh!

Uh oh!