feat: add referenceCursors tool

[doepnern]

Hey, I really love this repository and especially the improved API in the new version.
I created this tool for my personal use, its purpose is mostly to act like a linking of camera position and showing a reference to what your cursor is currently hovering over. It also helps a lot when you have multiple series which are not in the same Orientation, to have a feel for what you are looking at in different orientations. This is somewhat similar to the crosshair that exists, but it also works with Stack viewports, which I use it mostly for.
Would there be an interest in adding this tool, I would take the time to write tests as well if that is the case.
Thank you for your great library anyway!
Niclas

p.s. here is a small video
https://user-images.githubusercontent.com/32524613/199220760-a20ea8e2-9efb-4335-a763-0325ecba5f43.mp4

[netlify]

✅ Deploy Preview for cornerstone-3d-docs ready!

Name	Link
🔨 Latest commit	5b2da38
🔍 Latest deploy log	https://app.netlify.com/sites/cornerstone-3d-docs/deploys/637e187d7a021a000816e987
😎 Deploy Preview	https://deploy-preview-275--cornerstone-3d-docs.netlify.app
📱 Preview on mobile	Toggle QR Code... Use your smartphone camera to open QR code link.

---

To edit notification comments on pull requests, go to your Netlify site settings.

[sedghi]

@doepnern Hey
Thanks for your contribution. First of all, this is really impressive PR.
Yes, we have seen this tool on other viewers and would love to also include it in Cornerstone3D and eventually on OHIF.

I will go ahead and review this now

[doepnern]

I am not completely sure if I'm happy with how this works right now. Maybe it would be easier to not use the annotation tool at all as this doesnt use any of the features where having the worldPosition of the annotation is useful and the easiest way to render the crosshair would be to just have the current enabled element where the mouse is over and the canvas position and then just calculate the world position for every viewport on render.

[doepnern]

