select first enabled exposure module without mask

[dterrahe]

for histogram dragging, color calibration and agx.

As discussed here #19560 (comment) this makes more sense when the preferred shortcut criteria look at expanded or focused modules. @kofa73 please test if this does what you'd expect.

In the end, I duplicated the loop over all modules from dt_iop_get_module_preferred_instance because it was less readable to try to integrate the exposure lookup in the same loop when "prefer focused instance" would normally skip the lookup completely.

This also fixes a logic error where the "" modules shortcuts would only work if "prefer focused instance" was selected in preferences at the same time.

[dterrahe]

refactored

[TurboGit]

Looks good to me, thanks!

[TurboGit]

@kofa73 : Let me know when tested, I'll merge. TIA.

[wpferguson]

tested, works for me

[kofa73]

Let me quickly rebase agx on this.

[kofa73]

Sorry, won't go so quickly. How do I use this to find the first enabled, non-masked exposure instance?
I don't care about expanded, absolutely need (not just prefer) enabled (I only need exposure adjustments that take effect), prefer unmasked, and of course prefer the first instance, if all others are the same.
dt_iop_get_module_preferred_instance is not exposed via imageop.h.

It seems to solve the other problems, so I think a merge is OK, but currently I don't see how it helps me. Maybe I'm just too tired.

[dterrahe]

What if there is no enabled exposure module?

[kofa73]

What if there is no enabled exposure module?

Then exposure adjustment is 0. So "absolutely need (not just prefer) enabled" means: I want to filter out disabled instances that do not contribute any adjustment that would need to be taken into account when estimating the white/black point.

[dterrahe]

But then should you not have all enabled instances (presumably before agx) and aggregate them? Maybe that's what you were originally doing and I just missed it.

I'm sorry I haven't looked at all at what you are trying to do. I've hardly looked at the module's functionality itself I'm afraid.

[kofa73]

Using the 1st instance is better what filmic rgb currently does: it uses a hard-coded 0.7 EV correction, mirroring (but not reading etc.) the scene-referred default of exposure.
I think it's not unreasonable to say that the 1st instance of exposure (especially, if unmasked) is the one most probably used to set exposure correction, and the others just fine-tune that. It's an estimate only.

In exposure, reload_defaults() calls dt_iop_is_first_instance() (defined in imageop.c) to check whether it's the first instance, and only applies compensation for highlight preservation in that instance.
That's somewhat broken, because if the 1st instance is disabled, and the 2nd instance is the 'effective first', I think it won't do what it's supposed to. The other place it's called is color calibration (channelmixerrgb.c), in _declare_cat_on_pipe().

Maybe you should look into dt_iop_is_first_instance(), and its usages, too. I'm sorry to bring you more work. :-)

[dterrahe]

Inter-module interactions are somewhat fundamentally broken because the current design of the pipe execution and cache doesn't consistently support it. So there are band aids everywhere, like wiping the whole cache when we might need to rerun some bits to make sure everything is up to date. I don't enjoy thinking about the right kind of band aid as much as I like thinking about correct fixes and redesigns, but that's a big job that I'm still gathering courage for.

Part of that is knowing about "all" the places where workarounds or fragile approaches are in place that pass cursory testing but break in interesting ways, thereby betraying user confidence, discouraging the use of more complex scenarios and leading to magical thinking. All of those depend on the current execution rules and would need to be fixed/readapted if that is changed. I wasn't even aware of position dependent code in reload_defaults but assume that breaks when modules are reordered. Unless there's a band aid...

[kofa73]

I understand the concerns. I'm not sure what I can contribute, as I'm not familiar with darktable's architecture. We could say that such dependencies are not allowed, and live with the consequences, e.g. filmic rgb can stay with the 0.7 EV default, and not look into the other modules; I can do the same in agx, or remove the functionality altogether, since it would be broken anyway, made even worse by countering the 'highlight preservation' underexposure of the camera in the exposure module.
Alternatively, the instance of exposure that decides it is the one to perform such a correction publishes the value in a place that other modules can read. Still, the functionality to determine which instance of exposure is responsible for doing that has to be fixed 'somehow' (I understand it's a vague term), even if it's 'only' a question of instances of exposure being aware of their relative positions and whether the others are enabled. But even then, modules interested in 'the main' exposure instance would also have to somehow know if they are above or below said instance.

This is a can of worms. :-(

[dterrahe]

So I guess we are now clear that the existing proxy, or at least its use to pass on dragging on the histogram to the exposure sliders, and what you are looking for in agx are not going to align. So even though this will not serve this new purpose, I think it is an improvement for the existing one.

Can this PR be assessed and approved or rejected on that basis?

[kofa73]

Sure, I have no objections. I'm sorry if you have been waiting for that confirmation; I should have made that clear.

[TurboGit]

Looks good to me, thanks!