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

po/rework workflow & module groups #13234

Merged
merged 2 commits into from
Jan 16, 2023

Conversation

TurboGit
Copy link
Member

@TurboGit TurboGit commented Dec 30, 2022

  1. Rework the workflow preference to be simpler.

    The new values are now:

    • scene-refrerred (filmic)
    • scene-referred (sigmoid)
    • display-referred (legacy)
    • none

    The chromatic-adaptation is implied by the workflow setting (removing
    one preference). And is "modern" for all scene-referred workflow.

  2. Rework the module groups to be simpler.

    • Remove the "default" group.
    • Add either Sigmoid or FilmicRGB module based on current workflow
    • Use Sigmoid instead of FilmicRGB for the beginner workflow.

Fixes #12917.

@TurboGit TurboGit added this to the 4.4 milestone Dec 30, 2022
@TurboGit TurboGit self-assigned this Dec 30, 2022
@TurboGit TurboGit added priority: high core features are broken and not usable at all, software crashes feature: redesign current features to rewrite scope: UI user interface and interactions scope: image processing correcting pixels documentation-pending a documentation work is required release notes: pending labels Dec 30, 2022
@TurboGit TurboGit requested a review from AlicVB December 30, 2022 11:42
@Nilvus
Copy link
Contributor

Nilvus commented Dec 31, 2022

Good move but remains one thing not good with your PR. If you don't set any workflow on preferences, then on modulegroups, you only have scene-referred workflow with sigmoid and no more filmic. Some users (like me) will still prefer filmic to sigmoid and if no workflow set on preferences, modulegroups should give both choices: with sigmoid or filmic.

@TurboGit
Copy link
Member Author

Indeed, so if workflow is set to None we probably want both Sigmoid & Filmic to be in the scene-referred module group.

@TurboGit
Copy link
Member Author

@Nilvus : Should be fixed now.

@Nilvus
Copy link
Contributor

Nilvus commented Dec 31, 2022

Good. Thanks. I'm just wondering if it wouldn't be better to have 2 workflow on modulegroups for scene-referred. One with sigmoid and other with filmic, instead of one with both modules. filmic and sigmoid do not have to be used together.

@jandren
Copy link
Contributor

jandren commented Dec 31, 2022

I had liked to see the selection of display transform and the chosen module order to be decoupled if possible!

So in preferences:
Module order

  • Display Referred
  • Scene Referred

Auto apply display transform

  • Base Curve
  • Base Curve "camera" (the current auto apply check box)
  • Filmic
  • Sigmoid

How do you feel about that?

@TurboGit
Copy link
Member Author

TurboGit commented Jan 1, 2023

Not sure it makes lot of sense to have Display Referred & Filmic (or Sigmoid). This was discussed here btw, #12917.

So it feels better to me to have safe preferences and the choice None for people wanting to create their own workflow using auto-applied presets.

@Nilvus
Copy link
Contributor

Nilvus commented Jan 1, 2023

Ok

Copy link
Contributor

@AlicVB AlicVB left a comment

Choose a reason for hiding this comment

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

first, very quick test :

  • at startup , when opening preferences, I have :
    image
    See the "empty" combobox... I think it's due to the fact that my previous setting is no longer in the list

Also, I wonder if the "auto-apply per camera basecurve presets" couldn't be squashed inside our new preference...

Another point : I think we should follow the preference setting also for the beginner workflow.

I'll test again and review the code later ;)

@TurboGit
Copy link
Member Author

TurboGit commented Jan 2, 2023

See the "empty" combobox... I think it's due to the fact that my previous setting is no longer in the list

This is a separate bug already fixed on master (and 4.2 branch) 82963e7. Let me rebase this branch.

The new values are:

  - scene-refrerred (filmic)
  - scene-referred (sigmoid)
  - display-referred (legacy)
  - none

The chromatic-adaptation is implied by the workflow setting (removing
one preference). And is "modern" for all scene-referred workflow.

For darktable-org#12917.
- Remove the "default" group.
- Add either Sigmoid or FilmicRGB module based on current workflow
- If workflow is none, both Sigmoid & FilmicRGB are displayed
- Use Sigmoid instead of FilmicRGB for the beginner workflow.

Fixes darktable-org#12917.
@TurboGit TurboGit force-pushed the po/rework-workflow branch from a50979f to d2cff67 Compare January 2, 2023 22:07
@TurboGit
Copy link
Member Author

TurboGit commented Jan 3, 2023

Also, I wonder if the "auto-apply per camera basecurve presets" couldn't be squashed inside our new preference...

You mean having two display referred entries as workflows:

  • display referred
  • display referred (camera base curve)

(no more legacy in the name for keep the label short)

I have no strong opinion, not sure what is better/simpler/clearer there.

@AlicVB
Copy link
Contributor

AlicVB commented Jan 3, 2023

You mean having two display referred entries as workflows:

either this or just one workflow for display referred and we choose the most directly usable one (I'm not sure as it was a long time since I've switch to scene referred, but iirc camera base curve was quite ok for a first starting point)

That would remove one pref item, not needed in most cases as scene referred is the default nowdays... In fact we should even only show this pref for display referred workflows (but that would means lot of code change in preference.c for such a corner case)

@TurboGit
Copy link
Member Author

TurboGit commented Jan 3, 2023

either this or just one workflow for display referred and we choose the most directly usable one (I'm not sure as it was a long time since I've switch to scene referred, but iirc camera base curve was quite ok for a first starting point)

Sadly I'm not sure that's true for all camera. But I have left the display referred workflow to assess the current status.

@TurboGit TurboGit merged commit ca1ab42 into darktable-org:master Jan 16, 2023
@TurboGit
Copy link
Member Author

Merged, we can adjust the camera specific curve later if needed.

@TurboGit TurboGit deleted the po/rework-workflow branch January 17, 2023 08:39
@elstoc elstoc added documentation-complete needed documentation is merged in dtdocs and removed documentation-pending a documentation work is required labels Jul 9, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
documentation-complete needed documentation is merged in dtdocs feature: redesign current features to rewrite priority: high core features are broken and not usable at all, software crashes scope: image processing correcting pixels scope: UI user interface and interactions
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[RFC] : rework / simplify the workflows support
5 participants