AgX: wrong history display for pivot relative exposure

[kofa73]

Is there an existing issue for this?

• I checked and did not find my issue in the already reported ones

Describe the bug

Root cause:

• The module stores the x coordinate of the pivot in the pivot_x property, presented via the curve/pivot relative exposure slider
• It modifies (and on each change, recalulates) the offset and scale of the slider, so the user is presented with an EV value
• In the history change popup, the previous value of pivot_x is interpreted using the current offset and scale.

For example, starting from black/white -10 EV -> 6.5 EV, pivot at mid-grey (0 EV shift), and changing the black/white relative exposure to -8 EV -> 5 EV, and setting the pivot shift to 1.5 EV, then hovering over the history item 12 will show the original pivot exposure as -0.12 EV, not as 0 EV, because the old pivot_x is interpreted using the new scale and offset:

Resetting the sliders and hovering over history item 13 will then show the pivot was moved from 2.06 EV to 0 EV (not the correct 1.5 EV to 0 EV):

Hovering over the previous entry, number 12 (which previously showed -0.12 EV -> 1.5 EV), shows the pivot was shifted from 0 EV to 2.06 EV:

The field stores the pivot_x value (0..1, x coordinate of the curve's pivot) and not the EV value, and changes visualisation (offset and scale) so we don't have to modify hard limits (which was the previous solution, using those hard limits to enforce the pivot is between black and white relative exposure). This was done to address #19642 (comment). @dterrahe

This is a relatively minor issue, and is probably preferable to messing with hard limits.

Steps to reproduce

See above

Expected behavior

the history should be displayed correctly

Logfile | Screenshot | Screencast

No response

Commit

No response

Where did you obtain darktable from?

darktable.org / GitHub release

darktable version

7035503

What OS are you using?

Linux

What is the version of your OS?

OS independent

Describe your system

No response

Are you using OpenCL GPU in darktable?

None

If yes, what is the GPU card and driver?

No response

Please provide additional context if applicable. You can attach files too, but might need to rename to .txt or .zip

No response

[dterrahe]

Ah.. the law of preservation of misery.

Well, in the past the history tooltip just showed the raw value, no formatting, so then it wouldn't have been an issue.

in hindsight, maybe it would have been best not to mess with hard limits/offsets/factors and only adjust soft limits. That would mean allowing the user to set values outside the supported range, which then would have to be CLAMPed in commit_params. It would also avoid changing the value when one of the other sliders rendered it invalid (and not changing it back when it became valid again). Recent bauhaus at least gives a subtle indication when the active value is outside the soft range.

But you probably feel that 7 params versions before the first release is already an unbeatable record...

[kofa73]

But you probably feel that 7 params versions before the first release is already an unbeatable record...

Something I never really aspired to... Then again, I wouldn't have thought the module would be this much work.
Would it make sense to drop all the migration logic before release, saying everyone who used the previous versions were essentially alpha/beta testers, and should migrate their stacks now, if desired, while the general public will never need those 6 previous version, and we don't have to worry about mess and maintenance?

[victoryforce]

Would it make sense to drop all the migration logic before release, saying everyone who used the previous versions were essentially alpha/beta testers, and should migrate their stacks now, if desired, while the general public will never need those 6 previous version, and we don't have to worry about mess and maintenance?

@kofa73 Totally agree! Convenience for alpha testers that they didn't ask for and that makes no practical sense (no one should use constantly changing development versions for production work) is not worth the code complexity that can easily be avoided.

[kofa73]

@victoryforce : I think having those conversions was more than 'convenience'. It helped people become early adopters, enabling them to use AgX as their daily driver, get a feel to it, and provide useful feedback. However, the conversion code is messy, and clearly shows my own struggles with the UI layer (e.g. at one point I migrated numbers by multiplying them by 100 to show them as percents on the UI, then I learned one could do it using a slider scaling factor and converted back, 2 version migrations cancelling out). So if the others agree, I'd gladly drop those ~400 lines of useless code that no one will ever use after the release. @TurboGit ?

[victoryforce]

@kofa73: darktable always keeps its promise that edits made in the release version of the program will continue to be supported in future releases even when the module's version changes. Changes to modules that occur during development, between releases, have no such guarantee. We appreciate early adopters, those who test development snapshots, but, again, no one should use constantly changing development versions for production work, and it's not worth cluttering up the code for the sake of it.

[kofa73]

@dterrahe : Does it make sense to keep this open? It will go away if the slider is changed not to display an EV value (there already is some demand to revert back to the previous version of the slider...), or if the framework is updated to create a new instance of the module only to display the history stack, using the config from the previous as well as current history entry (unlikely, as I'm probably the only one to twist controls like that, so not worth the hassle for a minor visualisation glitch).

[TurboGit]

Well 7 versions is indeed a lot before the first release. That being said lot of people have been told on pixlus some weeks ago that the AgX module won't change at this point. So removing all the legacy_params code will be a big surprise for many. If you go the way to remove all the versions (and I'm ok with that) then please communicate to pixlus clearly.

[dterrahe]

if the framework is updated to create a new instance of the module only to display the history stack, using the config from the previous as well as current history entry

That's crazy talk, as you well realize. It would not be a copy of the module only (which we already do for history anyway), but of its whole gui (because that's where we pick up the formatting from: the live version of the Bauhaus slider).

[dterrahe]

. If you go the way to remove all the versions

You'd probably still start with version 8 (just not perform legacy transforms) to avoid crashes (?) from misinterpreting (wrongly sized) old params? Essentially legacy_params would use defaults for all previous versions?

[kofa73]

@TurboGit

Well 7 versions is indeed a lot before the first release. That being said lot of people have been told on pixlus some weeks ago that the AgX module won't change at this point. So removing all the legacy_params code will be a big surprise for many. If you go the way to remove all the versions (and I'm ok with that) then please communicate to pixlus clearly.

Why would removing all the legacy_params code be a big surprise? Of course I would announce on pixls that anyone who wants to save pre-7 edits must upgrade now, this is the last chance. But I think that the adventurous people who use AgX as their daily driver are already on 7, at least with edits they want to keep.
Another option would be removing all pre-7 legacy migrations immediately after the release (still, announcing right now that we'll do so), when people testing builds will have upgraded to the released version.

@dterrahe

You'd probably still start with version 8 (just not perform legacy transforms) to avoid crashes (?) from misinterpreting (wrongly sized) old params? Essentially legacy_params would use defaults for all previous versions?

That would still break everyone's old edits, even if they are fully upgraded to v7. But we could keep the version at 7, and simply set defaults for all older versions, if people did not migrate in time.

[TurboGit]

Starting at 7 with no legacy_param() routine will certainly surprise many devs :) So if you go this path we need a big comment in the code. That being said, migrating to 8 with setting all to default or starting back to 1 (which should reset all value to default IIRC) has the same behavior, so why not just start back at 1?

[kofa73]

why not just start back at 1

Because many people are already at 7, and they relied on the promise that they'll be able to keep their edit.
My preferred solution would be:

• announce it now on pixls / reddit / facebook that everyone who wants to keep edits made with previous versions must upgrade now, as the last chance
• update the code:
  • keep a legacy_param() function
  • reset anything <7 to defaults of 7, with a comment in the code saying those were development versions, removed with the release
  • merge that in a few days

If you can live with that, I'll announce this tonight and send a PR.

[kofa73]

The announcement. I hope I got the migration details (presets, styles, exported metadata) correct:
https://discuss.pixls.us/t/heads-up-agx-backwards-compatibility-change-coming/54211