And in the current version there would need to be a cleanup of the listeners with a setMouseOverElement(null), I am not quite sure where to put it as onSetToolDisabled ist not always called (for example not if the toolGroup is destroyed

[sedghi]

So I did my first pass to understand what is going on and basically put my questions also, I have other comments as well, but I prefer to get your opinions/answers for my questions before giving you the full review.

PS: I first thought, we can avoid adding an annotation to the state, since basically it does not serve as any purpose, but then our rendering loop handles each element separately so I think right now it is fine, if we just addAnnotation

Again, great job, and welcome

[doepnern]

Okay, so I hopefully applied most of the changes you suggested. Thanks a lot for the extensive feedback.

The reason I was working with imageChange events:
Currently on every mouseMove the annotations world position is changed which works fine. However if the viewport over which the cursor is currently positioned is for example scrolled via StackScrollMouseWheelTool, then no mouseMove event is triggered but the world position of the annotation should change. If all viewports would be rerendered we could just calculate it based on the canvasPosition but only the Viewport being scrolled gets rerended.

This leads to the cursor being stale until the next mouseMove is triggered. Also this will prevent the stacks to sync during scrolling for the same reason.

The only way I found around adding the event listeners now is:
When a Stack change happens, it will cause the annotation on the viewport which was scrolled to be rendered

• check during render of annotation if canvasPosition still corresponds to saved WorldPosition (this wont be true if a StackChange happened)
• if stackChange happened, update worldPosition and trigger rerender for all viewports

There after some longer debugging I noticed that calling triggerAnnotationRerender during an annotationRender will cause the whole tool to break (even when excluding the currently rendering viewport). For that reason, I had to wrap the call into a setTimeout. Feel kind of "hacky" but I just don't see a completely easy way to do this.

You can test the whole problem by commenting out the block in the renderAnnotation checking for the stackChange. When using StackScrollWheel the cursor won't be synced and the stack states won't be synced until a mouseMove is triggered.

About the cursor being rendered in all viewports:
I added it to the viewport with the cursor as well now.
I'm not sure what to do with the mouse though. Removing it completely might be confusing as all cursors will look the same then. Maybe the main cursor should have a different design to not confuse the user. (I thought about a different color, but then how do you know which one is the main on 2 viewports and how would you handle custom color choices). I'm very open to suggestions.

[doepnern]

Thanks for all the suggestions. onCameraModified is actually a perfect way. It would be really helpful if you would have a small overview of functions a tool can implement at some point. What I noticed is, that when you set the tool to passive there are errors in the console when clicking on the screen for functions not being implemented even though I'm now extending the AnnotationDisplayTool. You can try it out by setting the tool to passive in the example

[sedghi]

This check which is basically checking annotation manager if there is one exists can be done very first thing at the top of this method and avoid all the object and logics up here

[sedghi]

Thanks for the updates, this is good to go except I tried it and for the active viewport (the one with mouse) I see two cursors for some reason. Does this happen on your machine too?

Update: I think I know what is going on, the active tool is set to be Pan, so there is Pan SVG Cursor, but also we decided to show the cursor for the active viewport too, so I guess you need to hide the cursor when the tool is set to active as a call back setToolActive, see other annotation tools for how to hide cursor

Update2: Basically your tool and some other tools only should have two states: Active and Disabled. Enabled, and Passive does not have meaning here. Previously we had mixins for this, that redirected setToolPassive to setToolDisabled (https://github.com/cornerstonejs/cornerstoneTools/blob/master/src/mixins/activeOrDisabledBinaryTool.js) let me see what I can do

8a3d4587-7a93-4b6d-a04b-8a57d10330cb.webm

[sedghi]

Another comment after thinking, your tool has active state, meaning that one should activate it, so I expect the demo makes setToolActive which changes the cursor (right now it shows pan)

[doepnern]

I actually like the option of still having the cursor of another tool. In actual usage this tool would mostly be used while still having a tool active, so I dont like hiding the primary tool that much. I changed the look of the tool in the element with the cursor to hopefully not conflict with another tool and added a configuration to disable the other tools cursor. I was having a lot of issues with curors being replaced depending on the order in which tools are enabled. Would it maybe be useful to only change the cursor to the default one if there is no active primary cursor(I added a check for that in the ToolGroup). Feel free to remove that if you dont feel like this is wanted.

[sedghi]

default for disableCursor to be false hmmm, going back to the discussion on the cursors for this tool, I don't understand why one needs to have the pan cursor on and also the cursor for this tool at the same time. It is kind of not in harmony with the rest of the library in which the active tool on the mouse primary claims the cursor and not other tools. Can we set this tool to behave the same way by default? that it overwrite its cursor by default?

[sedghi]

Ignore this comment

[sedghi]

This might cause a bug when configuration changes, since you are only assigning it on the construtor, maybe always use this.configuration.disableCursor?

[sedghi]

typo

[sedghi]

Almost there, see my comments

1. Is there any usecase for having positionSync off? I feel like user wants it always on, and if it is off, it will cause confusion when other viewports not showing the same image

2. The demo doesn't work when switching between disable cursor on and off (probably related to my comment for the constructor)

3. There is a one pixel offset between the pan cursor and your tool cursor, do you think that is easy to fix?

[doepnern]

@sedghi
Good points,

1. I think mostly the sync is wanted, however it might interfere in some cases with some other tools or syncing mechanisms. Also the cursor is only displayed in the other viewport if the closest distance is smaller than displayThreshold, so there will be no cursor displayed if it doesn't make sense to display it.
2. This is a more complicated point and I decided for this implementation because the logic of hiding the cursor is pretty impractical to work with, namely:
   https://github.com/cornerstonejs/cornerstone3D-beta/blob/a923b6888e80838edc2253fb6a82f5827b90724e/packages/tools/src/cursors/elementCursor.ts#L17-L37
   Problems I had:

• Calling hideElementCursor twice without a resetElementCursor will set cursors[0] and cursors [1] to "none" -> This will lead to resetElementCursor to never work again (as it just sets the cursor to cursor[1]) until some other tool calls setElementCursor

This will also be a problem if 2 tools want to call hideElementCursor

So one would need to be sure to always clean up hideElemetCursor by calling resetElementCursor. As there is no onToolConfigurationChange Event the easiest way I found to do that was to set the internal _disableCursorEnabled every time the tool is activated and call the cleanup when the tool is disabled depending on that state. If you want to change the setting you have to deactivate and activate the tool once.

3. No idea on this one, I think it might have something to do with how the svgCursors are drawn in differentce to the Annotations

[sedghi]

Yeah the cursor API is probably the worst API in the whole repo, and I tried to fix it couple of times but initial implementation does not makes sense at all (and not my fault :D)

I think this is good to go

Thanks again! and looking forward to your next contribution!

[sedghi]

Great work