-
Notifications
You must be signed in to change notification settings - Fork 19
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
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
axelboc
commented
Dec 14, 2022
loichuder
approved these changes
Dec 19, 2022
There was a problem hiding this 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 🥇
const shift = centerClampingBox | ||
.clampPoint(center, new Vector3()) | ||
.sub(center) | ||
.setZ(0); // cancel `z` shift |
There was a problem hiding this comment.
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
axelboc
force-pushed
the
box
branch
4 times, most recently
from
December 19, 2022 13:59
7dc8802
to
955fe45
Compare
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
andclampRectangleToVis
. 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 inRatioSelectionRect
(itself used bySelectToZoom
), as the first step to displaying the effective zoom selection.Why refactor it?
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
andcenter
vector of the rectangle. This is a convenience utility that is used right after both occurrences ofgetRatioRespectingRectangle
(see above) and also inclampRectangleToVis
(see below). It's actually surprising we weren't also using it directly ingetRatioRespectingRectangle
since we also compute the center/size of a rectangle in there...Why refactor it?
box3.getCenter()
andbox3.getSize()
.Box3
can abstract away for us.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 inRatioSelectionRect
, see below) to keep the effective zoom rectangle within the bounds of the visualization (note: not within the FOV, see below).Why refactor it?
keepRatio
is enabled and the canvas is larger/taller than the visualization => the zoom box spills over the visualization bounds).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.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 callinggetEnclosedRectangle
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 toRatioSelectionRect
specifically - i.e. it takes a rectangle as two vectors and callsgetEnclosedRectangle
to get the center and size of the rectangle thatclampPositionToArea
requires; it also returns the shifted rectangle instead of just the shifted center so we can easily passstartPoint
andendPoint
toSelectionRect
.Why refactor it?
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 likestartPoint
,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 withBox3
, 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 extendsBox3
. I added:center
andsize
to abstractBox3
's more convolutedgetCenter
andgetSize
methods;Box.empty()
, because by default, Three sets themin
andmax
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 forsetFromPoints
that accepts individual vectors instead of an array (...points: Vector3[]
instead ofpoints: Vector3[]
);expandBySize
for expanding a box by a givenwidth
andheight
from the center (i.e. the expansion is spread equally both vertically and horizontally).getRatioRespectingRectangle
andclampRectangleToVis
: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 withtranslate
) -- as perclampPositionToArea
'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 newBox
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 withBox3
, I realised two things:moveCameraTo
doesn't care about the position of the FOV; it only cares about its size;bottomLeft
andtopRight
corners withgetWorldFOV
: we can simply multiply the camera'sscale
bycanvasSize
!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 callcamera.updateMatrixWorld()
when we update thescale
before callingmoveCameraTo
, 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. 😅