cornerstonejs / cornerstoneTools

A framework for tools built on top of Cornerstone.
https://tools.cornerstonejs.org/
MIT License
577 stars 455 forks source link

Is it necessary to add toolSelectedCallback feature? #1006

Open xingbofeng opened 5 years ago

xingbofeng commented 5 years ago

Prerequisites

For more information, see the CONTRIBUTING guide.

Description

There is the method that pointNearTool and toolSelectedCallback that I can only select the tool by mousedown near the handle, but I want to select the tool by mousedown inner the measurement by the pointInnerTool setted by myself. So I changed the mouseDown.js:

  const annotationToolsWithPointInnerClick = activeAndPassiveTools.filter(
    tool => {
      const toolState = getToolState(element, tool.name);
      const isInnerPoint = 
        toolState &&
        toolState.data &&
        tool.pointInnerTool &&
        toolState.data.some(data =>
          tool.pointInnerTool(element, data, coords, 'mouse')
        );
      return isInnerPoint;
    }
  );
  if (annotationToolsWithPointInnerClick.length > 0) {
    const firstToolInnerPoint = annotationToolsWithPointInnerClick[0];
    const toolState = getToolState(element, firstToolInnerPoint.name);
    const firstAnnotationInnerPoint = toolState.data.find(data => 
      firstToolInnerPoint.pointInnerTool(element, data, coords)
    );

    if (firstAnnotationInnerPoint && firstToolInnerPoint.innerSelectedCallback) {
      firstToolInnerPoint.innerSelectedCallback(
        evt,
        firstAnnotationInnerPoint,
        'mouse'
      );
      return;
    }
  }

I Have add my custom feature in my version(forked from the official), @dannyrb If you think it's necessary to add it , I will create PR.

dannyrb commented 5 years ago

It's cool to see other people playing around in the listeners/dispatchers ^_^

Tools that implement pointNearTool are basically saying that, if this is true, I want to handle the incoming event, and I want to do it in the library standard way of dragging all handles (unless I've overridden the default behavior).

Do you see pointInnerTool being used for behavior other than dragging all annotation handles? Do you have any practical real world examples of different behavior we might want to use when this is true? Is this behavior that should be a default implementation for innerSelectedCallback on BaseAnnotationTool?

To me, the biggest questions/risks are:

CC: @JamesAPetts

Thanks for the suggestion, @xingbofeng! It's really encouraging to get feedback and feature requests for this part of the library.

xingbofeng commented 5 years ago

Thanks for your replay, and thanks for cornerstoneTools project. I changed it for the reason that doctor using our pulmonary nodule medical image product think drag the nodule (we use EllipticalRoiTool and FreehandMouseTool based on cornerstone and cornerstoneTools) is too hard, so I changed the code of EllipticalRoiTool and FreehandMouse, add pointInnerTool and changed mouseDown.js.

In the opinion of my partner doctors who is in China, because the nodule is too small, dragging the nodule by mousedown at the side of roi is too hard for them.

As you say, pointInnerTool has a higher chance of multiple tools reporting back true, i have some idea now:

Can you give me some advice? Looking forward to your reply.

JamesAPetts commented 5 years ago

Just to play devil's advocate, how would you intend to deal with the case of drawing one region inside the other in this pointInnerTool mode?

e.g.:
giphy

As the passive functionality of the first ROI would grab the event and you would instead start moving the ROI.

dannyrb commented 5 years ago

In the opinion of my partner doctors who is in China, because the nodule is too small, dragging the nodule by mousedown at the side of roi is too hard for them.

For this specific use case, I would recommend pointNearTool having a larger threshold/distance for when it would return true to make the nodules easier to grab/drag. You could even change pointNearTool to conditionally use pointInnerTool logic when the width or height of the shape is small.

If any of the above things are not easy/obvious to do in the current setup, then maybe we could explore ways to make modifications like that more accessible to developers?

pointInnerTool, to me, feels more like a way to initiate a behavior that is different than pointNearTool's. If you had said:

Then I would be more keen to add it as the tool logic changes based on the context/interaction.

@JamesAPetts, your point is a good one. We'd need to be very careful about how/where we implement this as a default as passive tool interaction currently takes priority over active.

xingbofeng commented 5 years ago

Just to play devil's advocate, how would you intend to deal with the case of drawing one region inside the other in this pointInnerTool mode?

e.g.: giphy

As the passive functionality of the first ROI would grab the event and you would instead start moving the ROI.

@JamesAPetts I'm not consider this problem,now i just choose the first annotation of the array.I need to consider it.

Thanks for the suggestion of @dannyrb ,i will talk about this suggestion with our doctors, now i think it's not necessary to add this feature, maybe.Thanks!