Repository navigation
Conversation
- 🐛 Defects:
- `_orientation_transform` built its viewport with the raw source width
for rotation 0 and 180 and applied `pixel_aspect` only in the 90/270
branch, while `MediaTransform.oriented_display_size` applies it to the
width for 0 and 180. `MediaTransform.scale` then fitted against a box
the mapping did not use, so `oriented_rect_to_source_rect` returned a
rectangle outside the frame: the oriented `CropSpec(8, 0, 8, 8)` on a
16x8 source at pixel aspect 2 mapped to `x=8 y=-4 w=8 h=16`, twice the
height of an 8 px frame and starting above it. Six of sixteen
rotation/pixel-aspect combinations were wrong, every one of them a 0 or
180 rotation with a pixel aspect other than 1 — so the crop rectangle
the user drags sat somewhere other than the region that was cut on an
anamorphic source without rotation.
- The viewport keeps the oriented display axes from
`MediaTransform.oriented_display_size` but rounds the pixel-aspect width
with the decoder's exact rule. Pixel aspect 1 is unchanged, which is what
the existing crop, staging and controller tests pin.
- Fractional display widths now use the decoder's round-half-up extent, so
a 720x480 source at pixel aspect 32/27 maps against 853 decoded pixels
rather than an 853.3333-pixel canvas.
- 🧪 Tests:
- Two matrices over rotation {0, 90, 180, 270} and pixel aspect
{1.0, 2.0, 1.5, 0.75} in `tests/core/test_crop.py`: the mapped
rectangle stays inside the source frame and round-trips back through the
display geometry, and `oriented_point_from_widget` round-trips through
`build_crop_geometry`. Twelve of the new cases fail without the fix. The
existing rotation-270/pixel-aspect-2 case is one of the ten combinations
that already passed, which is why the gap survived.
- A four-rotation fractional-aspect matrix asserts the viewport against
`_display_dimensions` itself rather than a written-out 853, so it fails
if either side's rounding drifts from the other.
- 🔧 Guardrails: third consecutive `fix:` touching `core/crop.py`, agreed
with the maintainer rather than taken silently. It is not a hardening
spiral: `a296a3b` bounded `fit_crop_aspect`, `cc9ec01` changed
`clamp_crop`, and this changes `_orientation_transform` — three
independent defects in three functions, and this one removes the
duplicated viewport expression instead of adding a guard. The round-half-up
rule does remain in two layers, because `core` may not import `jobs`; the
fractional-aspect test is what holds the two copies together.
Trigger: measured `oriented_rect_to_source_rect` over a synthetic 16x8
source at pixel aspect 2, rotation 0 — it returned a source rectangle at
y=-4 with height 16; a 720x480 source at pixel aspect 32/27 produced a
decoded width of 853 while the canvas extent was 853.3333.
Refs #164
- 📝 Design for user-drawn exclusion rectangles that force alpha 0 after
segmentation, so a burned-in overlay does not survive into the cutout:
- `FramingSpec.exclusions: tuple[CropSpec, ...]`, stored in oriented
source pixels and mapped into cut space once, in a new
`core/exclusion.py`. A region outside the crop produces no box and
stales nothing.
- The effective cut-space boxes enter the render, preview and union
fingerprints only when non-empty, so cut sets and union metadata
written by 0.4.1 stay byte-identical hits for users without regions.
- Six consumers read cut alpha for framing; the enumeration and the
three producers behind them are recorded, including the result
player, which the first draft missed.
- `_resolve_union` never reads `metadata.fingerprint` while the two
render-side checks do — one predicate replaces all three.
- Trim degrades to the untrimmed canvas when no pixel survives the
regions, rather than aborting the job.
- Regions are carried across a source change and clipped, not slid, to
the new frame, so what is painted and what is masked are the same
rectangle by construction.
- 📝 Records a prerequisite defect found while verifying the design: the
oriented viewport in `core/crop.py` omits the pixel aspect for rotation
0 and 180, which mispaints the crop rectangle on anamorphic sources.
Scoped out as its own fix.
Refs #80
- 📝 Scope:
- Records exclusion regions as in scope: several user-drawn rectangles
over the source frame, forced to alpha 0 after segmentation. No
existing setting addressed a burned-in overlay — `Crop` takes the
subject with it when the overlay sits in front of the chest, and a
largest-component filter was measured on a real clip to keep the
overlay at one sample and delete the subject's raised hand at two
others.
- Records the deviation that the rectangles are a parameter carried
across a source change rather than a per-source value, and that they
are clipped rather than slid to each new source, so the painted and
the masked rectangle are the same by construction.
- 📝 G8 makes this file authoritative, so the entry lands before any
implementation rather than as a tidy-up after it.
Refs #80
- ✨ Features:
- `FramingSpec.exclusions` carries user-drawn rectangles in oriented
source pixels. `core/exclusion.py` is the only place that maps them:
`cut_exclusions` translates by the crop origin, clips to the crop and
returns a sorted, de-duplicated tuple, so a region outside the crop
yields no box at all and two orderings of the same regions are the
same request. `exclude_alpha` is the only place that zeroes.
- The effective cut-space boxes enter the render, preview and union
identities only when non-empty, so every cut set and union record
written by 0.4.1 stays a byte-identical hit for a user with no
regions — the update does not invalidate a single cached cut.
- 🔄 Refactor:
- `cached_union` replaces three checks that had drifted apart:
`PreviewService._matching_union` and `RenderService._rebuild_union`
compared the fingerprint and the threshold, while the UI's
`_resolve_union` compared only the threshold and never read
`metadata.fingerprint` at all.
- `RenderService` stages each cut *before* `exclude_alpha` mutates it,
so the stored set stays unmasked and reusable under any regions and
no second full-resolution RGBA owner is allocated.
- `_iter_rebuild_frames` and the encoder's exclusion handling live in
`jobs/transform_stage.py` rather than the render orchestrator, and
`stage_encoder_frames` is split into named helpers — it was 66 lines
against the 60-line budget. `jobs/render.py` is unchanged at 2886
lines, so the frozen module does not grow.
- 🐛 Degradation:
- `union_alpha_bounds_or_none` returns `None` only for a genuinely
empty mask; a non-RGBA frame or mismatched canvas sizes still raise,
because `INVALID_FRAMING` covers all three and catching it would have
reported corrupt frames as an empty selection. Trim then frames the
untrimmed canvas and records a note instead of aborting the render.
- 🧪 Tests:
- `test_identities_without_exclusions_match_the_previous_schema` pins
the 0.4.1 compatibility above; it fails if the exclusions key is
emitted unconditionally.
- `test_render_stores_unexcluded_cuts_but_unions_and_encodes_without_
the_region` pins stage-then-mask, and
`test_render_peak_rgba_owner_count_is_unchanged_by_exclusions` pins
that it costs no extra owner.
- `test_union_alpha_bounds_or_none_propagates_invalid_frame_errors`
pins that the degradation does not swallow corrupt frames, and
`test_render_still_refuses_a_trimmed_box_below_the_minimum_dimension`
keeps the minimum-dimension refusal that the empty-mask change could
have taken with it.
- `test_cut_exclusions_is_order_independent_and_deduplicated`,
`test_rebuild_with_a_region_misses_the_cached_union_and_recomputes_it`
and `test_stage_encoder_frames_zeroes_excluded_boxes_before_framing`
cover the remaining seam behaviour.
Steps 2 and 3 of `docs/designs/exclusion-regions.md`; no UI yet.
Refs #80
- ✨ Features:
- `ParameterState.exclusions` with `ExclusionsChanged`, persisted under
`parameters/exclusions`. The reducer compares *effective* cut-space
boxes rather than raw tuples, so nudging a region that lies entirely
outside the crop leaves the preview valid.
- `clip_exclusions` intersects carried regions with each newly loaded
source and drops those that fall outside it. Intersection, not the
inward slide `clamp_crop` applies to a crop: sliding an exclusion
would move it onto wanted content. After the clip, the rectangle the
canvas would paint and the rectangle the renderer masks are the same
one.
- The Transform group and the result player now honour regions, which
are consumer sites 5 and 6 of the design's six. Without site 6 the
looping player would show the overlay the encoder removed.
- 🐛 Defects:
- `_resolve_union` accepted cached union metadata on a matching alpha
threshold alone and never read `metadata.fingerprint`, while the two
render-side checks read both. It now shares the single `cached_union`
predicate, so the Transform group's framed size no longer trusts a
union recorded under different inputs. This was wrong before
exclusions existed; a region merely made it reachable.
- ⚙️ Guardrails:
- `core/state.py` 882 to 883 lines: the one `SourceLoaded` line that
applies the clip. Raised in the baseline by hand for that entry alone
rather than with `--update`, which would also have recorded unrelated
drift in four modules that shrank since the baseline was written.
- `ui/transform_stage.py` is 750 lines against the 800 budget.
- 🧪 Tests:
- `test_loading_a_smaller_source_clips_carried_regions_and_drops_those_
outside` pins the clip, and
`test_moving_a_region_outside_the_crop_does_not_invalidate_the_preview`
pins that the reducer compares effective boxes.
- `test_facts_ignore_cached_union_metadata_whose_fingerprint_does_not_
match` fails on the previous threshold-only check.
- `test_frame_load_worker_masks_exclusion_boxes_before_framing` and
`test_region_change_recomputes_facts_and_reloads_player_frames_without
_the_region` pin sites 5 and 6.
- `test_malformed_exclusions_preference_falls_back_to_empty_regions`
pins that a damaged settings value degrades instead of raising.
Steps 4 and 5 of `docs/designs/exclusion-regions.md`. Regions still cannot
be drawn; the canvas and its controls are steps 6 and 7.
Refs #80
- ✨ UI: - `CropCanvas.set_exclusion_edit` makes region editing a mode switch: crop mode has no region targets and region mode has no crop targets, so overlaps do not become an N×9 priority list in `build_crop_geometry`. - `mouseMoveEvent` keeps the in-progress region local; `mouseReleaseEvent` emits exactly one `ExclusionsChanged`, turning a 200-sample drag into one dispatch and one facts worker instead of 200 dispatches, threads, and about 2,400 settings writes. - Arrow keys nudge by 1 source pixel, Shift by 10, Delete/Backspace removes the selected region, and Escape abandons a drag or clears selection; `Inspector.tab_widgets` keeps Edit before Clear in the keyboard route. - `ExclusionControls` keeps Edit, Clear, and the textual `%n region(s)` count in the inspector. The visible Edit label brings the row minimum to 122 px for the 322 px viewport while descriptive accessible names, tooltip, and count text remain intact. - 🧪 Tests: - `test_a_region_drag_dispatches_exactly_once_on_release` pins no command during 20 moves and one command on release; the rubber-band, 8×8 discard, mode isolation, selection, move, delete, keyboard, exit, and accessible-bounds tests pin the remaining canvas affordances. - `test_exclusion_controls_toggle_edit_mode_and_clear_regions` pins the three controls, their accessible names, the tooltip, clear behavior, and disabled state while editing is unavailable. - 🌐 Catalogues: - `pyside6-lupdate` extracted 535 source texts in both catalogues; `pyside6-lrelease` reports 535 finished and 0 unfinished for English and German, with `Edit` written as `Bearbeiten` in German. Refs #80
✨ Feature - ✨ Seed one exclusion region through the screenshot script’s existing state seam. 📝 Docs - 📝 Describe exclusion regions beside the cleanup controls. - 🖼️ Regenerate all tracked README screenshots from the real widgets. Refs #80
- 🐛 Defects:
- `_reduce_exclusions` returned the original state whenever the old and
new regions produced identical effective cut boxes, which threw the
raw tuple away with the recomputation. A region lying outside the
current crop could therefore not be cleared at all — Clear emitted
`ExclusionsChanged(())`, both effective sets were empty, and the
reducer ignored it. Deleting one of two effectively identical regions
and reordering regions failed the same way, and a region moved outside
the crop vanished on release although the design requires it to stay
painted. Every valid tuple is now stored; the effective boxes decide
only whether the preview is invalidated.
- With `crop is None` the tuple is stored without invalidation, because
there is no preview to invalidate.
- `SourceLoaded` clipped carried regions into `state.parameters`, but
settings were written only through `_dispatch_parameter`, so the
clipped value never reached disk: quitting without touching another
control restored the old region on restart, while touching any
unrelated control wrote the clipped tuple and made the loss permanent.
`_source_loaded` now persists the reduced parameters, so what is saved
no longer depends on what else the user happened to change.
- 🧪 Tests:
- Five tests asserted something other than their name and passed with
their production code reverted. The worst is the one that should have
caught the defect above: `test_exclusion_controls_toggle_edit_mode_and
_clear_regions` checked only that Clear emitted a command and never
sent it through the reducer, so it stayed green while Clear did
nothing. It now routes Clear through the reducer and asserts the tuple
is empty.
- `test_region_mode_gives_the_crop_no_hit_targets` clicked without
moving, so it would have passed with crop dragging still enabled; it
now drags. `test_leaving_region_mode_restores_crop_handles_and_keeps
_painting_regions` checked geometry keys that exist in both modes; it
now checks rendered pixels.
`test_render_peak_rgba_owner_count_is_unchanged_by_exclusions` had no
unexcluded baseline; it now compares one and checks the stored cuts,
the output alpha and the union bounds.
`test_malformed_exclusions_preference_falls_back_to_empty_regions`
passed even if the setting was never read, because the default is
already empty; it now proves the setting is read first.
- A new test crosses settings, `SourceLoaded` clipping and restart, the
path the persistence defect lived on.
- The gesture contract is now asserted on every path that can mutate a
region, not only the drag: resize and rubber band emit once on release
and never per pointer move, and keyboard nudge, delete and Clear each
emit once.
Trigger: repro — draw an exclusion region outside the current crop, press
Clear, and observe that nothing happens.
Refs #80
![gitar-bot[bot]](https://avatars.githubusercontent.com/in/827041?s=60&v=4){kind=small}
| if request.framing.trim and union is None: | ||
| notes.append("trim skipped: no pixel above the alpha threshold " | ||
| + "survives the exclusion regions") | ||
| plan = framing_plan((manifest.width, manifest.height), union, request.framing) |
There was a problem hiding this comment.
💡 Quality: Empty-union trim note misattributes cause to exclusion regions
_encode_snapshot appends the note "trim skipped: no pixel above the alpha threshold survives the exclusion regions" whenever request.framing.trim and union is None, but that condition is also reached when the segmentation itself produces nothing above the alpha threshold and there are no exclusion regions at all. In that case the render now degrades to an untrimmed canvas (previously framing_plan raised INVALID_FRAMING) and records a note that wrongly blames exclusion regions, obscuring an actually-empty or misconfigured segmentation. Consider wording the note conditionally on whether any exclusions were applied (e.g. only mention exclusions when request.framing.exclusions is non-empty), so a genuinely empty mask is reported for what it is.
Was this helpful? React with 👍 / 👎
- 🐛 Defects:
- A gesture kept its own region index while the canvas cleared only the
selection when the tuple shrank, so releasing a drag after the region
under it was deleted recreated that region at index 0 and raised
`IndexError` at index 1. `_reconcile_exclusion_state` now cancels a
drag or rubber band when the source, the index or the region's value
changes underneath it. Selection lives in the canvas while the regions
live in the reducer, and this is the seam where the two disagreed.
- Region painting and presses were gated on `model.crop_enabled`, so
unchecking Crop hid the regions and made Edit inert while it stayed
checked — and the hidden regions still removed pixels from the output.
Regions now have their own activation rule.
- `TransformStageController` compared raw framing tuples, so moving a
region that lies entirely outside the crop rescheduled the facts worker
and reloaded every frame of the result player: up to 300 PNGs re-read
for an edit with no effect on a single pixel. `_effective_framing
_changed` compares the mapped cut boxes while the raw framing is still
stored, the same distinction the reducer makes.
- `action_availability` left exclusions out of its comparison although
`ParametersReset` clears them, so Reset stayed disabled after drawing a
region and only woke up once an unrelated parameter changed.
- ⌨️ Accessibility:
- A keyboard-only user could not create a region, could not select one,
and therefore could not delete an individual region — full keyboard
reachability is a V1 scope commitment, so this was a scope defect
rather than a rough edge. Insert, N and Space create, Tab and Backtab
cycle the selection, Delete removes the selected region.
- A nudge compared the candidate against the *crop* rather than the
region being edited, so a region that happened to coincide with the
crop could not be moved that last pixel.
- The canvas announced "Crop bounds" while in region mode; it now
announces the mode and the selected region.
- 🔄 Refactor:
- The region mode moved out of `ui/crop_canvas.py` into a new
`ui/exclusion_canvas.py`, leaving 410 and 594 lines rather than one
module pressing against the 800-line budget.
- 🧪 Tests:
- Three seams had no test crossing them, each with a production edit that
would have broken behaviour while the suite stayed green: deleting the
`main_window` signal connection left Edit checked while dragging still
edited the crop; swapping `request.crop` for an origin-zero crop of the
same size misplaced every exclusion, which no fixture caught because
they all used origin-zero crops; and writing a union fingerprint
computed with empty exclusions kept the pixels right while missing the
cache forever after. One test each now crosses them.
- `test_region_state_change_cancels_an_in_flight_drag` pins deletion,
replacement and reindexing under a live gesture;
`test_region_change_outside_the_cut_does_not_recompute_or_reload_frames`
pins that an ineffective edit costs nothing;
`test_arrow_key_can_move_a_region_onto_the_crop_bounds` pins the final
pixel; `test_region_mode_keyboard_creates_a_region` and
`test_region_mode_keyboard_cycles_selection_and_deletes_it` pin the
keyboard path end to end.
- 📝 The German catalogue carries the new announcement strings; 0 unfinished
in both languages.
Trigger: repro — press and hold on a region to start a drag, Tab to the
inspector's Clear and activate it with Space, then release the mouse.
Refs #80
|
A third independent review ran over the branch and requested changes; A cleared region could come back, or crash. A gesture kept its own region index while the canvas cleared only the selection when the tuple shrank. Press and hold on a region to start a drag, Tab to Clear and activate it with Space, then release: at index 0 the deleted region was recreated, at index 1 it raised Unchecking Crop disabled exclusion editing while Edit stayed checked. Region painting and presses were gated on A keyboard-only user could not create, select or individually delete a region. Full keyboard reachability is a V1 scope commitment, so this was a scope defect rather than a rough edge. Insert, N and Space create; Tab and Backtab cycle; Delete removes. The canvas also announced "Crop bounds" in region mode and now announces the mode and the selected region. An edit with no effect on a single pixel reloaded the whole result animation. Reset stayed disabled after drawing a region, because Three seams had no test crossing them, each with a production edit that would have broken behaviour while the suite stayed green: deleting the The region mode moved out of Verification: ruff, mypy on all three platforms, guardrails, Still to come on this branch: a right-click context menu on the source frame, so a region can be created where the user is looking rather than only from the inspector toggle. |
- ✨ Features:
- Right-clicking the source frame now offers the region actions where the
user is looking, instead of only through the inspector's Edit toggle.
`ExclusionCanvas.contextMenuEvent` builds the menu from what lies under
the pointer: *Exclude this area* over empty canvas, *Remove this
region* over a region, plus *Remove all regions* whenever any exist and
a checkable *Edit exclusion regions* that mirrors the inspector.
- `_create_exclusion` places a region sized at 20% of the source frame,
clamped to it, selects it and enters region mode. The action creates a
default-sized region rather than starting a rubber band, because
beginning a drag gesture from a menu is unreliable; the handles that
already exist resize it.
- Every entry is one `ExclusionsChanged` through the reducer, never a
direct canvas mutation, so the menu cannot drift from the inspector.
- The menu is absent while editing is unavailable, rather than offering
entries the reducer would silently drop — this is the first context
menu in the application, so it sets the pattern.
- `contextMenuEvent` carries Shift+F10 and the Menu key, and the keyboard
path opens at the focused region rather than at the widget origin.
- 🔄 The two menu shapes are built by one path with the entries in the same
order in both. A region under the pointer implies at least one exists, so
a single condition covers what were two duplicated branches that also
placed *Remove all regions* differently in each.
- 🧪 Tests:
- `test_context_menu_lists_region_actions_for_empty_and_occupied_canvas
_areas` pins both menu shapes and the checked mode state.
- `test_context_menu_excludes_a_clamped_default_region_with_one_dispatch`
pins one dispatch, the clamp, the selection and the mode change;
`test_context_menu_removes_the_region_under_the_pointer` and
`test_context_menu_removes_all_regions_with_one_dispatch` pin targeted
and bulk deletion at one dispatch each.
- `test_keyboard_context_menu_uses_the_focused_region_position` pins the
keyboard route and a position that is not the widget origin;
`test_context_menu_offers_no_actions_when_editing_is_unavailable` pins
the capability gate;
`test_source_context_menu_edit_action_keeps_inspector_mode_in_sync`
pins that both surfaces agree.
- A `QAction` carries no accessible-name API — assistive technology reads
the action's text — so a dynamic property of that name was a no-op that
looked like accessibility work. It is gone, and the test that asserted
it now asserts the text is present instead.
- 📝 Decision 6 of `docs/designs/exclusion-regions.md` records the menu, why
it creates a default-sized region, and that the inspector row remains the
discovery and keyboard surface. Catalogues carry the four new strings,
540 total, 0 unfinished in both languages.
Refs #80
- 🐛 Defects:\n - Region creation now cancels a different in-flight drag before the new selection can be nudged or released.\n - Bracket keys cycle regions without entering Qt's focus-traversal path, and crop handles stay hidden when Crop is disabled.\n- 🧪 Tests:\n - The drag regression covers N, a reducer presentation update, an arrow nudge and the later mouse release.\n - The keyboard regression uses a real window with a second focusable control and checks the Crop-off rendering.\n\nTrigger: repro — drag an existing region, press N, nudge the new region and release; with another focusable widget present, Tab moves focus instead of selecting a region.\n\nRefs #80
- 🐛 Defects:\n - Context-menu actions now revalidate editability when triggered, and removal resolves the captured CropSpec instead of a stale tuple index.\n - Keyboard context menus pass the selected region directly, so overlapping regions share one deletion target with the Delete key.\n - Open menus are deleted after dismissal, including the no-action path.\n- 🔄 Structure:\n - Context-menu behavior moves to ; stays below the 800-line budget.\n- 📝 Corrections:\n - claimed every context-menu entry emits ; Edit changes UI-local mode and emits none, so that commit record is corrected here.\n- 🧪 Tests:\n - Keyboard-menu tests use Qt's Menu key, pin the focused position, and cover overlapping deletion.\n - Regression tests cover stale permissions, stale targets and four open-and-dismiss cycles.\n\nTrigger: repro — retain a menu, change editability or replace its regions, then trigger the old action; open a keyboard menu over overlapping regions and remove the selection; repeat open and dismiss four times.\n\nRefs #80
- 🐛 Defects: - Match retained remove actions to the captured CropSpec object, so an equal duplicate that disappeared cannot delete its survivor. - 🧪 Tests: - Cover the duplicate-target race and dismiss four real QMenu.exec openings. Trigger: repro — retain a remove action for one of two equal-valued regions, replace it with the other, and open and dismiss four context menus. Refs #80
- 🐛 Defects: - Paint each region's `cut_exclusions` intersection with the existing solid red treatment and fade the inert remainder with a dashed outline. - Keep full-region geometry for handles and announce partial or wholly ineffective selections. - 🧪 Tests: - Render and compare effective, ineffective, inside, and outside canvas regions; assert the painted fragment is reconstructed from `cut_exclusions`. - Cover truthful accessibility announcements for partial and wholly outside regions. - 📝 Docs: - Record the effective/inert painting rule in Decision 6 and regenerate English/German Qt catalogues. Trigger: repro — draw a region across the crop edge over a bystander's face; the result shows only a narrow edge bite while most of the marked face remains. Refs #80
- 🐛 Defects: - Removed `compact_field` from the count label and placed it on a full-width line, keeping “1 Region” visible at 322 px. - Connected canvas selection to a readout of stored oriented-source coordinates and classified crop overhang through `cut_exclusions`; no selection clears stale values. - 🧪 Tests: - Added one behavior test for count width and three for selected, empty, and overhanging readouts. - 📝 Catalogues: - Regenerated English/German sources and binaries with 547 finished entries and 0 unfinished entries each. Trigger: repro — at the 322 px inspector viewport, the Exclusions row painted “1 Region” as “1 Re”. Refs #80
- 🐛 The selected-region readout was assembled from a stem plus the appended fragments `"; entirely outside crop"` and `"; partly outside crop"`. A fragment carries no grammar of its own, so it forces every language into English word order and cannot be translated at all by one that has to lead with the qualifier or restructure the clause. German happens to append too, which is why it looked correct. - 🛠 Each case is now one complete literal sentence chosen by `_selection_template`, matching how the canvas announcement added in `70293e0` already did it. `_crop_relation` names the three cases from the same `cut_exclusions` mapping the renderer uses, so the readout cannot disagree with the painting. - 📝 Catalogues regenerated: 547 strings, 547 finished, 0 unfinished in both languages; the two fragments are gone rather than left obsolete. Trigger: review of the strings added in `2cf9c8a` — three catalogue entries for one sentence. Refs #80
- 📝 Decision 7 adds regions that blank the frame *before* segmentation, so
a bystander at the frame edge is never read as a person at all. Zeroing
alpha afterwards removes them from the output but leaves the model
treating them as foreground, which distorts the matte of the wanted
subject — the post path cannot fix that.
- One global switch rather than a flag per region: about 60 lines
against about 200, and the request-level shape is identical, so it can
grow into a per-region flag later without a migration.
- Pre-model is a superset of post, never an alternative: with the switch
on, a region acts in both stages.
- The boxes enter the cut cache key only when non-empty, so a user with
no pre-model region keeps every cut set stored by 0.4.1 byte for byte.
Verified case by case against the literal key pinned in
`tests/core/test_fingerprints.py`.
- The fill identifier lives inside the key's value, so changing the fill
later invalidates only pre-model sets.
- 📝 Records a trap found while verifying it: `_validate_cache_inputs`
accepts only exactly known keys, so without a validator change every
manifest carrying a pre-model region would be rejected as invalid and
the cache would silently never be reused.
- 📝 The fill is to be measured, not asserted. A hard black rectangle is
itself an edge and a model may read an edge as an object, which would
make the feature worse than not using it. Six candidates, a paired
harness against a real model, a metric, a rejection rule, and
`imagenet-mean` as the fallback — the one candidate with a
model-independent argument, being the normalisation zero of twelve of
the thirteen models.
- 📝 Marks Decisions 1 to 6 as implemented; the document still claimed no
implementation existed while fifteen commits had landed.
Refs #80
- ✨ Features: - `blank_model_input` fills model boxes with `imagenet-mean` `(124, 116, 104)` while preserving alpha; `_produce_cut_frame` blanks before segmentation and zeroes the result afterward. - `_model_exclusions` emits immutable cut-key `boxes` only for effective non-empty regions, preserving the canonical no-region key; `_validate_cache_inputs` accepts exactly the optional manifest schema. - `ExclusionsBeforeModelChanged` selects segmentation invalidation, while effective empty regions remain inert; `request_for_workspace` reverses manifest cut boxes into source-space `CropSpec` values. - 🧪 Tests: - `test_segmentation_spec_rejects_exclusions_that_are_not_crop_specs` pins segmentation validation; `test_blank_model_input_fills_each_box_and_nothing_else` and `test_blank_model_input_with_no_boxes_leaves_the_image_byte_identical` pin fill scope and empty-path identity. - `test_cut_key_without_model_exclusions_matches_the_canonical_schema`, `test_cut_key_ignores_a_model_exclusion_outside_the_crop_or_clipped_to_nothing`, `test_cut_key_tracks_model_exclusion_boxes_and_the_fill_identifier`, `test_cut_key_inputs_with_model_exclusions_are_recursively_immutable`, `test_preview_identity_tracks_model_exclusions`, and `test_render_and_union_identities_ignore_model_exclusions` pin fingerprint identity and immutability. - `test_manifest_accepts_model_exclusions_and_reproduces_the_cut_key`, `test_manifest_rejects_malformed_model_exclusion_boxes`, and `test_manifest_without_model_exclusions_still_validates` pin manifest compatibility. - `test_exclusions_before_model_invalidates_the_preview_as_segmentation`, `test_exclusions_before_model_with_no_effective_region_does_not_invalidate`, `test_region_change_invalidates_as_segmentation_while_the_switch_is_on`, `test_parameters_reset_clears_exclusions_before_model`, and `test_exclusions_before_model_settings_round_trip_and_malformed_value_fall_back` pin reducer behavior. - `test_render_blanks_model_exclusions_and_zeroes_stored_cut`, `test_render_with_model_exclusions_reads_the_original_pixels_outside_the_box`, `test_render_with_the_switch_off_sends_the_model_the_unblanked_frame`, `test_render_reuses_the_unblanked_cut_set_after_the_switch_is_turned_off`, `test_render_with_whole_frame_model_exclusion_notes_untrimmed_canvas`, `test_preview_shows_no_filler_pixels_inside_a_model_exclusion`, and `test_rebuild_of_a_model_exclusion_set_reproduces_its_key_from_the_manifest` pin the shared pipeline and cache reuse. Refs #80
- ✨ Features: - `ExclusionControls` dispatches `ExclusionsBeforeModelChanged`, disables the inert switch without regions, and names the re-segmentation cost in the checkbox and count label. - `ExclusionCanvas` uses a `BDiagPattern` hatch and 2 px red pen for effective pre-model boxes, and appends the literal accessible announcement suffix. - `ParameterPresentation`, `CropPresentation`, preferences, and `_render_request` carry the global flag; `scripts/screenshots.py` captures it in the generated main-window image. - 🎨 Style: - README describes the separate cached cut set, and regenerated English/German catalogues contain all 551 finished messages with the four new translations. - 🧪 Tests: - `test_before_model_checkbox_dispatches_and_is_inert_without_regions` and `test_before_model_count_label_names_the_blank` pin dispatch, inert state, checked state, and count wording. - `test_model_exclusions_are_painted_hatched_and_announced` pins visible hatching and the accessible suffix; `test_exclusions_before_model_persists_as_a_bool` pins the QSettings bool. - `test_parameter_and_crop_presentations_carry_exclusion_regions` and `test_preview_and_render_requests_share_every_inspector_parameter` pin state projection and switch-dependent request tuples. Refs #80
- ⚙️ `jobs/render.py` 2886 to 2889 lines. `_produce_cut_frame` gains the two halves of the pre-model path: `blank_model_input` on the cropped frame before it reaches the model, and `exclude_alpha` on the result, because rembg composites the input RGB back into what it returns. Both have to sit between the crop and the model call, and both are in that function. - ⚙️ Raised by hand for this one entry rather than with `--update`, which would also record unrelated drift in four modules that shrank since the baseline was written. Refs #80
Parked, not abandonedConverted to draft and labelled What works and is greenDecisions 1–6 of The unresolved defect that caused the pauseTested on a real clip, the cutout does not match what the region appears to select. Measured so far:
So the defect is somewhere the above does not cover, and it was not found before the pause. The readout added in Pre-model regions are new and unprovenDecision 7 (blank the frame before the model, so a bystander is never read as a person) is implemented and unit-tested, but has never been run against a real model. Two things are open by design:
Review trailFour independent review rounds, each of which found real defects the previous one missed: the design was rejected once and revised; the implementation was rejected twice. Findings are recorded in the comments above. The last round's findings are all addressed. Before resuming
Depends on #165, which is not parked and stands on its own. |
|
| available = editable and presentation.duration is not None | ||
| self.edit_button.setEnabled(available) | ||
| self.clear_button.setEnabled(available and count > 0) | ||
| self.before_model_checkbox.setEnabled(available and count > 0) | ||
| blocked = self.before_model_checkbox.blockSignals(True) | ||
| try: | ||
| self.before_model_checkbox.setChecked( | ||
| presentation.exclusions_before_model | ||
| ) | ||
| finally: | ||
| self.before_model_checkbox.blockSignals(blocked) |
There was a problem hiding this comment.
💡 Edge Case: before-model flag persists silently across a clear-all
When "Blank before the model" is on and the user then removes all regions, apply disables the checkbox (available and count > 0) but leaves exclusions_before_model set to True in state and the box checked-but-greyed. Adding a new region later re-enables the flag, so the next region is silently re-segmented and blanked before the model rather than treated as a normal post-segmentation exclusion — behavior the user did not re-request and could not toggle off during the zero-region window. Consider clearing exclusions_before_model when the region set becomes empty, or keeping the checkbox interactive so the preference can always be turned off.
Was this helpful? React with 👍 / 👎
CI failed: Type-checking errors due to missing arguments in UI crop functions, and a Python segmentation fault during PySide6 UI test execution.OverviewTwo major failures were detected in the CI pipeline: static type-checking errors via mypy and a Python segmentation fault (exit code 139) during GUI/PySide6 test execution. FailuresMissing Named Arguments in Type Checker (confidence: high)
Segmentation Fault During UI Tests (confidence: high)
Summary
Code Review 👍 Approved with suggestions 1 resolved / 3 findingsExclusion regions for masking burned-in overlays after segmentation, with staged masking before encoding to avoid re-segmentation on every adjustment. Six consumers of the exclusion mapping, comprehensive test coverage including two rejection-round fixes, and verified against ruff, mypy, guardrails, and 2086 passing tests. Consider wording the empty-union trim note conditionally on whether exclusions were applied, so a genuinely empty mask is reported accurately rather than blamed on regions. Also consider clearing the before-model flag when the region set becomes empty, so the flag does not silently persist across a clear-all and re-enable unexpected re-segmentation when a new region is added. 💡 Quality: Empty-union trim note misattributes cause to exclusion regions📄 src/matteloop/jobs/render.py:1378-1381
💡 Edge Case: before-model flag persists silently across a clear-all📄 src/matteloop/ui/exclusion_controls.py:57-67 📄 src/matteloop/core/parameters.py:395-409 When "Blank before the model" is on and the user then removes all regions, ✅ 1 resolved✅ Bug: Crop resize handles painted over a disabled/inactive crop
🤖 Prompt for agentsTip Comment OptionsAuto-apply is off → Gitar will not commit updates to this branch. Comment with these commands to change the behavior for this request:
Important Your trial ends in 7 days — upgrade now to keep code review, CI analysis, auto-apply, custom automations, and more. Was this helpful? React with 👍 / 👎 | Gitar |
1 participant
Closes #80.
A burned-in overlay — a composited Twitch chat box, an alert box, a webcam
frame, a bystander at the frame edge — is read as foreground and survives into
the cutout.
Cropcannot remove it when it sits in front of the subject, andthe issue records that a largest-component filter was measured on a real clip
to keep the overlay at one sample and delete the subject's raised hand at two
others.
This adds exclusion regions: several rectangles the user drags over the source
frame, forced to alpha 0 after segmentation.
Why after segmentation, not before
The crop is part of the cut key (
core/fingerprints.py), so masking the inputwould re-segment the whole clip on every drag — minutes per adjustment. And
rembg models judge the frame globally, so blanking a region changes the model's
reading of everything else. Zeroing alpha afterwards is deterministic, and it
reuses the stored cut set: Render and Rebuild never re-segment because of a
region. The preview does re-run the model when it is refreshed, because only
the displayed image is retained; that limit is stated rather than papered over.
How it is put together
core/exclusion.pyholds the only mapping (cut_exclusions,which translates by the crop origin, clips to the crop, sorts and
de-duplicates) and the only zeroing (
exclude_alpha). Regions are stored inoriented source pixels; stored cut frames are crop-sized, so the translation
exists once rather than at each call site.
union, encoder staging, the Transform group's framed size, and the result
player. The first draft of the design listed five and missed the player —
which would have looped the overlay the encoder removes. The module docstring
enumerates all six and the three producers behind them.
place, so the stored set stays unmasked and reusable under any regions and no
second full-resolution RGBA buffer is allocated.
cut set and union record written by 0.4.1 stays a byte-identical hit for a
user with no regions. Updating invalidates nothing.
cached_unionreplaces three checks that had drifted apart. Two comparedthe fingerprint and the threshold; the UI's
_resolve_unioncompared only thethreshold and never read
metadata.fingerprint. That was wrong before thisbranch — a region merely made it reachable.
render frames the untrimmed canvas and records a note. The empty-mask signal
is distinct from
INVALID_FRAMING, which also covers a non-RGBA frame andmismatched canvas sizes, so a corrupt frame still fails loudly.
slid — to each new source, so the rectangle the canvas paints and the
rectangle the renderer masks are the same one. Sliding, which is what
clamp_cropdoes to a crop, would move an exclusion onto wanted content.mode the crop has no hit targets, in crop mode the regions are painted but
inert, so a region lying over a crop handle is never ambiguous. Edits are
dispatched once on release, never per pointer move —
_schedule_factsstarts a QThread with no debounce and each dispatch rewrites about twelve
QSettings keys, so a 200-sample drag would otherwise mean 200 threads and 200
cut scans.
Review trail
The design (
docs/designs/exclusion-regions.md) was written, independentlyreviewed, rejected, and revised; the implementation was then independently
reviewed and rejected again. Both rounds are worth knowing about:
frames, that one dispatch per pointer move was a worker storm, and that
persisted regions would be painted where they do not apply.
of merely skipping recomputation, so Clear silently did nothing for a
region lying outside the crop; that source-load clipping never reached
persistence, making the saved value depend on whether the user happened to
touch another control; and that five tests asserted something other than
their name and passed with their production code reverted. The Clear test
is the instructive one: it checked only that a command was emitted and never
sent it through the reducer, so it stayed green while the feature was broken.
Both rounds are fixed in this branch, each with a test that fails without its
fix.
Depends on #165
That branch fixes the oriented-to-widget mapping, which this feature needs for
its painted-equals-masked guarantee. It is in this history until #165 lands;
rebase onto
mainafterwards.Guardrail baseline
Two entries raised by hand, each for exactly the lines this change adds, rather
than with
--update— which would also have recorded unrelated drift in fourmodules that shrank since the baseline was written:
core/state.py882 to 883: the oneSourceLoadedline applying the clip.ui/inspector.py897 to 904: import, construction, signal,apply, the formrow, and two tab-order entries.
jobs/render.pyis unchanged at 2886 lines: the four new call sites are offsetby the three union checks collapsing into one predicate.
Verification
Translation catalogues: 535 strings, 535 finished, 0 unfinished in both
languages. README and screenshots regenerated.