Add bounding box tool

[MichaelBuessemeyer]

URL of deployed dev instance (used for testing):

• https://addboundingboxtool.webknossos.xyz

Steps to test:

• Open a tracing
• Use the context menu. There should be an option for creating a bounding box. Each created bounding box should have bounds according to what the user can see. If the bounding box is created using the context menu, its center will be shifted to the point where the context menu was opened.
• Then select the bounding box tool. Each visible edge in the plane viewports should be resizable. If the cursor is near a corner, the edges of both corners should be resized. While resizing the bounds of the bounding box the edges snap automatically to full integer positions.
• When using the bounding box tool the "create cell" button should change to a "create bounding box" button that creates a bounding box in the center of the plane viewports.
• When the bounding box tool is active and the context menu is opened on a bounding box, then the menu should have additional menu items that allow changing the color, hiding and renaming.
• The bounding box tab should still work as it did before. The onliest difference is, that a newly created bounding box via the plus button at the bottom of the bounding box list is created with the same bounds as it would be done with the "create bounding box" button in the toolbar.
• Additionally, there is another button for each bounding box in the list, that upon clicking changes the flycam's position to the center of the bounding box.
• The undo/redo feature of wk should now support resizing, color-changing, renaming, and so on of bounding boxes.
• Test what happens when the arbitrary mode is opened. The bounding box tool should automatically deactivate.
• Test what happens when the dataset only has two dimensions. Are the bounds of the created bounding boxes correct?

Issues:

• fixes Add a tool for drawing/editing bounding boxes #5185, 5784

Note:

• I am still trying to fix the tests

---

• Updated (unreleased) changelog
• Updated (unreleased) migration guide if applicable
• Updated documentation if applicable
• Adapted wk-connect if datastore API changes
• Needs datastore update after deployment
• Ready for review

[MichaelBuessemeyer]

TODOs:

• The BBox might not resize when zoomed far in, because then the delta movement might be lower than 0.5 voxel. Use the mouse position of the event and calculate the global pos from there to get the correct values for the bbox.
• Check what happens when the minimum edge crosses the maximum edge and therefore becomes the new maximum edge itself.
• Add the context menu to the bbox tool and add an always active item (if in view mode or changing the annotation is allowed) to create a new bbox in the center of the viewports.
• Make separate actions for manipulating the bounding boxes / a single bounding box.
• The cursor doesn't change anymore if bbox resizing is possible.
• The dataset scale seems to affect the bbox hit range.
• The button "create Cell" should become a create bounding box button when the bbox tool is active.
• Each added bounding box should use the same function and should be centered just like the left click of the box tool.
• Left click for bbox creation might be a little too extreme. Use the shortcut c instead. (Like creating a new cell)
• Bboxes created with the context should have the clicked position as a center.
• alt+mouse move should move the viewports around.
• Somehow support arbitrary view modes. e.g. just disable the whole tool. Adding a box from the bbox tab needs to be disabled or should work for arbitrary views.
• 🐛 Only adding a new bounding box is not saved in backend. The bounding box needs to be resized.
• Jump to bbox if the bbox is clicked in the bbox tab
• Rename bbox naming schema from user bounding box x to bounding box x.

Maybe in this PR / maybe in a follow-up pr:

• Add Corner dragging
• Add context menu functionality when a bbox is hit (delete, change color, rename, hide, etc..)

Testing TODOs:

• Test this in arbitrary mode
• Test whether 2d datasets work as expected: The third dimension should have an extent of 1.
• Consider making a "bbox layer" in the layers tab to make all boxes visible/invisible.

[philippotto]

Great stuff :) Here's my first round of feedback. Didn't review all files yet, but will do the rest tomorrow!

[philippotto]

Wow, really impressive how you poured the complexity into code and even added usability perks, such as resizing via corners and undo/redo 👍 I left a couple of comments on which we should do at least one iteration pass, I think. Also, I'm looking forward to testing, but the CI fails. Let me know in case you need some help with that 🤙

[philippotto]

I don't understand why this is computing the distance to the corner (even though the method is named getDistanceToBoundingBoxEdge). Wouldn't this mean that if I try to resize the top border of a bbox like this:

                                          mouse grabs here
                                                  x
--------------------------------------------------------------------------------------------------
|                                                                                                |
|                                                                                                |

a very long distance to a corner would be returned (because the bbox is very wide) so that I cannot really grab the handle, even though I'm only one pixel away from the border?

[MichaelBuessemeyer]

The first case computes the distance to the left top corner in your example.
But this way of calculating the distance is only chosen when mouse.x is smaller than the min.x of the bounding box.
See pos[edgeDim] < min[edgeDim] in the if at line 63.

Here is a matching when which case kicks in and the vector / distance that is calculated;

             min.x                                            max.x
               ↓                                               ↓
               ↓                                               ↓
     case 1    ↓           case 3      case 3                  ↓   case 2
           '.                |           |                        .'
             '↘              ↓           ↓                      ↙'
               -------------------------------------------------
               |                                               |
               |                                               |

Is a little bit better understandable?

[philippotto]

