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

exposure: rename the black setting #1972

Merged
merged 2 commits into from Jan 8, 2019

Conversation

Projects
None yet
4 participants
@aurelienpierre
Copy link
Contributor

aurelienpierre commented Jan 5, 2019

Users have taken the poor habit to use the black parameter to add more density in blacks. This is not the place to do it (use levels instead) and can seriously harm colors by pushing near-black RGB values into negatives, thus out of gamut: https://discuss.pixls.us/t/darktable-filmic-and-saturation/10622/11

We rename the "black" parameter "unclip negative values" to make its technical purpose clear and add a warning tooltip.

This setting should be let alone most of the time, except if the raw black level is wrong.

@upegelow

This comment has been minimized.

Copy link
Member

upegelow commented Jan 5, 2019

I am fine with warning the user in the tooltip to be cautious, but I am against the name change. "Unclip negative values" while maybe technically correct is obscure from a user perspective.

@TurboGit

This comment has been minimized.

Copy link
Member

TurboGit commented Jan 5, 2019

@aurelienpierre : I'm leaning toward upegelow point of view. I would prefer to see the tooltip changed only. I understand that dt is for advanced users, but most of them won't understand "unclip negative values"... And I'm one of those :)

@TurboGit TurboGit self-requested a review Jan 5, 2019

@TurboGit TurboGit self-assigned this Jan 5, 2019

@TurboGit TurboGit added the enhancement label Jan 5, 2019

@TurboGit TurboGit added this to the 2.8 milestone Jan 5, 2019

@aurelienpierre

This comment has been minimized.

Copy link
Contributor Author

aurelienpierre commented Jan 5, 2019

is obscure from a user perspective.

But that is exactly the purpose: obfuscate the name so people will need to read at least the tooltip, if not the doc, before they start messing up things without knowing it.

"black" suggests it's you average friendly slider, that can help you get more/less density, and in that sense, it's redundant with the levels module, except this variant can backfire badly.

"unclip negative values" suggests "don't use that unless it's to fix an issue and you know what you are doing".

The goal is actually to prevent most users to use it, because there are safer way to do the same thing in the software, and the fact that most modules deal silently with negative values makes it super difficult to track down the clipping issues to the exposure module when you don't know the guts of dt.

I have had multiple users reporting that either filmic or colorbalance clipped lowlights, and when they sent me the XMP, they all had their black level cranked like crazy, outputting negative values that nor log or gamma functions know to handle. So the problem they created before in exposure showed later in other modules. And now that this bad habit has been taken, it needs harsh measures to be reversed.

@TurboGit

This comment has been minimized.

Copy link
Member

TurboGit commented Jan 6, 2019

@aurelienpierre : I would see a warning in the tooltip in this case as it is done for the sliders in the "target/display" section of filmic. We could also rename "black" to "black level" if this helps? With is an indication that we are talking a level and not a simple adjustment of the dark tones. Hope this compromise will be ok to you.

@aurelienpierre

This comment has been minimized.

Copy link
Contributor Author

aurelienpierre commented Jan 6, 2019

Could "black/noise level" be an option ?

@TurboGit

This comment has been minimized.

Copy link
Member

TurboGit commented Jan 7, 2019

@aurelienpierre : to me yes.

@upegelow

This comment has been minimized.

Copy link
Member

upegelow commented Jan 7, 2019

Could "black/noise level" be an option ?

I don't like this. It follows the same idea of obfuscating the parameter. Even worse, some users might think that this is a denoise feature. As said I am fine to change the tooltip.

@aurelienpierre

This comment has been minimized.

Copy link
Contributor Author

aurelienpierre commented Jan 7, 2019

So "black level correction" then ?

@TurboGit

This comment has been minimized.

Copy link
Member

TurboGit commented Jan 7, 2019

To me it sounds good. That's clear, precise and should be clear to anyone. But please also makes the necessary comments in the tooltip as discussed above.

Show resolved Hide resolved src/iop/exposure.c Outdated
@upegelow

This comment has been minimized.

Copy link
Member

upegelow commented Jan 7, 2019

Fine for me as well.

@aurelienpierre

This comment has been minimized.

Copy link
Contributor Author

aurelienpierre commented Jan 7, 2019

Good! I'll make the change

aurelienpierre added some commits Jan 5, 2019

exposure: rename the black setting
Users have taken the poor habit to use the black parameter to add more density in blacks. This is not the place to do it (use levels instead) and can seriously harm colors by pushing near-black RGB values into negatives, thus out of gamut: https://discuss.pixls.us/t/darktable-filmic-and-saturation/10622/11

We rename the "black" parameter "unclip negative values" to make its technical purpose clear and add a warning tooltip.

@aurelienpierre aurelienpierre force-pushed the aurelienpierre:exposure-warning branch from 8d0a623 to 475255d Jan 7, 2019

@TurboGit

This comment has been minimized.

Copy link
Member

TurboGit commented Jan 8, 2019

Thanks, sounds good to me!

@TurboGit TurboGit merged commit 7045231 into darktable-org:master Jan 8, 2019

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

TurboGit added a commit that referenced this pull request Jan 16, 2019

exposure: rename the black setting (#1972)
* exposure: rename the black setting

Users have taken the poor habit to use the black parameter to add more density in blacks. This is not the place to do it (use levels instead) and can seriously harm colors by pushing near-black RGB values into negatives, thus out of gamut: https://discuss.pixls.us/t/darktable-filmic-and-saturation/10622/11

We rename the "black" parameter "unclip negative values" to make its technical purpose clear and add a warning tooltip.

* change the black setting name to "black level correction", remove a space

phweyland added a commit to phweyland/darktable that referenced this pull request Feb 4, 2019

exposure: rename the black setting (darktable-org#1972)
* exposure: rename the black setting

Users have taken the poor habit to use the black parameter to add more density in blacks. This is not the place to do it (use levels instead) and can seriously harm colors by pushing near-black RGB values into negatives, thus out of gamut: https://discuss.pixls.us/t/darktable-filmic-and-saturation/10622/11

We rename the "black" parameter "unclip negative values" to make its technical purpose clear and add a warning tooltip.

* change the black setting name to "black level correction", remove a space
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.