Skip to content

Fixes masks jumping around when crop is toggled.#20575

Merged
TurboGit merged 1 commit intodarktable-org:masterfrom
masterpiga:fix_20563
Mar 18, 2026
Merged

Fixes masks jumping around when crop is toggled.#20575
TurboGit merged 1 commit intodarktable-org:masterfrom
masterpiga:fix_20563

Conversation

@masterpiga
Copy link
Copy Markdown
Contributor

Hopefully, this fixes issue #20563.

Fix in action:

Screen.Recording.2026-03-18.at.16.14.21.mp4
  • Works with HQ/Preview pipelines
  • Works with different crop ratios
  • Works regardless of the relative position of the mask and the crop module

I don't know what is your regular benchmark for mask stress testing, let's see if @TurboGit or @jenshannoschwalm can break it ⚒️

More details below.

Root cause: two pipes, one pixel apart

darktable runs two independent pixel pipelines simultaneously:

  • Full pipe — processes the image at full resolution (e.g. 6022×4024 → crop → 4023px wide)
  • Preview pipe — processes a downscaled version (e.g. 1346×900 → crop → 899px wide)

Both apply exactly the same modules. The crop module computes its output size with
integer truncation:

roi_out->x     = MAX(0, (int)(roi_in->width  * d->cx));
roi_out->width = MAX(4, (int)(roi_in->width  * (d->cw - d->cx)));

For a 3:1 crop ratio, d->cx is adjusted so the cropped output is exactly 3× the
height. In the full pipe this might produce crop_left = 446 px; in the preview pipe,
working on a 6× smaller image, it might produce crop_left = 74 px. But
74 × 6 = 444 ≠ 446 — the two pipes are off by 2 input pixels, which is a fraction
of a preview pixel after downscaling. This fraction, multiplied by zoom_scale (~35×),
becomes ~35 screen pixels.


The five changes

1. src/iop/crop.c_get_crop_offset(): use integer crop offsets

Before: _get_crop_offset called modify_roi_out on a 100× scaled copy of
buf_in to reduce floating-point truncation error, then divided back by 100. This
produced a non-integer result (e.g. 446.69) that didn't match what the pipeline
actually used (integer 446), causing a ~0.7 px misalignment in every mask overlay.

After: Call modify_roi_out directly on buf_in at 1:1 scale. The result is the
exact same integer roi_out.x/y the pipeline computed during processing — mask
overlays now subtract the same integer offset the image data was cropped by.

2. src/develop/develop.cdt_dev_get_preview_size(): use actual preview dimensions

Before: Computed the preview canvas size as full_pipe->processed_width / iscale
— a floating-point division of the full-pipe output.

After: Uses preview_pipe->processed_width directly. These two values differ by
up to 1 pixel when crop's integer truncation lands differently across the two pipes.
All mask coordinate code (dt_masks_get_image_size, hit testing, Cairo overlay) now
uses the same canvas size the preview pipe actually produced.

3. src/develop/masks.hdt_masks_get_image_size(): same fix, same reason

This function is the single place all mask shape code fetches wd/ht (the canvas
size in which mask point coordinates live). It was computing
full.pipe->processed_width / iscale for the same reason as above. Changed to use
preview_pipe->processed_width as the primary source, with the old formula as a
fallback when the preview pipe hasn't processed yet.

4. src/develop/develop.cdt_dev_zoom_move(): prevent viewport drift on crop toggle

Before: Any call to dt_dev_zoom_move(DT_ZOOM_MOVE, 0, 0) — a validation-only
call with no actual movement — used a 0.5-pixel threshold to decide whether
port->zoom_x needed updating. When a crop was toggled, the pipeline changed, and
the backward transform of the viewport center could land just over 0.5px from the
stored position, silently updating zoom_x by a sub-pixel amount. This drift
accumulated and appeared as an image/mask shift.

