Skip to content

Snap overlays to device instead of UI pixels on web - #4281

Merged
timon-schelling merged 1 commit into
masterfrom
fix-overlay-snapping-web
Jun 25, 2026
Merged

timon-schelling merged 1 commit into
masterfrom
fix-overlay-snapping-web

Conversation

@timon-schelling

Copy link
Copy Markdown
Member

Web version of #3493

image image

@timon-schelling
timon-schelling requested a review from Keavon June 24, 2026 23:52

@gemini-code-assist gemini-code-assist Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Code Review

This pull request replaces hardcoded coordinate rounding and half-pixel offsets with dynamic snapping helper methods (snap_to_physical_pixel and snap_to_physical_pixel_center) that scale with the viewport. A review comment points out an inconsistency in push_path where the starting point is snapped to the physical pixel boundary, while subsequent points are snapped to the physical pixel center, which could cause visual misalignment.

Important

The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.

};

let start_point = transform.transform_point2(point_to_dvec2(first.start()));
let start_point = self.snap_to_physical_pixel(start_point);

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

medium

In push_path, the starting point of the path (start_point) is snapped to the physical pixel boundary using snap_to_physical_pixel, whereas all subsequent points in the path (lines, quadratic curves, cubic curves) are snapped to the physical pixel center using snap_to_physical_pixel_center.

This inconsistency causes the first segment of the path to be misaligned or skewed by half a physical pixel relative to the rest of the path, which is especially noticeable when the path is stroked or closed. Snapping the start point to the physical pixel center ensures visual consistency across the entire path.

Suggested change
let start_point = self.snap_to_physical_pixel(start_point);
let start_point = self.snap_to_physical_pixel_center(start_point);

@cubic-dev-ai cubic-dev-ai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

1 issue found across 1 file

Confidence score: 3/5

  • In editor/src/messages/portfolio/document/overlays/utility_types_web.rs, the path uses mixed snapping methods (snap_to_physical_pixel for start_point vs snap_to_physical_pixel_c... for later points), which can create subtle geometry discontinuities or visible jitter at segment joins in rendered overlays; align all path points to the same snapping strategy and verify with a quick visual regression check before merging.
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/portfolio/document/overlays/utility_types_web.rs">

<violation number="1" location="editor/src/messages/portfolio/document/overlays/utility_types_web.rs:946">
P2: Inconsistent snapping: `start_point` is snapped with `snap_to_physical_pixel` while all subsequent points in the path (line endpoints, quadratic/cubic control and end points) are snapped with `snap_to_physical_pixel_center`. This causes the first segment to be offset by half a physical pixel relative to the rest of the path. Other methods like `draw_line` and `draw_polygon` consistently use `snap_to_physical_pixel_center` for all points including `move_to` — this should do the same.</violation>
</file>

Reply with feedback, questions, or to request a fix.

Re-trigger cubic

};

let start_point = transform.transform_point2(point_to_dvec2(first.start()));
let start_point = self.snap_to_physical_pixel(start_point);

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

P2: Inconsistent snapping: start_point is snapped with snap_to_physical_pixel while all subsequent points in the path (line endpoints, quadratic/cubic control and end points) are snapped with snap_to_physical_pixel_center. This causes the first segment to be offset by half a physical pixel relative to the rest of the path. Other methods like draw_line and draw_polygon consistently use snap_to_physical_pixel_center for all points including move_to — this should do the same.

Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At editor/src/messages/portfolio/document/overlays/utility_types_web.rs, line 946:

<comment>Inconsistent snapping: `start_point` is snapped with `snap_to_physical_pixel` while all subsequent points in the path (line endpoints, quadratic/cubic control and end points) are snapped with `snap_to_physical_pixel_center`. This causes the first segment to be offset by half a physical pixel relative to the rest of the path. Other methods like `draw_line` and `draw_polygon` consistently use `snap_to_physical_pixel_center` for all points including `move_to` — this should do the same.</comment>

<file context>
@@ -941,29 +943,30 @@ impl OverlayContext {
 			};
 
 			let start_point = transform.transform_point2(point_to_dvec2(first.start()));
+			let start_point = self.snap_to_physical_pixel(start_point);
 			self.render_context.move_to(start_point.x, start_point.y);
 
</file context>
Suggested change
let start_point = self.snap_to_physical_pixel(start_point);
let start_point = self.snap_to_physical_pixel_center(start_point);

@timon-schelling
timon-schelling added this pull request to the merge queue Jun 25, 2026
@Keavon
Keavon removed this pull request from the merge queue due to a manual request Jun 25, 2026
@Keavon Keavon changed the title Snap overlays to physical instead of logical pixels on web Snap overlays to device instead of CSS pixels on web Jun 25, 2026
@timon-schelling timon-schelling changed the title Snap overlays to device instead of CSS pixels on web Snap overlays to device instead of ui pixels on web Jun 25, 2026
@timon-schelling timon-schelling changed the title Snap overlays to device instead of ui pixels on web Snap overlays to device instead of UI pixels on web Jun 25, 2026
@timon-schelling
timon-schelling added this pull request to the merge queue Jun 25, 2026
Merged via the queue into master with commit f3faefd Jun 25, 2026
10 checks passed
@timon-schelling
timon-schelling deleted the fix-overlay-snapping-web branch June 25, 2026 09:08
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