Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Refactor zoom and selection computations with Box3 #1323

Merged
merged 1 commit into from
Dec 19, 2022
Merged

Refactor zoom and selection computations with Box3 #1323

merged 1 commit into from
Dec 19, 2022

Conversation

axelboc
Copy link
Contributor

@axelboc axelboc commented Dec 14, 2022

This was a difficult but rewarding one. I considered splitting in multiple commits/PR, but the overall reasoning would have been lost in the process. I did revert a few changes as I wrote this, though. So here goes nothing...


At its core, this PR deals with the interaction utilities in lib/src/interactions/utils.ts: getRatioRespectingRectangle, getEnclosedRectangle clampPositionToArea and clampRectangleToVis. Let's first go over them to see what they do (or rather used to do) and why they need to be refactored:

getRatioRespectingRectangle

Takes a rectangle as two vectors and a ratio, and expands the rectangle either vertically or horizontally (and equally on both sides) to match the ratio. It is used in SelectToZoom, when a selection ends, as the first step to computing the area to zoom on. It is also used in RatioSelectionRect (itself used by SelectToZoom), as the first step to displaying the effective zoom selection.

Why refactor it?

  • The name is a bit awkward.
  • The internal logic takes particular care to keep the "orientation" of the input rectangle, but we don't actually care about this: we just need a box to zoom on and draw on screen.
  • We compute the size and center of the rectangle, which Box3 is very god at doing.
  • Box3 provides a method for expanding a box by a vector (x for horizontal expansion, y for vertical expansion).

getEnclosedRectangle

Takes a rectangle as two vectors and returns the width, height and center vector of the rectangle. This is a convenience utility that is used right after both occurrences of getRatioRespectingRectangle (see above) and also in clampRectangleToVis (see below). It's actually surprising we weren't also using it directly in getRatioRespectingRectangle since we also compute the center/size of a rectangle in there...

Why refactor it?

  • This is basically the same as box3.getCenter() and box3.getSize().
  • The code cares about the "orientation" of the input rectangle, a complexity that Box3 can abstract away for us.
  • Computing both the center and size every time might be inefficient if you only need one or the other.

clampPositionToArea

Even though it's quite concise, this interaction utility is actually the most obscure. It basically takes a box (as a center and a size) and an "area" box (only as a size, assuming that the center of the area is at Vector(0, 0, 0)), and shifts the box so it remains within the area box. What it returns is the center of the shifted box.

This is used in:

  • useMoveCameraTo to prevent moving the FOV outside of the bounds of the visualization;
  • clampRectangleToVis (itself used in RatioSelectionRect, see below) to keep the effective zoom rectangle within the bounds of the visualization (note: not within the FOV, see below).

Why refactor it?

  • The name is confusing: the input box is not actually clamped but shifted. It even supports passing a box bigger than the area box, in which case the box ends up centered around the area box (e.g. which can happen with the select-to-zoom interaction, when keepRatio is enabled and the canvas is larger/taller than the visualization => the zoom box spills over the visualization bounds).
  • In useMoveCameraTo, we effectively pass the target FOV we want the camera to have as well as the visualization area we want to keep the FOV in. This works great, but the API does not make this clear at all.
  • In clampRectangleToVis, we pass the box we ideally want to zoom on as well as the visualization area. Again the API isn't clear and it requires calling getEnclosedRectangle first. But there's a bigger problem: the fact that we pass the visualization area means that the zoom box can actually overflow the FOV of the camera. This is a bit tricky to replicate: you need to zoom in the middle of the visualization and then make a selection with such a ratio that the zoom box overflows the FOV; then, you can reduce your selection in one direction to see the zoom box move in and out of the FOV.
  • Box3 has a method to translate a box.

clampRectangleToVis

This is basically just a wrapper around clampPositionToArea to make this function's API more palatable to RatioSelectionRect specifically - i.e. it takes a rectangle as two vectors and calls getEnclosedRectangle to get the center and size of the rectangle that clampPositionToArea requires; it also returns the shifted rectangle instead of just the shifted center so we can easily pass startPoint and endPoint to SelectionRect.

