Fix selection box positioning and alignment when document is PTZ'd#3718
Conversation
In all cases, the document-space start point and document-space current pointer position should determine the bounding box shown, as well as the current document tilt. You can always take those three parameters (start point, current point, current tilt) and draw a rectangle matching the document space coordinates of them (but transformed into viewport space for the final drawing of the rectangle, since overlays are in viewport space). |
Thanks for the clarification! I think the current implementation already handles this correctly for both panning and zooming. |
…ctive-box-selection
| pub fn selection_box(&self) -> [DVec2; 2] { | ||
| if self.drag_current == self.drag_start { | ||
| pub fn selection_box(&self, metadata: &DocumentMetadata) -> [DVec2; 2] { | ||
| // If we have a document-anchored start point, transform it to viewport |
There was a problem hiding this comment.
Why is this an "if" scenario? Why isn't it always the case that the start point is document-anchored?
There was a problem hiding this comment.
Thanks for pointing out, you are correct it should always be document anchored i have fixed this in next commit.
| snap_candidates: Vec<SnapCandidatePoint>, | ||
| auto_panning: AutoPanning, | ||
| drag_start_center: ViewportPosition, | ||
| drag_start_document: Option<DVec2>, |
There was a problem hiding this comment.
See other comment. I'm possibly a bit concerned that this is an Option. When would it be None? Can you make sure that this is built (if it's not already) in such a way that it makes invalid state unrepresentable?
There was a problem hiding this comment.
because the tool is not always in a dragging state (e.g., when it is [Ready]). However, the variants themselves (like [Dragging], [Drawing], [ResizingBounds], etc.) now store [drag_start] as a mandatory, non-optional field(in the new commit i am pushing in some time).
|
Select tool behavior is good from my QA test. Just see my concern from my code review comments. It might represent an opportunity to simplify the code if there is some kind of extra conditionality going on right now. The equivalent implementation is missing from the Path tool. There is also one small bug I've noticed that this implementation seems to consistent trigger, which is that the box selection (upon initially clicking down but not yet dragging) draws a tiny square. I've observed this in a few other occasions but now your branch appears to be making this happen all the time, which is good since that should make it easier to debug and fix the root cause. capture_12_.mp4Please mark as Ready for Review when you respond so I don't miss this while it's a draft. |
…ection' of https://github.com/krVatsal/Graphite into fix-cooridnate-system-mismatch-during-an-active-box-selection
…ctive-box-selection
…ctive-box-selection
|
Can you please help me understand how you went from +29-18 lines of changes before my code review that requested a potential simplification, to a staggering +196-89 after the new commit? It is too many changes for me to try to read and understand, but this doesn't make sense to me at all and it makes me think it was either written by an LLM or— actually I don't have any other theories that could explain how my change requests resulted in 6x the changes from before then. And this doesn't even include the requested Path tool changes, which would be the only thing I requested that I would expect a ~doubling (not 6x'ing) of the code. But before you apply this to the Path tool, let's make sure we first figure out what the problem is here that I'm pointing out. |
Yeah, i was a bit blank about how should i simplify this further, that's why i took help of llm apologies for that. I'll look into this again and update with a much cleaner version. Thankyou for your patience. |
|
Undisclosed LLM usage is a violation of our acceptable AI usage policy. Consider this your one warning. |
Fixes #2391
When performing a box selection with the Select tool, panning (via scroll wheel) would cause the selection rectangle's starting point to move with the viewport instead of staying fixed on the canvas.
Solution
Store the drag start position in document coordinates (drag_start_document) rather than viewport coordinates
Transform this document anchored point back to viewport space when computing the selection box
Remove the viewport shift compensation for the start point during auto panning since the start is now properly anchored to document space
Still confused about the expected outcome for zooming, can you clarify more for what should be the output for it?
Recording.2026-02-04.235848.mp4