Brush presets

[dieknolle3333]

Steps to test:

• Go into brush mode when viewing an annotation
• Open the change brushsize popover
• Make sure there are buttons you can click to set the brush size to small, medium and large
• The brush itself should be visible in the center of the active viewport

Issues:

• partly fixes Brush presets #7046:
  -- creates buttons with preset brush sizes
  -- renders brush in the center of the active viewport
• fixes Dataset/Annotation view: disable hover effects while viewport is occluded #5363

---

(Please delete unneeded items, merge only when none are left open)

• Updated changelog
• Removed dev-only changes like prints and application.conf edits
• Considered common edge cases
• Needs datastore update after deployment

[dieknolle3333]

@philippotto Some thoughts and questions.

• I manage to center the brush upon opening the popover. The problem is that it is only shown in the active viewport (or in Plane_XY as a default am seeing that correctly). That also means that at first the brush is centered in YZ, but as the popover is above XY, the plane with the centered brush switches to XY. I find that to be quite confusing. Should I simply remember the active viewport upon opening the popover and display the brush there as long as the popover remains open? So the viewport where the brush is shown could only change after closing and opening the popover.
• The initial problem, that the popover is covering up the brush, can still happen (see picture) after resizing the upper right viewport. Is that just how it is or should I try to take care of that?
  

[philippotto]

@dieknolle3333 I think you found a deeper problem in the current event handling. Namely, that the brush can be moved even when the mouse is an another UI (e.g, your popover which is placed above the viewport). I investigated this a bit and think that the input.ts module should be adapted accordingly. I just a committed the change for this on this PR. Please test whether this solves your problems. If this is the case, it should even be possible to remove the code you added that ignores the "set mouse position" event while the popover is visible (you might want to comment out the relevant condition first and if this works then the rest can be cleaned up; e.g., storing the visible flag in the store is probably not necessary anymore then).

The initial problem, that the popover is covering up the brush, can still happen (see picture) after resizing the upper right viewport. Is that just how it is or should I try to take care of that?

I think, this edge case is okay to ignore for now. You could try to change the placement property of the antd popover to right so that the popover hides a bit less of the viewports. However, I'm not sure whether the popover will stick out of the top window edge and thus only will be partly visible (which would be bad ^^).

[dieknolle3333]

@philippotto
It works, thank you!

[dieknolle3333]

A brush smaller than 10vx is hardly visible for the dataset I used to test (test-agglomerate-file without zooming in), so I thought this makes sense but wasn't sure. See screenshots attached for visibility of brush sizes 5vx and 10vx with said settings

[philippotto]

A brush smaller than 10vx is hardly visible for the dataset I used to test (test-agglomerate-file without zooming in), so I thought this makes sense but wasn't sure.

I'd vote to not add any clamping, because it can be confusing to the user. Especially, when zooming very far in, this would be very unintuitive in my opinion. If the brush size is so small, that one cannot see it, so be it :)

[dieknolle3333]

but would this make sense for the default sizes or should the small preset simply be the smallest size possible?

[philippotto]

I'm not sure if I understood your question correctly, but I'd change the small brush size preset from 1 to 10 vx, since that is more common in my opinion.

[philippotto]

Great! Looks already very good 👍 I have left some feedback, but nothing major :)

[philippotto]

Nice touch with the rounding to multiples of 5 👍

[philippotto]

I'd move that above line 560 since it's used in line 561. The current way works technically, because line 562 is executed later than line 567, but I find it cleaner to switch the order.

[philippotto]

This is a side-effect (since it causes an action dispatch) which should ideally not happen in the normal rendering code. For example, an endless rendering loop could happen if this statement wasn't guarded with a proper if-logic. Currently, it probably works just fine, but it's still cleaner to wrap it with useEffect. The code would look somewhat like this:

  useEffect(() => {
    if (presetBrushSizes.length === 0) {
      handleUpdatePresetBrushSizes(getDefaultBrushSizes());
    }
  }, [presetBrushSizes]); 

  // basically the old code without the handleUpdatePresetBrushSizes:
  let smallBrushSize: number, mediumBrushSize: number, largeBrushSize: number;
  if (presetBrushSizes.length === 0) {
    [smallBrushSize, mediumBrushSize, largeBrushSize] = getDefaultBrushSizes();
  } ...

[philippotto]

I think, I'd go with an object because the semantics are way clearer then :) Also, I don't think, that it will result in significantly more code, so there's not really a downside, I'd hope. I'd go with: presetBrushSizes and {small, medium, large} then (the keys in the object don't need brushSize again as the context makes this clear).

[philippotto]

Looks great, but the keyboard handling code needs to be moved somewhere else :)

[philippotto]

Awesome, let's merge it :)

[dieknolle3333]

I just thought, maybe I should quickly add the new shortcuts to the keyboard shortcut docu? If you agree I'll do that :) @philippotto

[philippotto]

Sure, good catch!

[dieknolle3333]

@philippotto can you have a quick look at what I wrote?

[philippotto]

@philippotto can you have a quick look at what I wrote?

Looks good :)