Fix selection box positioning and alignment when document is PTZ'd

[krVatsal]

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

[Keavon]

Still confused about the expected outcome for zooming, can you clarify more for what should be the output for it?

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).

[krVatsal]

Still confused about the expected outcome for zooming, can you clarify more for what should be the output for it?

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.

[Keavon]

Why is this an "if" scenario? Why isn't it always the case that the start point is document-anchored?

[krVatsal]

Thanks for pointing out, you are correct it should always be document anchored i have fixed this in next commit.

[Keavon]

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?

[krVatsal]

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).

[Keavon]

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 (#2595). 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_.mp4

Please mark as Ready for Review when you respond so I don't miss this while it's a draft.

[Keavon]

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.

[krVatsal]

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.

[Keavon]

Undisclosed LLM usage is a violation of our acceptable AI usage policy. Consider this your one warning.

[krVatsal]

Sure, this won't happen again.

[Keavon]

Do you have any updates on reverting this to the original state and then taking action on my original comments?

[krVatsal]

Yeah was working on this pr actually my mid semester exams are going on so there was some blockage of time, will update this by EOD.

[krVatsal]

I think i should open a new PR as this one is getting a bit messy for me to manage.

[Keavon]

Please do not, there is no reason to do so.

[cubic-dev-ai]

1 issue found across 4 files

Prompt for AI agents (unresolved issues)

Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.


<file name="editor/src/messages/tool/tool_messages/select_tool.rs">

<violation number="1" location="editor/src/messages/tool/tool_messages/select_tool.rs:448">
P2: Floating-point equality comparison after coordinate-space roundtrip will virtually never be true. `start_viewport` is computed by roundtripping through document space (`viewport→document→viewport`), introducing floating-point drift, while `drag_current` comes directly from mouse input. Use a distance-based threshold comparison instead of exact equality to preserve the click-selection tolerance fallback.</violation>
</file>

---

Since this is your first cubic review, here's how it works:

• cubic automatically reviews your code and comments on bugs and improvements
• Teach cubic by replying to its comments. cubic learns from your replies and gets better over time
• Add one-off context when rerunning by tagging @cubic-dev-ai with guidance or docs links (including llms.txt)
• Ask questions if you need clarification on any suggestion

_{Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.}

[cubic-dev-ai]

1 issue found across 1 file (changes from recent commits).

Prompt for AI agents (unresolved issues)

Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.


<file name="editor/src/messages/tool/tool_messages/select_tool.rs">

<violation number="1" location="editor/src/messages/tool/tool_messages/select_tool.rs:447">
P2: The `roundtrip_epsilon` of `10. * f64::EPSILON` (~2.2e-15) is far too small for comparing viewport coordinates that have been round-tripped through an affine transform. Floating-point error from `M * M⁻¹ * p` scales with coordinate magnitude, so for typical pixel values (hundreds to thousands), the per-component error is ~1e-13, vastly exceeding this threshold. This makes the epsilon check effectively equivalent to the original exact `==` comparison.

Use a small absolute epsilon that's meaningful in pixel space (e.g. a tiny fraction of a pixel) rather than a multiple of machine epsilon.</violation>
</file>

_{Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.}