fix: sub-pixel and off-by-one errors in bounded_move_shapes clamping#242
Merged
Conversation
Copilot's review of #241 flagged that the left/top clamp still did int(o1.x()) before wrapping in QPointF, so sub-pixel overflows (e.g. -0.3) truncate to 0 and silently fail to correct the position. Verifying that fix surfaced a second, independent bug in the same lines: the right/bottom clamp corrects against pixmap.width()/height() instead of (width() - 1)/(height() - 1), which is the bound out_off_pixmap() actually enforces — so a shape overflowing the right or bottom edge stayed uncorrected regardless of the int() truncation. Both are pre-existing (predate #237/#238/#240/#241); this PR only touches the four lines already modified by #241. Adds a regression test covering both edges plus the original TypeError crash path.
There was a problem hiding this comment.
Pull request overview
This PR fixes Canvas.bounded_move_shapes() clamping math so sub-pixel overflows are actually corrected (no int() truncation) and right/bottom clamping matches out_off_pixmap()’s inclusive (w - 1, h - 1) bounds, preventing off-by-one “no-op” corrections at the far edge.
Changes:
- Update left/top clamp to use floating-point deltas (no truncation), preserving sub-pixel corrections.
- Update right/bottom clamp to correct against
(width - 1)/(height - 1)to matchout_off_pixmap()’s boundary logic. - Add regression tests covering the prior TypeError path and both sub-pixel left/top and right/bottom overflow corrections.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
anylabeling/views/labeling/widgets/canvas.py |
Fixes the clamp deltas in bounded_move_shapes() to be sub-pixel-safe and consistent with out_off_pixmap()’s bounds. |
tests/test_canvas_bounded_move.py |
Adds regression tests validating the corrected clamping behavior (including sub-pixel cases) and the previously reported TypeError scenario. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
3 tasks
This file contains hidden or 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
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.
Summary
Follow-up to #241. Copilot's review on that PR flagged that the left/top clamp still did
int(o1.x())before wrapping inQPointF, so sub-pixel overflows (e.g.-0.3) truncate to0and the correction silently no-ops.Writing an output-correctness test for that (not just a crash check) surfaced a second, independent bug in the same lines: the right/bottom clamp corrects against
pixmap.width()/height()instead of(width() - 1)/(height() - 1)— which is the boundout_off_pixmap()itself enforces. So a shape overflowing the right or bottom edge stayed uncorrected regardless of theint()issue.Both are pre-existing (predate #237/#238/#240/#241) — this only touches the four lines #241 already modified.
Fix
int()truncation so sub-pixel corrections aren't dropped.width() - 1/height() - 1, matchingout_off_pixmap()'s own bound.Test plan
tests/test_canvas_bounded_move.py: sub-pixel overflow at left/top edge, sub-pixel overflow at right/bottom edge (asserts exact corrected value), original TypeError crash path.CLAUDE.md(unrelated), no new failures.