feat: add explicit left/right orientation with swap button
Fixes mirrored slice exports by making orientation explicit: - PhotoSlice.swapped flag persists a user left/right swap - swap_left_right() mirrors [TL,TR,BR,BL] -> [TR,TL,BL,BR] - normalize_top_left() honors the flag so squarize/reset_rotation no longer overwrite manual orientation - Swap L/R toolbar button wired through canvas state + controller - Color-code left (orange) vs right (cyan) corners and edges
This commit is contained in:
parent
047ea60fa5
commit
607f5e0c57
7 changed files with 114 additions and 31 deletions
77
AGENTS.md
77
AGENTS.md
|
|
@ -18,8 +18,8 @@ credit and the "inspired by PhotoSlicer" note.
|
||||||
|
|
||||||
## Goals of this fork
|
## Goals of this fork
|
||||||
|
|
||||||
1. Fix a mirrored-export orientation bug and add an explicit left/right
|
1. Fix the mirrored-export orientation bug and add an explicit left/right
|
||||||
orientation workflow (see below). This is the primary patch.
|
orientation workflow (implemented, see below). This is the primary patch.
|
||||||
2. Keep the app usable on the maintainer's own photo scans; afterwards only
|
2. Keep the app usable on the maintainer's own photo scans; afterwards only
|
||||||
minimal maintenance is expected.
|
minimal maintenance is expected.
|
||||||
3. Push to the maintainer's Forgejo instance at `code.dgerdesmann.de` (no
|
3. Push to the maintainer's Forgejo instance at `code.dgerdesmann.de` (no
|
||||||
|
|
@ -29,40 +29,43 @@ The maintainer is primarily an R user and new to Python/Git. Changes should be
|
||||||
small, self-checked, clean, and secure; do not assume the maintainer can vet
|
small, self-checked, clean, and secure; do not assume the maintainer can vet
|
||||||
Python code.
|
Python code.
|
||||||
|
|
||||||
## The orientation bug (primary work item)
|
## The orientation fix (implemented)
|
||||||
|
|
||||||
- Exported slices can come out mirrored (left/right swapped).
|
The bug: exported slices could come out mirrored (left/right swapped). Root
|
||||||
- Manual workaround: drag-switch the two corners of the top edge so the red
|
cause is `PhotoSlice.normalize_top_left()` (in `slices.py`), which re-derives
|
||||||
edge (edge 0) becomes the true top.
|
"top" from geometry — it picks a top-left corner and resolves winding so edge
|
||||||
- Problem: `squarize()` and `reset_rotation()` re-run `normalize_top_left()`,
|
0→1 is the "most horizontal" top edge. That inference is ambiguous, and because
|
||||||
which re-derives "top" from geometry and silently overwrites the manual fix.
|
`squarize()` and `reset_rotation()` both re-run `normalize_top_left()` at the
|
||||||
|
end, any manual correction was silently overwritten.
|
||||||
|
|
||||||
Desired outcome:
|
The fix adds an explicit left/right orientation workflow:
|
||||||
|
|
||||||
1. A visual indication of left vs. right edges/corners (in addition to the
|
- `PhotoSlice.swapped` — persistent flag (default `False`): "user has explicitly
|
||||||
existing red top edge).
|
mirrored left/right".
|
||||||
2. A "swap left/right" button for the active slice.
|
- `PhotoSlice.swap_left_right()` — toggles `swapped` and reindexes the corners
|
||||||
3. The corrected orientation must persist through `squarize()` /
|
`[TL,TR,BR,BL] -> [TR,TL,BL,BR]` (i.e. `bbox[[1,0,3,2]]`), a pure left/right
|
||||||
`reset_rotation()`.
|
mirror that keeps the top (edge 0→1) and bottom edges in place. NOTE: the
|
||||||
|
earlier `[0,3,2,1]` reindex was wrong — it rotated the quad 90°.
|
||||||
|
- `normalize_top_left()` applies the same `[1,0,3,2]` reindex when `swapped` is
|
||||||
|
true (after its geometric inference), so manual orientation survives
|
||||||
|
`squarize()` / `reset_rotation()`.
|
||||||
|
- "Swap L/R" toolbar button (`ui_builders_preview.py`) ->
|
||||||
|
`CanvasStateMixin.swap_active_slice_left_right()` ->
|
||||||
|
`ScanSlicerCanvasActionsMixin.swap_active_box_left_right()`.
|
||||||
|
- Visual indication in `CanvasRenderMixin.paintEvent()` (`canvas_render.py`):
|
||||||
|
corners color-coded left = orange, right = cyan; edges colored red = top
|
||||||
|
(0→1), green = right (1→2), orange = bottom (2→3), and the left edge (3→0)
|
||||||
|
follows lock state (blue locked / lightgreen unlocked).
|
||||||
|
|
||||||
## Key implementation facts
|
## Key implementation facts
|
||||||
|
|
||||||
- `PhotoSlice.bbox` is a 4×2 numpy array in the order
|
- `PhotoSlice.bbox` is a 4×2 numpy array in the order
|
||||||
`[top-left, top-right, bottom-right, bottom-left]`.
|
`[top-left, top-right, bottom-right, bottom-left]`.
|
||||||
- `PhotoSlice.normalize_top_left()` (in `slices.py`) picks a top-left corner and
|
|
||||||
resolves winding so edge 0→1 is the "most horizontal top" edge. This is the
|
|
||||||
source of the ambiguity.
|
|
||||||
- `PhotoSlice.squarize()` and `reset_rotation()` both call
|
|
||||||
`normalize_top_left()` at the end, discarding any manual orientation.
|
|
||||||
- `Autoslicer.save_slice()` (in `autoslicer.py`) builds the perspective
|
- `Autoslicer.save_slice()` (in `autoslicer.py`) builds the perspective
|
||||||
transform assuming the `[TL,TR,BR,BL]` order; mirroring arises upstream of
|
transform assuming the `[TL,TR,BR,BL]` order. Do NOT change that contract
|
||||||
this from a wrongly "normalized" quad.
|
without updating every consumer.
|
||||||
- Edge 0 is drawn red in `CanvasRenderMixin.paintEvent()` (`canvas_render.py`).
|
- `PhotoSlice.squarize()` and `reset_rotation()` call `normalize_top_left()` at
|
||||||
|
the end; the `swapped` flag makes that safe for manual orientation.
|
||||||
A persistent orientation flag on `PhotoSlice` (respected by
|
|
||||||
`normalize_top_left`/`squarize`/`reset_rotation`) plus a swap action is the
|
|
||||||
likely shape of the fix. Do not change the `[TL,TR,BR,BL]` contract of
|
|
||||||
`save_slice` without updating every consumer.
|
|
||||||
|
|
||||||
## Repository structure
|
## Repository structure
|
||||||
|
|
||||||
|
|
@ -106,10 +109,28 @@ uv run scanslicer /path/to/scans # or: python -m scanslicer
|
||||||
```
|
```
|
||||||
|
|
||||||
- AppImage (preferred): `bash packaging/appimage/build-appimage.sh`
|
- AppImage (preferred): `bash packaging/appimage/build-appimage.sh`
|
||||||
- Docker: `docker compose up --build`
|
- AppImage via Docker: `bash packaging/appimage/build-appimage-docker.sh`
|
||||||
|
- Docker runtime: `docker compose up --build`
|
||||||
- Dependencies (from `pyproject.toml`): PySide6, opencv-python, numpy, Pillow.
|
- Dependencies (from `pyproject.toml`): PySide6, opencv-python, numpy, Pillow.
|
||||||
- Python >= 3.10; dev env is Python 3.12 in `.venv`.
|
- Python >= 3.10; dev env is Python 3.12 in `.venv`.
|
||||||
|
|
||||||
|
### AppImage build: Docker context gotcha
|
||||||
|
|
||||||
|
The maintainer's Docker points at Docker Desktop (`desktop-linux` context). Its
|
||||||
|
bind-mount filesystem forbids `chown`, but `appimage-builder` needs root
|
||||||
|
ownership while extracting `.deb` packages, so the build fails with
|
||||||
|
`dpkg-deb ... Cannot change ownership to uid 0, gid 0: Operation not permitted`.
|
||||||
|
|
||||||
|
Workaround: run the Dockerized build against the native daemon (or switch
|
||||||
|
context with `docker context use default`):
|
||||||
|
|
||||||
|
```bash
|
||||||
|
DOCKER_HOST=unix:///var/run/docker.sock bash packaging/appimage/build-appimage-docker.sh
|
||||||
|
```
|
||||||
|
|
||||||
|
Output lands in `appimage/dist/ScanSlicer-<version>-x86_64.AppImage` (the
|
||||||
|
`appimage/` dir is git-ignored).
|
||||||
|
|
||||||
## Config / persistence
|
## Config / persistence
|
||||||
|
|
||||||
Runtime files live in `~/.config/scanslicer/` (or `$XDG_CONFIG_HOME/scanslicer/`)
|
Runtime files live in `~/.config/scanslicer/` (or `$XDG_CONFIG_HOME/scanslicer/`)
|
||||||
|
|
|
||||||
|
|
@ -28,14 +28,22 @@ class CanvasRenderMixin:
|
||||||
b = points[(i + 1) % 4]
|
b = points[(i + 1) % 4]
|
||||||
if i == 0:
|
if i == 0:
|
||||||
color = QColor("red")
|
color = QColor("red")
|
||||||
|
elif i == 1:
|
||||||
|
color = QColor("green")
|
||||||
|
elif i == 2:
|
||||||
|
color = QColor("orange")
|
||||||
else:
|
else:
|
||||||
color = QColor("blue") if s.locked else QColor("lightgreen")
|
color = QColor("blue") if s.locked else QColor("lightgreen")
|
||||||
painter.setPen(QPen(color, 3))
|
painter.setPen(QPen(color, 3))
|
||||||
painter.drawLine(a, b)
|
painter.drawLine(a, b)
|
||||||
|
|
||||||
|
# Corners are color-coded: left = orange, right = cyan.
|
||||||
|
# Edge 0 (top, red) runs TL->TR, so corners 0 and 3 are the left
|
||||||
|
# side and corners 1 and 2 are the right side.
|
||||||
|
corner_colors = (QColor("orange"), QColor("cyan"), QColor("cyan"), QColor("orange"))
|
||||||
|
for p, color in zip(points, corner_colors):
|
||||||
painter.setPen(QPen(QColor("blue"), 2))
|
painter.setPen(QPen(QColor("blue"), 2))
|
||||||
painter.setBrush(QColor(128, 128, 128, 170))
|
painter.setBrush(color)
|
||||||
for p in points:
|
|
||||||
painter.drawRect(QRectF(p.x() - 5, p.y() - 5, 10, 10))
|
painter.drawRect(QRectF(p.x() - 5, p.y() - 5, 10, 10))
|
||||||
|
|
||||||
center = np.mean(s.bbox, axis=0)
|
center = np.mean(s.bbox, axis=0)
|
||||||
|
|
|
||||||
|
|
@ -192,6 +192,15 @@ class CanvasStateMixin:
|
||||||
self.update()
|
self.update()
|
||||||
return True
|
return True
|
||||||
|
|
||||||
|
# swap_active_slice_left_right: function definition for ScanSlicer behavior.
|
||||||
|
def swap_active_slice_left_right(self):
|
||||||
|
active_slice = self.get_active_slice()
|
||||||
|
if active_slice is None:
|
||||||
|
return False
|
||||||
|
active_slice.swap_left_right()
|
||||||
|
self.update()
|
||||||
|
return True
|
||||||
|
|
||||||
# reset_active_slice_rotation: function definition for ScanSlicer behavior.
|
# reset_active_slice_rotation: function definition for ScanSlicer behavior.
|
||||||
def reset_active_slice_rotation(self):
|
def reset_active_slice_rotation(self):
|
||||||
active_slice = self.get_active_slice()
|
active_slice = self.get_active_slice()
|
||||||
|
|
|
||||||
|
|
@ -26,6 +26,7 @@ class ScanSlicerCanvasActionsMixin:
|
||||||
self.active_box_lock_button.setEnabled(False)
|
self.active_box_lock_button.setEnabled(False)
|
||||||
self.active_box_squarize_button.setEnabled(False)
|
self.active_box_squarize_button.setEnabled(False)
|
||||||
self.active_box_reset_rotation_button.setEnabled(False)
|
self.active_box_reset_rotation_button.setEnabled(False)
|
||||||
|
self.active_box_swap_button.setEnabled(False)
|
||||||
self.active_box_remove_button.setEnabled(False)
|
self.active_box_remove_button.setEnabled(False)
|
||||||
self.active_box_lock_button.blockSignals(True)
|
self.active_box_lock_button.blockSignals(True)
|
||||||
self.active_box_lock_button.setChecked(False)
|
self.active_box_lock_button.setChecked(False)
|
||||||
|
|
@ -38,6 +39,7 @@ class ScanSlicerCanvasActionsMixin:
|
||||||
self.active_box_lock_button.setEnabled(True)
|
self.active_box_lock_button.setEnabled(True)
|
||||||
self.active_box_squarize_button.setEnabled(True)
|
self.active_box_squarize_button.setEnabled(True)
|
||||||
self.active_box_reset_rotation_button.setEnabled(True)
|
self.active_box_reset_rotation_button.setEnabled(True)
|
||||||
|
self.active_box_swap_button.setEnabled(True)
|
||||||
self.active_box_remove_button.setEnabled(True)
|
self.active_box_remove_button.setEnabled(True)
|
||||||
self.active_box_lock_button.blockSignals(True)
|
self.active_box_lock_button.blockSignals(True)
|
||||||
self.active_box_lock_button.setChecked(active_slice.locked)
|
self.active_box_lock_button.setChecked(active_slice.locked)
|
||||||
|
|
@ -69,6 +71,13 @@ class ScanSlicerCanvasActionsMixin:
|
||||||
return
|
return
|
||||||
self.update_statusbar("Active box squarized.")
|
self.update_statusbar("Active box squarized.")
|
||||||
|
|
||||||
|
# swap_active_box_left_right: function definition for ScanSlicer behavior.
|
||||||
|
def swap_active_box_left_right(self):
|
||||||
|
if not self.slicing_canvas.swap_active_slice_left_right():
|
||||||
|
self.update_statusbar("No active box selected.")
|
||||||
|
return
|
||||||
|
self.update_statusbar("Active box left/right swapped.")
|
||||||
|
|
||||||
# reset_active_box_rotation: function definition for ScanSlicer behavior.
|
# reset_active_box_rotation: function definition for ScanSlicer behavior.
|
||||||
def reset_active_box_rotation(self):
|
def reset_active_box_rotation(self):
|
||||||
if not self.slicing_canvas.reset_active_slice_rotation():
|
if not self.slicing_canvas.reset_active_slice_rotation():
|
||||||
|
|
|
||||||
|
|
@ -230,6 +230,14 @@ class QtScanSlicer(QMainWindow, ScanSlicerUiBuilderMixin, ScanSlicerControllerMi
|
||||||
"NzRMMjEgMTYiLz48cGF0aCBkPSJNMTYgMTZoNXY1Ii8+PGNpcmNsZSBjeD0iMTIiIGN5PSIxMiIgcj0iMSIv"
|
"NzRMMjEgMTYiLz48cGF0aCBkPSJNMTYgMTZoNXY1Ii8+PGNpcmNsZSBjeD0iMTIiIGN5PSIxMiIgcj0iMSIv"
|
||||||
"Pjwvc3ZnPg=="
|
"Pjwvc3ZnPg=="
|
||||||
)
|
)
|
||||||
|
SWAP_LR_ICON_DATA_URL = (
|
||||||
|
"data:image/svg+xml;base64,"
|
||||||
|
"PHN2ZyB4bWxucz0iaHR0cDovL3d3dy53My5vcmcvMjAwMC9zdmciIHdpZHRoPSIyNCIgaGVpZ2h0PSIyNCIg"
|
||||||
|
"dmlld0JveD0iMCAwIDI0IDI0IiBmaWxsPSJub25lIiBzdHJva2U9ImN1cnJlbnRDb2xvciIgc3Ryb2tlLXdp"
|
||||||
|
"ZHRoPSIyIiBzdHJva2UtbGluZWNhcD0icm91bmQiIHN0cm9rZS1saW5lam9pbj0icm91bmQiPjxwYXRoIGQ9"
|
||||||
|
"Ik04IDMgNCA3bDQgNCIvPjxwYXRoIGQ9Ik00IDdoMTYiLz48cGF0aCBkPSJtMTYgMjEgNC00LTQtNCIvPjxw"
|
||||||
|
"YXRoIGQ9Ik0yMCAxN0g0Ii8+PC9zdmc+"
|
||||||
|
)
|
||||||
RESET_ROTATION_ICON_DATA_URL = (
|
RESET_ROTATION_ICON_DATA_URL = (
|
||||||
"data:image/svg+xml;base64,"
|
"data:image/svg+xml;base64,"
|
||||||
"PHN2ZyB4bWxucz0iaHR0cDovL3d3dy53My5vcmcvMjAwMC9zdmciIHdpZHRoPSIyNCIgaGVpZ2h0PSIyNCIg"
|
"PHN2ZyB4bWxucz0iaHR0cDovL3d3dy53My5vcmcvMjAwMC9zdmciIHdpZHRoPSIyNCIgaGVpZ2h0PSIyNCIg"
|
||||||
|
|
|
||||||
|
|
@ -18,6 +18,9 @@ class PhotoSlice:
|
||||||
self.locked = False
|
self.locked = False
|
||||||
# Side-length handles are an explicit edit mode, enabled by squarize.
|
# Side-length handles are an explicit edit mode, enabled by squarize.
|
||||||
self.side_handles_enabled = False
|
self.side_handles_enabled = False
|
||||||
|
# False = default winding, True = explicit left/right swap by the user.
|
||||||
|
# Persists through squarize/reset_rotation so manual orientation wins.
|
||||||
|
self.swapped = False
|
||||||
|
|
||||||
@property
|
@property
|
||||||
# bbox: function definition for ScanSlicer behavior.
|
# bbox: function definition for ScanSlicer behavior.
|
||||||
|
|
@ -49,8 +52,23 @@ class PhotoSlice:
|
||||||
if horizontal_score(reversed_winding) > horizontal_score(ordered):
|
if horizontal_score(reversed_winding) > horizontal_score(ordered):
|
||||||
ordered = reversed_winding
|
ordered = reversed_winding
|
||||||
|
|
||||||
|
# A user-requested left/right swap overrides the geometric inference,
|
||||||
|
# so manual orientation survives normalize_top_left().
|
||||||
|
if getattr(self, "swapped", False):
|
||||||
|
ordered = ordered[[1, 0, 3, 2]]
|
||||||
|
|
||||||
self.bbox = ordered
|
self.bbox = ordered
|
||||||
|
|
||||||
|
# swap_left_right: function definition for ScanSlicer behavior.
|
||||||
|
def swap_left_right(self):
|
||||||
|
# Mirror left/right: swap the two top corners and the two bottom
|
||||||
|
# corners, keeping the top (edge 0->1) and bottom edges in place.
|
||||||
|
# [TL, TR, BR, BL] <-> [TR, TL, BL, BR].
|
||||||
|
self.swapped = not self.swapped
|
||||||
|
self.bbox = self.bbox[[1, 0, 3, 2]]
|
||||||
|
self._rect_cache = None
|
||||||
|
self._rect_cache_tol = None
|
||||||
|
|
||||||
# toggle_locked: function definition for ScanSlicer behavior.
|
# toggle_locked: function definition for ScanSlicer behavior.
|
||||||
def toggle_locked(self, locked=None):
|
def toggle_locked(self, locked=None):
|
||||||
if locked is not None:
|
if locked is not None:
|
||||||
|
|
|
||||||
|
|
@ -123,6 +123,16 @@ class ScanSlicerUiPreviewMixin(ScanSlicerUiSharedMixin):
|
||||||
)
|
)
|
||||||
preview_toolbar_layout.addWidget(self.active_box_reset_rotation_button)
|
preview_toolbar_layout.addWidget(self.active_box_reset_rotation_button)
|
||||||
|
|
||||||
|
self.active_box_swap_button = self._create_toolbar_button(
|
||||||
|
preview_toolbar,
|
||||||
|
text="Swap L/R",
|
||||||
|
tooltip="Swap left/right orientation of the active box",
|
||||||
|
icon_data_url=self.SWAP_LR_ICON_DATA_URL,
|
||||||
|
callback=self.swap_active_box_left_right,
|
||||||
|
enabled=False,
|
||||||
|
)
|
||||||
|
preview_toolbar_layout.addWidget(self.active_box_swap_button)
|
||||||
|
|
||||||
self.add_box_button = self._create_toolbar_button(
|
self.add_box_button = self._create_toolbar_button(
|
||||||
preview_toolbar,
|
preview_toolbar,
|
||||||
text="Add box",
|
text="Add box",
|
||||||
|
|
|
||||||
Loading…
Add table
Add a link
Reference in a new issue