Skip to content
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.

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

better initialisation of history stack #5414

Merged
merged 3 commits into from
Jun 12, 2020
Merged

Conversation

elstoc
Copy link
Contributor

@elstoc elstoc commented Jun 11, 2020

when clearing the history stack or loading a new image

  • any module which is forced on and cannot be switched off appears first in the history stack
  • any module that is on by default but can be switched off appears next in the history stack
  • finally any modules that are enabled by presets or as a result of a preference setting (workflow or sharpen) are added

the final set of modules can be disabled and removed from the history stack but, as previously, the first two sets will be automatically re-added if they are removed

also fixes bug where highlight reconstruction was not present in the history stack

finally changes the scene-referred exposure defaults to a preset to ensure that they only apply to the first module instance and can be easily reapplied if necessary (by choosing the preset)

Resolves #5396, #5416

when clearing the history stack or loading a new image

 - any module which is forced on and cannot be switched off appears
   first in the history stack
 - any module that is on by default but can be switched off appears next
   in the history stack
 - finally any modules that are enabled by presets or as a result of a
   preference setting (workflow or sharpen) are added

the final set of modules can be disabled and removed from the history
stack but, as previously the first two sets will be automatically re-added if
they are removed

also fixes bug where higlight reconstruction was not present in the
history stack

Resolves darktable-org#5396
@elstoc
Copy link
Contributor Author

elstoc commented Jun 11, 2020

For some reason the flip module appears last in the stack and I can't figure out why. By no means is this a showstopper though because this PR fixes all of the issues in 5396 so IMO it's still better than master even if not 100% consistent.

@elstoc
Copy link
Contributor Author

elstoc commented Jun 11, 2020

Thinking on this again, the exposure autosetting really has to be managed via a preset, to ensure that subsequent instances don't also start with the same defaults. A second instance of exposure should never need the +1EV or exposure bias compensation. Filmicrgb default settings cannot be managed with a preset as it needs to set parameters based on exposure data from the image.

ensures that the scene-referred defaults are only applied to the first
instance of exposure
@TurboGit
Copy link
Member

For the flip module it is special cased in different part of the code. For example in dt_image_get_orientation (common/images.c). Maybe that's why it behave differently.

@TurboGit TurboGit self-requested a review June 12, 2020 07:19
@TurboGit TurboGit added this to the 3.2 milestone Jun 12, 2020
@TurboGit TurboGit added bugfix pull request fixing a bug feature: enhancement current features to improve priority: medium core features are degraded in a way that is still mostly usable, software stutters scope: codebase making darktable source code easier to manage labels Jun 12, 2020
@elstoc
Copy link
Contributor Author

elstoc commented Jun 12, 2020

I don't think it's an issue now I've moved everything else to presets. Also related to the second issue, I don't really understand what the code in database.c is designed to do. Maybe that has something to do with the basecurve auto-apply issues.

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.

Not tested yet but sounds correct to me.

Let me know if you agree about the preset.

src/iop/exposure.c Outdated Show resolved Hide resolved
@elstoc
Copy link
Contributor Author

elstoc commented Jun 12, 2020

Yes I agree - I did think about that when I did it. I've pushed that change

@elstoc
Copy link
Contributor Author

elstoc commented Jun 12, 2020

I'm still not convinced that any of the modules that may be switched off should be auto-enabled via the module->default_enabled flag due to the problems I've mentioned in my issue (namely that if one can disable the module one should also be able to remove it from the stack) but given how close we are to feature freeze I didn't think it was worth opening that can of worms yet. IMO that flag should really be limited to just those modules that are fully mandatory.

@TurboGit
Copy link
Member

but given how close we are to feature freeze I didn't think it was worth opening that can of worms yet. IMO that flag should really be limited to just those modules that are fully mandatory.

Agreed on both counts !

@elstoc
Copy link
Contributor Author

elstoc commented Jun 12, 2020

Perhaps I'll raise another FR to discuss for 3.4

@TurboGit
Copy link
Member

Perhaps I'll raise another FR to discuss for 3.4

Would be perfect !

@TurboGit
Copy link
Member

Thanks for working on this. Better initialization indeed.

@TurboGit TurboGit merged commit b2aadd4 into darktable-org:master Jun 12, 2020
@elstoc
Copy link
Contributor Author

elstoc commented Jun 12, 2020

FR as discussed is #5434.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bugfix pull request fixing a bug feature: enhancement current features to improve priority: medium core features are degraded in a way that is still mostly usable, software stutters scope: codebase making darktable source code easier to manage
Projects
None yet
Development

Successfully merging this pull request may close these issues.

History stack strangeness
2 participants