Why refactor it?

  • The name is confusing because the rectangle is not clamped but shifted -- "kept within" the visualization area.
  • As discussed above, the fact that the zoom box sometimes stays within the FOV (thanks to SelectionTool) and sometimes moves out of it is confusing from the user's viewpoint.

The refactoring consists mainly in embracing Three's Box3 class.

At first, I tried using Box3 directly in the utilities above but their APIs (based on parameters like startPoint, endPoint, center, etc.) quickly got in the way and the code quickly became unreadable. Then, I tried making new utilities with APIs that worked better with Box3, but this led to a lot of duplication and inefficiencies (initialising and cloning lots of vectors/boxes).

In the end, and after much consideration, I decided to make my own Box class that extends Box3. I added:

  • a couple of getters for center and size to abstract Box3's more convoluted getCenter and getSize methods;
  • a couple of convenience static methods for creating boxes:
    • Box.empty(), because by default, Three sets the min and max vectors to +Infinity and -Infinity respectively, which makes expanding the box by a set amount challenging... They really like to complicate things...
    • Box.fromPoints(), which is a shortcut for setFromPoints that accepts individual vectors instead of an array (...points: Vector3[] instead of points: Vector3[]);
  • a convenience method, expandBySize for expanding a box by a given width and height from the center (i.e. the expansion is spread equally both vertically and horizontally).
  • two methods that effectively replace and improve upon getRatioRespectingRectangle and clampRectangleToVis:
    • expandToRatio, to expand a box to a given ratio (either horizontally or vertically but always equally on both sides so the center of the box doesn't change);
    • keepWithin, to keep a box within another "area" box (by shifting it if needed with translate) -- as per clampPositionToArea's original behaviour, if the area box is smaller than the source box, the source box becomes centered around the area box.

As per other Box3 methods and in line with Three's coding style, the new Box methods mutate the instance and are chainable. As you'll see, this is quite convenient and makes the code very readable.


A second important aspect of the refactoring concerns moveCameraTo. As I was refactoring things with Box3, I realised two things:

  1. moveCameraTo doesn't care about the position of the FOV; it only cares about its size;
  2. there's a much simpler way to get the size of the FOV that doesn't require unprojecting the bottomLeft and topRight corners with getWorldFOV: we can simply multiply the camera's scale by canvasSize!

These realisations allowed me to simplify/clarify the code a lot by means of the new Box class. Since the new solution doesn't make use of the camera's projection matrix, it also means we no longer have to call camera.updateMatrixWorld() when we update the scale before calling moveCameraTo, which removes a hidden coupling and is a big win in my book!


Hopefully this gives you enough context to understand the diff and the comments within, where I explain the implementation details. 😅

packages/lib/src/interactions/hooks.ts Outdated Show resolved Hide resolved
packages/lib/src/interactions/hooks.ts Outdated Show resolved Hide resolved
packages/lib/src/interactions/hooks.ts Show resolved Hide resolved
packages/lib/src/interactions/hooks.ts Show resolved Hide resolved
packages/lib/src/vis/shared/RatioEnforcer.tsx Show resolved Hide resolved
@axelboc axelboc requested a review from loichuder December 14, 2022 12:58
Copy link
Member

@loichuder loichuder left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Some nitpicks here and there but overall, I think this improves greatly the code.

Well done 🥇

packages/lib/src/interactions/box.ts Outdated Show resolved Hide resolved
packages/lib/src/interactions/box.ts Outdated Show resolved Hide resolved
packages/lib/src/interactions/box.ts Show resolved Hide resolved
packages/lib/src/interactions/hooks.ts Outdated Show resolved Hide resolved
const shift = centerClampingBox
.clampPoint(center, new Vector3())
.sub(center)
.setZ(0); // cancel `z` shift
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I can live with that

packages/lib/src/interactions/hooks.ts Outdated Show resolved Hide resolved
@axelboc axelboc force-pushed the box branch 4 times, most recently from 7dc8802 to 955fe45 Compare December 19, 2022 13:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants