cornerstoneTools icon indicating copy to clipboard operation
cornerstoneTools copied to clipboard

Add / Remove Nodes in FreehandRoi Tool

Open pzehle opened this issue 4 years ago • 9 comments

Prerequisites

  • [x] Latest
  • [x] Are you reporting to the correct repository?
  • [x] No related issues, one of them got me no answers.

Description

I would like to know, if there is a possibility to add or remove nodes after the tool has been closed. I am thinking of it as getting the coords from the mouse, and on click, check the tool state and if there is an existing node, remove it (causes error) and re-render the state, and if there is no node, then add it and as well re-render the state.

Expected behavior:

When adding or removing nodes from a rendered tool, be able to add or remove this information from the tool state's object and re-render the tool state to update the view with/without the selected nodes.

Actual behavior:

After removing or adding a node from the tool state's object, changing the lines coordinates and re-rendering, after a click or movement the tool crashes stating Undefined is not an object.

As usual, thanks in advance.

pzehle avatar Mar 17 '20 10:03 pzehle

Do you have an example implementation that is causing this issue? I too would have guessed that removing a point from a FreehandRoi annotation and triggering a re-render would do the trick:

https://github.com/cornerstonejs/cornerstoneTools/blob/master/src/tools/annotation/FreehandRoiTool.js#L107-L109

dannyrb avatar Mar 17 '20 18:03 dannyrb

@dannyrb thanks for your response. What I am currently doing as an example on my project is:

Every time I mark a point with the Freehand tool, I am saving the state of the tool in React's state. At any moment, I can press 'cmd + z' and the last node will get deleted from the state, with the following code:

undoPoint = () => {
	var markings = cornerstoneTools.getToolState(this.element, 'FreehandRoi');
	var data = markings.data[0];
	if(data.handles.points.length === 1) {
		return false;
	} else {
		data.handles.points.pop();
		let latestPoint = data.handles.points.length - 1;
		data.handles.points[latestPoint].lines = [];
		cornerstoneTools.clearToolState(this.element, 'FreehandRoi');
		cornerstoneTools.addToolState(this.element, 'FreehandRoi', data);
		cornerstone.updateImage(this.element);
	}
}

I am removing the last available node's lines array as the last node should not have lines, AFAIK. This all works correctly, but after continuing with the marking, the next point I mark makes my application crash and output the following error:

cornerstoneTools.js:32070 Uncaught TypeError: Cannot read property 'lines' of undefined
    at FreehandRoiTool._addPoint (cornerstoneTools.js:32070)
    at FreehandRoiTool._drawingMouseDownCallback (cornerstoneTools.js:31756)
    at triggerEvent (cornerstoneTools.js:48197)
    at HTMLDivElement.mouseDown (cornerstoneTools.js:8356)
_addPoint @ cornerstoneTools.js:32070
_drawingMouseDownCallback @ cornerstoneTools.js:31756
triggerEvent @ cornerstoneTools.js:48197
mouseDown @ cornerstoneTools.js:8356
cornerstoneTools.js:32070 Uncaught TypeError: Cannot read property 'lines' of undefined
    at FreehandRoiTool._addPoint (cornerstoneTools.js:32070)
    at FreehandRoiTool._addPointPencilMode (cornerstoneTools.js:32106)
    at FreehandRoiTool._drawingDrag (cornerstoneTools.js:31685)
    at FreehandRoiTool._drawingMouseDragCallback (cornerstoneTools.js:31656)
    at triggerEvent (cornerstoneTools.js:48197)
    at HTMLDocument.onMouseMove (cornerstoneTools.js:8398)

When comparing two objects, the first being an object being generated as I mark lesions, and the second object being the object output after the undo function, they seem exactly the same. I don't really know if internally there is a points or lines counter on the cornerstoneTools' object or something that is making this error appear after removing a node from the array.

I am putting here a small vide as an example of the things I described above.

I hope this can make my issue clearer. Thanks again.

ezgif-5-500e6ba332e9

pzehle avatar Mar 20 '20 12:03 pzehle

@dannyrb Hi again Danny, I know these are difficult times, but I just want to know if there is anything you could help me with, regarding the issue I am having.

Thank you again.

pzehle avatar Mar 31 '20 09:03 pzehle

My best guess is that you're not cleaning up event listeners. The state may "look" identical, but any references in event listeners were cleared.

We recently created a PR that establishes a library level way to "cancel" an in-progress placement. It drops any handles / annotations currently being moved, and clears related listeners.

For "complete" measurements, this is good enough. For multi-step tools like freehand that can have an "incomplete" state, you have to manually delete them after a cancel as their is (currently) no way to resume editing to achieve a "complete" state.

To achieve what you want, you almost have to build this kind of functionality into the tool (or a forked 3rd party tool), or establish a library level pattern to support it.

dannyrb avatar Mar 31 '20 10:03 dannyrb

Thank you for your answer. So, to do what I want to do (undo markings) would be pretty hard I understand?

On a related matter, the way I am trying to do this, you think it would work once the tool has a completed state? I mean, to add or remove nodes once the tool has finished its marking?

Thank you.

pzehle avatar Mar 31 '20 10:03 pzehle

I try not to classify problems as easy/hard. I see two approaches:

  • Enhance the tool
  • Enhance the library

If we define expectations, then we could create a feature branch where we PR failing tests that assert those expectations, then eventually PR the implementation that causes those tests to pass.

A bit more rigor than usual for this repository, but it would help make sure an implementer is working toward something we would be okay merging.


It "should" work if you have complete annotations and no straggling interactions/listeners. If you encounter issues, it would be because of a bug.

dannyrb avatar Mar 31 '20 10:03 dannyrb

export const undoFreehandPoint = (toolName, enableElement) => {
  if (!toolName || !enableElement) return;
  try {
    const tool = cornerstoneTools.getToolForElement(enableElement, toolName);
    const activeDrawing = (tool || {})._activeDrawingToolReference;
    if (
      activeDrawing &&
      activeDrawing.handles &&
      (activeDrawing.handles.points || []).length > 1
    ) {
      const configuration = tool._configuration;

      if (configuration.currentHandle > 1) {
        const len = activeDrawing.handles.points.length;
        activeDrawing.handles.points.length = len - 1;
        activeDrawing.handles.points[len - 2].lines = [];
        configuration.currentHandle = configuration.currentHandle - 1;
        cornerstone.updateImage(enableElement);
      }
    }
  } catch (error) {
    console.log(error);
  }
};

HoAnhVan avatar Dec 10 '20 07:12 HoAnhVan

Hi @pzehle, thanks for opening this issue and providing a detailed explanation of the behavior you're seeing. It was helpful to detect some concerns in our prototypes too.

@dannyrb mentioned that there may be issues with event listeners not being properly cleaned up. They also mentioned a PR that provides a way to "cancel" an in-progress placement and clears any related listeners. This might be a useful approach to consider.

As far as I know, adding or removing nodes after the tool has finished its marking should be possible as long as there are no incomplete annotations or straggling interactions/listeners. If you encounter any issues, it might be due to a bug. I also noticed that you mentioned using the clearToolState function to remove all states from the state manager corresponding to the name and element of the tool. This is indeed one way to solve similar issues.

Let me know if the following helps or if you have any further questions:

https://stackoverflow.com/a/73204850/2371987

FMCalisto avatar Mar 14 '23 20:03 FMCalisto

@FMCalisto thanks for the comment. What are the odds? I left my project about 3 years ago, and I just started working on it a couple of days ago, then you comment on an issue 3 years old. I'm shocked.

Anyway, thanks for the explanation, now that I will be working on it again maybe if I come up with a solution I will comment it here. I tried to use the new Cornerstone3D library, but completely failed to use it with React, so I will be used the good ol' tested implementation. Thanks!

pzehle avatar Mar 14 '23 22:03 pzehle