Ahhhh, now I got it :) So, in my example, edgeDim would need to be 0 because it's referring to the case where the horizontal edge is checked. And consequentially, it would compare the x values (and not the y values which I somehow assumed would also be a valid case).

Could you please adapt the above comment to explain how to interpret edgeDim? I.e., 1 means it's a vertical edge. Now, I also understand the "This example is for the xy viewport for y as the main direction / edgeDim." comment which I apparently didn't really grok before.

[MichaelBuessemeyer]

Ahhhh, now I got it :) So, in my example, edgeDim would need to be 0 because it's referring to the case where the horizontal edge is checked. And consequentially, it would compare the x values (and not the y values which I somehow assumed would also be a valid case).

Exactly 🐈

Could you please adapt the above comment to explain how to interpret edgeDim? I.e., 1 means it's a vertical edge.

Not exactly, edgeDim gives the dimension index that the edge extents to: 0 <=> edge direction is x (like above), 1 <=> edge direction is y (like above but downwards), 2 <=> edge direction is z.

I changed the comment highlighted above in the code. Could you please check whether this is now understandable? If not, could you please try to help me explain this logic? I think the explanation is not straightforward.

[philippotto]

awesome :) I'll test tomorrow!

[MichaelBuessemeyer]

TODOS:

• The hovering mechanism is not dataset scale independent but it should be.
  • Is hopefully fixed now 🙈
• Test whether the CSS border needs to be added to the global position calculation

[philippotto]

Testing went great mostly! I only noticed:

• when using "hide bbox" via context menu, the menu doesn't close automatically
• the tooltip for the bbox tool doesn't match the wording of the other tools (full stop should be removed and a long-dash with an explanation should be there)

And also a thing which is not blocking, though:

• renaming via context menu sometimes loses the focus of the input field? sometimes, I hit the "delete" key and afterwards I cannot type anymore? hard to reproduce, though. not super high-pri. one workaround could be to only update the name on blur.

[MichaelBuessemeyer]

• renaming via context menu sometimes loses the focus of the input field? sometimes, I hit the "delete" key and afterward I cannot type anymore? hard to reproduce, though. not super high-pri. one workaround could be to only update the name on blur.

Yeah, I also had the issue while testing. Only updating on enter or blur fixes the issue. 👍

• when using "hide bbox" via context menu, the menu doesn't close automatically

That was easy to fix. I accidentally remove the line that closed the context menu. But the bounding box color selection item is problematic, as color inputs only have an on change and on blur. The problem with the blur is, that is triggered once a color is selected for the first time and not when the color input of the browser is closed. Therefore closing the context menu on blur leads to wrong behavior.
As I could not come up with a solution, I suggest keeping it as it is. Thus the user has to explicitly close the context menu after selecting a new color.

And also important:

During testing I noticed a regression of a bug that the cursor might be fixed at looking like something is resizable when changing form the bbox tool to another tool. Therefore the deselection mechanism added by this pr wasn't working properly, but it had before.
The reason was that the saga responsible for triggering the deselection relied on the fact that the currently active tool was the previous annotation tool. But as the reducer is first executed and then the saga, the active tool was already updated. Therefore each annotation tool changing action must also pass the annotation tool that is about to be deselected to the action. That makes it easy to get the "deselection-saga" working.

As this was a "regression" bug in this pr, I added two tests to avoid a regression later on. Note that I had to update the testing lib "sinon" to get a function needed for the tests.

[philippotto]

The reason was that the saga responsible for triggering the deselection relied on the fact that the currently active tool was the previous annotation tool. But as the reducer is first executed and then the saga, the active tool was already updated. Therefore each annotation tool changing action must also pass the annotation tool that is about to be deselected to the action. That makes it easy to get the "deselection-saga" working.

Another way of solving this could be to remember the "previous tool" in the saga (also see the collectUndoStates as an example). Something like this:

let previousTool = select(...)

while (true) {
  const action = yield* take("CHANGE_ACTIVE_TOOL");
  deselect(previousTool)
  previousTool = action.tool
}

I like this solution better, because it doesn't require adding more information into the existing action. Do you think, this can be changed?

That was easy to fix. I accidentally remove the line that closed the context menu. But the bounding box color selection item is problematic, as color inputs only have an on change and on blur. The problem with the blur is, that is triggered once a color is selected for the first time and not when the color input of the browser is closed. Therefore closing the context menu on blur leads to wrong behavior.
As I could not come up with a solution, I suggest keeping it as it is. Thus the user has to explicitly close the context menu after selecting a new color.

Cool! This means that the context-menu always closes automatically except when changing the color, right?

[philippotto]

As this was a "regression" bug in this pr, I added two tests to avoid a regression later on. Note that I had to update the testing lib "sinon" to get a function needed for the tests.

Great 🐝 🐝 🐝

[MichaelBuessemeyer]

I like this solution better, because it doesn't require adding more information into the existing action. Do you think, this can be changed?

Yeah, your suggestion is much better than my current solution. I just coded in your solution and it works purrfect 🐱

Cool! This means that the context-menu always closes automatically except when changing the color, right?

Indeed 👍

@philippotto Could you please do a final testing? I think this pr should be finally ready to go

[philippotto]

Awesome work 🥇 Feel free to merge :)