Skip to content

Conversation

@kofa73
Copy link
Contributor

@kofa73 kofa73 commented Nov 2, 2025

  • 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.

src/iop/agx.c Outdated

if(g && p)
{
dt_bauhaus_slider_set_hard_min(g->basic_curve_controls.curve_pivot_x_relative_ev, p->range_black_relative_ev);
Copy link
Member

Choose a reason for hiding this comment

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

Haven't reviewed or tested this, so this is more a general remark that may have little relevance here. For _from_params sliders, the intent is to have hard min/max set from the params $MIN $MAX values and keep them unchanged. That way we know that any validation imposed on the params based on introspection will enforce the same limits as the ui and hopefully can prevent modules crashing even if values set via some other means. Of course these are very simple per-field limits and don't enforce any consistency among variables. It may be possible to achieve consistency by changing this ev adjustment to some kind of proportional value (0-1 of the currently allowed range) but that may make the ui interaction awkward? You can still have the slider show the same values by setting appropriate offset and factor. Again, I'm perfectly fine if you ignore here, but something to keep in mind as an additional nice-to-have.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It used to be a proportional value -- well, 2 'proportional values': -1 to 0 to shift towards black, 0 to 1 to shift towards white. The steps in the two directions were not the same (e.g. black relative EV -10, white 6; -0.1 meant -1 EV, +0.1 meant 0.6 EV).
I'll have to look into offset and factor, I don't know much about the UI.

src/iop/agx.c Outdated
dt_color_picker_new(self, DT_COLOR_PICKER_AREA | DT_COLOR_PICKER_DENOISE,
dt_bauhaus_slider_from_params(self, "range_white_relative_exposure"));
GtkWidget *white_slider = dt_bauhaus_slider_from_params(self, "range_white_relative_ev");
dt_bauhaus_widget_set_module(white_slider, DT_ACTION(real_self));
Copy link
Member

Choose a reason for hiding this comment

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

Again, haven't tested this, but are you sure passing the real_self along like this is neceesary? The intention of my changes to DT_IOP_SECTION_FOR_PARAMS is that this lookthrough is done automatically (i.e. that dt_bauhaus_slider_from_params and dt_color_picker_new sets the module based on the "real" self not on the fake one; see DT_IOP_SECTION_FOR_PARAMS_UNWIND). If that is not happening and you required this workaround then that's a bug and I'd rather fix it than work around it everywhere.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'll have to check, thanks.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's not needed for dt_color_picker_new, but is needed for g_signal_connect and dt_action_define_iop, otherwise get crash at startup or when the picker/button is clicked.

kofa73 added 19 commits November 9, 2025 10:12
…m exposure data, using the heuristics from filmic RGB.
…ft-over debug code; inset/outset hard-limited to 0.99
…tter visibility (no bar from neutral position to selected value)
… rotation sliders always painted with reversed hues (showing the effect)
…) to find 'best' exposure instance to read data from
…ce to imageop; refactored finding the 'exposure' instance in develop.c.
…posure (should always be called withe gui_data available)
@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
GtkLabel *deflicker_used_EC;
GtkWidget *compensate_exposure_bias;
GtkWidget *compensate_hilite_preserv;
volatile float effective_exposure; // used to cache the final computed exposure
Copy link
Contributor Author

Choose a reason for hiding this comment

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

No idea if volatile is really needed here -- could this be read/written in different threads without some synchronization mechanism guaranteeing visibility?

{
dt_iop_exposure_gui_data_t *g = self->gui_data;
// should not be invoked when not in GUI mode
return g ? g->effective_exposure : 0.f;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't think it's ever invoked if gui_data is not available, so maybe the check should be removed?

@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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

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

Development

Successfully merging this pull request may close these issues.

3 participants