After: Validation-only calls (zero x/y with DT_ZOOM_MOVE) use a 3.0-pixel
threshold, which is large enough to absorb the integer-truncation rounding across the
crop toggle without suppressing real panning.

5. src/views/darkroom.c — Cairo overlay alignment and hit-test correction

This is the most complex change, needed because even with the above fixes the ~1
preview pixel difference between full-pipe and preview-pipe viewport centers still
exists structurally (the two pipes are independent and will always differ slightly
after integer operations).

The remaining problem: Mask overlay points are computed by distort_transform
through the preview pipe (output space: 0…899 px). The Cairo drawing transform in
expose() was set up using the full-pipe viewport center (zoom_x from
dt_dev_get_viewport_params). At high zoom this 1-pixel difference in image space
becomes ~35 screen pixels.

zoom_x_vp computation: Before drawing, expose() now also transforms the
viewport center through the preview pipe to get zoom_x_vp — the preview-pipe
equivalent of zoom_x.

Drawing fix: The main Cairo transform for all overlays (guides, crop frame, color
picker) still uses the full-pipe zoom_x, so those stay aligned with the image
bitmap. Only inside the mask-specific cairo_save/restore block is an additional
cairo_translate((zoom_x - zoom_x_vp) * wd, ...) applied, shifting the coordinate
origin by the exact sub-pixel difference so mask overlays land correctly on the image.

Hit-test fix: Mouse event handlers (mouse_moved, button_pressed,
button_released, scrolled) compute the mouse position through the full pipe via
_get_zoom_pos. Before passing it to any mask event function, they now call
_preview_pipe_zoom_correction() to compute the same zoom_x_vp - zoom_x_full
difference and add it to pzx/pzy. This makes the mouse coordinate land in the
same preview-pipe space as the mask point coordinates, so anchor selection, dragging,
and segment proximity detection all work correctly.

@TurboGit
Copy link
Copy Markdown
Member

It's impressive, really! Was this done with the help of Claude?

@masterpiga
Copy link
Copy Markdown
Contributor Author

It's impressive, really! Was this done with the help of Claude?

This was really a team effort. Claude helped a lot, but it was going around in circles. It needed a lot of guidance, and I burned through one week of quota in one day 😭 But it would have taken me days of full time work to fix this by myself, especially because I am not familiar with this part of the code. An LLM can traverse all the relevant code and put debug statements when they are needed in ten seconds. It would take me hours just to find the right bits of code and type the stuff.

@TurboGit
Copy link
Copy Markdown
Member

Well, three of us have been looking at this issue without any progress. We had some ideas to fix that, don't remember all details, but this was a quite complete and invasive rewrite of the way the mask was computed.

A big thanks you for tackling that and for delivering a fix... that I didn't break!

@TurboGit TurboGit modified the milestone: 5.6 Mar 18, 2026
@TurboGit TurboGit added bugfix pull request fixing a bug priority: high core features are broken and not usable at all, software crashes scope: UI user interface and interactions scope: image processing correcting pixels release notes: pending labels Mar 18, 2026
@masterpiga
Copy link
Copy Markdown
Contributor Author

that I didn't break!

Yet! 🤣

Copy link
Copy Markdown
Member

@TurboGit TurboGit left a comment

Choose a reason for hiding this comment

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

Cannot break it! Masks stable and can be dragged to pixel precision up to zoom 1600.

Just need a release note entry in the fixes section. TIA.

@TurboGit TurboGit merged commit d69c3f2 into darktable-org:master Mar 18, 2026
5 checks passed
@masterpiga
Copy link
Copy Markdown
Contributor Author

@TurboGit Also here, you can remove the release_notes_pending tag. TIA!

@TurboGit
Copy link
Copy Markdown
Member

Done.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

ai:generated bugfix pull request fixing a bug priority: high core features are broken and not usable at all, software crashes scope: image processing correcting pixels scope: UI user interface and interactions

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants