Skip to content

Agx UI toe shoulder sliders and primaries rotations #19642

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Open
wants to merge 29 commits into
base: master
Choose a base branch
from
Open

Agx UI toe shoulder sliders and primaries rotations #19642

wants to merge 29 commits into from
+667 −253

Conversation

@kofa73
Copy link
Contributor

@kofa73 kofa73 commented Nov 2, 2025 •
edited
Loading

  • The primaries hue and purity (incl. reversal) sliders now have visual hints (hue, purity);
  • The hue and purity sliders look like in color balance rgb, not like in sigmoid / rgb primaries (there is no white stripe from the neutral position to the selected value, obscuring some of the visual hints)
    • => it may make sense to alter sigmoid and rgb primaries the same way, but I did not include such a change in this PR
  • the black/white, toe / shoulder sliders have white/shoulder on the top, black/toe at the bottom
  • a new 'exposure picker' (camera icon): the module will read the exposure parameters from an early, enabled, preferably unmasked instance of exposure, in order to reflect user adjustments + in-camera exposure compensation and 'highlight preservation' modes. This required changes in _exposure.c_, develop.* and imageop.*. Unlike filmic, no estimate (using hard-coded exposure of 0.7 and reading the EXIF) is used, and no adjustment is done in reload_defaults, as the exposure instance is not yet available when it is called. The previous method (picking the limits based on image content) also remains.
  • in the params, the pivot input (x) coordinate is now stored explicitly, instead of storing a shift. The slider has been renamed to pivot relative exposure, as it displays the value in EV. The value is maintained if black/white relative exposure is changed via the sliders or exposure pickers. => module version upgrade to v7 + migration
    The consequences:
    • the pivot's distance from mid-grey remains constant, therefore if exposure limits are adjusted, the projection of the pivot remains the same (the same input levels are mapped to the same output levels as before). Previously, since the output remained unchanged, but the input changed, the mapping changed, and the region of highest contrast moved.
    • the shape of the curve will change, reflecting the movement of the pivot relative to the mid-grey-to-end-of-exposure-range distance.
    • pivot input (x) will still be enforced to stay inside the exposure range; if you shrink that so much that the pivot is forcibly moved (because it would fall outside the exposure limits), then expand the range again, the pivot will not be restored. For example: your black and white relative exposures were -8 EV and +5 EV. You set your pivot at mid-grey +2 EV. Then, you set a very low white exposure limit, like 1.5 EV -> the pivot will be forced to 2 EV (minus epsilon). Then, you restore your white relative exposure to 5 EV -> the pivot will remain at 2 EV.
    • there are now 2 pivot pickers, since some disliked the idea of the pivot input (x) picker also adjusting the output, since that is done by calculating what the brightness would be if we used the default mid-grey -> mid-grey mapping for the pivot. So, the picker next to pivot relative exposure only changes that slider. In order to set both the input and output value (as was done previously), there's a new picker, placed next to pivot target output. This may be a bit confusing, at first, but if you think about it, it makes no sense to set the output using a picker without also saying the brightness of what region is to be set to that value. Explanatory tooltips are provided.

@TurboGit TurboGit added this to the 5.4 milestone Nov 2, 2025
@TurboGit TurboGit added feature: enhancement current features to improve priority: medium core features are degraded in a way that is still mostly usable, software stutters feature: redesign current features to rewrite scope: UI user interface and interactions scope: image processing correcting pixels labels Nov 2, 2025
@kofa73 kofa73 marked this pull request as draft November 2, 2025 15:35
@kofa73
Copy link
Contributor Author

kofa73 commented Nov 2, 2025

Back to draft, checking an alternative way to calculate pivot y.

dterrahe
dterrahe reviewed Nov 3, 2025
View reviewed changes
dterrahe
dterrahe reviewed Nov 3, 2025
View reviewed changes
dterrahe
dterrahe reviewed Nov 3, 2025
View reviewed changes
kofa73 added 19 commits November 9, 2025 10:12
@kofa73
agx: Flipped toe and shoulder power + start + target black/white
d6f5d83
@kofa73
agx: Added "read exposure", setting black/white relative exposure fro…
582d61c 
…m exposure data, using the heuristics from filmic RGB.
@kofa73
agx: Fixed toe/shoulder power warning redraw; fixed 'read exposure'
9471730
@kofa73
agx: removed workaround to redraw of toe/shoulder warning, now proper…
a6241ce 
…ly fixed in the GUI framework
@kofa73
agx: hue/purity sliders default to 0; removed useless comments and le…
058c04e 
…ft-over debug code; inset/outset hard-limited to 0.99
@kofa73
agx: turn 'read exposure' into a button, so it can have a shortcut; f…
47913ca 
…ix exposure picker tooltip
@kofa73
agx: added tooltips for pickers
2acbeb3
@kofa73
agx: separated auto-tune from label
5998b94
@kofa73
agx: Separate pickers for pivot x and x+y
dfd684a
@kofa73
agx: Set 'feedback=0' for agx primaries hue and purity sliders for be…
944342f 
…tter visibility (no bar from neutral position to selected value)
@kofa73
agx: Removed plugins/darkroom/agx/reverse_hue_for_unrotation; reverse…
dbb51f4 
… rotation sliders always painted with reversed hues (showing the effect)
@kofa73
agx: Added exposure-based relative black/white to reload_defaults
7811e08
@kofa73
agx: Added _find_preferred_enabled_instance to develop.c (to be moved…
957a6fe 
…) to find 'best' exposure instance to read data from
@kofa73
agx: Switched pivot from shift to relative EV
47d0acb
@kofa73
agx: Added dt_iop_get_module_enabled_preferring_unmasked_first_instan…
510eeaf 
…ce to imageop; refactored finding the 'exposure' instance in develop.c.
@kofa73
agx: Don't adjust exposure limits for HDR input in reload_defaults
09bb010
@kofa73
agx: No 'headless fallback' mode for _exposure_proxy_get_effective_ex…
c0f70ae 
…posure (should always be called withe gui_data available)
@kofa73
agx: Try to keep pivot output when picking
a18b047
@kofa73
agx: Move painting the sliders into the functions _setup_purity_slide…
241645a 
…r, _setup_hue_slider.
@kofa73 kofa73 force-pushed the agx-ui-toe-shoulder-sliders-and-primaries-rotations branch from 7fa6e9f to 01e016d Compare November 9, 2025 09:12
@kofa73 kofa73 force-pushed the agx-ui-toe-shoulder-sliders-and-primaries-rotations branch from 01e016d to 9d0621d Compare November 9, 2025 09:14
kofa73
kofa73 commented Nov 9, 2025
View reviewed changes
kofa73
kofa73 commented Nov 9, 2025
View reviewed changes
@kofa73 kofa73 marked this pull request as ready for review November 9, 2025 09:37
@kofa73 kofa73 marked this pull request as draft November 9, 2025 09:47
@kofa73 kofa73 marked this pull request as ready for review November 9, 2025 10:04
TurboGit
TurboGit requested changes Nov 9, 2025
View reviewed changes
Copy link
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.

A bit frightening so close from the release.

Some style comments and a question. TIA.

src/iop/exposure.c

if (self->gui_data)
{
((dt_iop_exposure_gui_data_t *)self->gui_data)->effective_exposure = d->params.exposure;
Copy link
Member

@TurboGit TurboGit Nov 9, 2025

Choose a reason for hiding this comment

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

what happens if one read effective_exposure before commit_params is called?

Copy link
Contributor Author

@kofa73 kofa73 Nov 9, 2025

Choose a reason for hiding this comment

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

I assume they'd read 0. Currently, this is only used by agx, when the GUI is active, and the user clicks the 'camera' icon, so I assume exposure's commit_params will have been called.
It may be worth to update color calibration, too, which currently uses the proxy call that does not make the total correction into account:

const float user_exposure = exp2f(dt_dev_exposure_get_exposure(darktable.develop));

@kofa73
Copy link
Contributor Author

kofa73 commented Nov 9, 2025

A bit frightening so close from the release.

Yes, I know. I have not heard any bug reports from anyone, though.

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

Reviewers

@dterrahe dterrahe dterrahe left review comments

@TurboGit TurboGit TurboGit requested changes

Assignees

No one assigned

Labels

feature: enhancement current features to improve feature: redesign current features to rewrite priority: medium core features are degraded in a way that is still mostly usable, software stutters scope: image processing correcting pixels scope: UI user interface and interactions

Projects

None yet

Milestone

5.4

Development

Successfully merging this pull request may close these issues.

3 participants

@kofa73 @TurboGit @dterrahe