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

Restore slider digits where lost in introspection changes #5721

Merged
merged 1 commit into from
Jul 8, 2020

Conversation

dterrahe
Copy link
Member

@dterrahe dterrahe commented Jul 8, 2020

Closes #5707

Hopefully this resolves any serious regressions people experience; nothing more jumps out at me in slider_params.xlsx

This should still be revisited as part of a larger input layer review:

  • format&digits should be automatically linked in bauhaus
  • rounding to digits might only be needed for integers
  • stepsize could be a fixed proportion (1/100th?) of the (current) min/max range

But since this will lead to changes people won't like, there needs to be a way to configure granularly (via lua?) which might be relatively easy/free to achieve as part of a new input layer approach.

All way past 3.2, hence this quick fix for the release.

@TurboGit
Copy link
Member

TurboGit commented Jul 8, 2020

This is for 3.2 right? I'm not sure to understand your last sentence (I'm not native English).

@dterrahe
Copy link
Member Author

dterrahe commented Jul 8, 2020

Yes, sorry; I meant the other meanderings about a long term solution/approach are for after 3.2. Therefore the short-term fixes in this PR should go into 3.2 to not cause regressions.

@TurboGit TurboGit added this to the 3.2 milestone Jul 8, 2020
@TurboGit TurboGit added the bugfix pull request fixing a bug label Jul 8, 2020
@TurboGit TurboGit self-requested a review July 8, 2020 17:45
@@ -2347,22 +2347,26 @@ void gui_init(struct dt_iop_module_t *self)
GtkWidget *page2 = self->widget = GTK_WIDGET(gtk_box_new(GTK_ORIENTATION_VERTICAL, 0));

g->cx = dt_bauhaus_slider_from_params(self, "cx");
dt_bauhaus_slider_set_digits(g->cx, 4);
dt_bauhaus_slider_set_factor(g->cx, 100.0);
dt_bauhaus_slider_set_format(g->cx, "%0.f %%");
Copy link
Member

Choose a reason for hiding this comment

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

Don't we need %0.2 here ? Otherwise the values are displayed as plain integer in the interface. Likewise below.

Copy link
Member Author

Choose a reason for hiding this comment

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

Don't need to. There are many places where there are more digits than are visible. I didn't intend to "clean" all of those up, as it may not be a bad thing anyway. As i said, i think we could get rid of rounding to nearest "digit" completely, except for integers.

Copy link
Member

Choose a reason for hiding this comment

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

Ok, let's merge this then. I understand that there is probably many cases like this. I think it makes sense to at least fix this one for the release. I'll issue a PR.

Copy link
Member Author

Choose a reason for hiding this comment

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

To be clear, I didn't consider this a bug and that's why I didn't "fix" it. Rather than showing a lot of digits in the format, the user should just look at the image to see if the cropping is correct.

If you do for some reason want to enforce "consistency" between the number of digits and the format, the cases where they don't match are fairly easy to spot in the spreadsheet I attached. The fixes are all trivial. Again, I didn't do that because I didn't consider it a good idea, but it is up to you.

The number of digits itself (not the format) is important, because the slider is rounding to them, so if not enough digits are set, the user is unable (even invisibly) to get more precision. That's why I submitted this PR to fix such regressions.

Copy link
Member

Choose a reason for hiding this comment

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

If you do for some reason want to enforce "consistency" between the number of digits and the format,

That's not about consistency. What I want is also a fix, on the crop&rotate if you ctrl-scroll on any slider in margin tab, you have no feedback about the change in the value. You need to do 10 changes to see the value go from 0% to 1% for example. I consider this a bug. And this is a regression (due to this PR) as previously the scroll or ctrl-scroll was both changing the value by step of 1%, so always having a visual feedback.

Also, if you edit the slider and enter .5 the value displayed is still 0%. Really a bug to me.

Hope this clarify my thinking.

@TurboGit TurboGit merged commit 536df44 into darktable-org:master Jul 8, 2020
@dterrahe
Copy link
Member Author

dterrahe commented Jul 9, 2020

This list may be helpful.

module		label		step	digits	factor	format
watermark	opacity		1	2	1	%.f%%
watermark	scale		1	2	1	%.f%%
dither		damping		1	3	1	%.0fdB
velvia		strength	1	2	1	%.0f%%
soften		size		1	2	1	%.0f%%
soften		saturation	1	2	1	%.0f%%
soften		mix		1	2	1	%.0f%%
grain		strength	1	2	1	%.0f%%
grain		midtones bias	1	2	1	%.0f%%
highpass	sharpness	1	2	1	%.0f%%
highpass	contrast boost	1	2	1	%.0f%%
relight		width		0.5	2	1	%.1f
levels		black		0.1	2	1	%.1f%%
levels		gray		0.1	2	1	%.1f%%
levels		white		0.1	2	1	%.1f%%
filmic		latitude	0.05	3	1	%.2f EV
colorzones	mix		10	2	1	%.01f%%
bloom		size		1	2	1	%.0f%%
bloom		threshold	1	2	1	%.0f%%
bloom		strength	1	2	1	%.0f%%
vibrance	vibrance	1	2	1	%.0f%%
graduatednd	hardness	1	2	1	%.0f%%
ashift		focal length	1	2	1	%.0fmm
ashift		lens dependence	1	2	1	%.0f%%
exposure	exposure	0.02	3	1	%.2f EV
tonemap		spatial extent	1	2	1	%.0f%%

Only some of those are actual regressions vs 3.0.2.

What I intended to do for 3.4 was to explicitly link digits and format in all cases. Now that only works for the standard format ("%.1f"; setting number of digits also changes the format) but if there needs to be an extension, it needs to be set (again) with a full format ("%.1f%%", "%.2f EV") and it will be overwritten if the number of digits is changed. If instead going forward the format were set with ..._set_format("%%") it could just be concatenated.

Actually, I wanted to do this in dt_bauhaus_slider_get_text with

if(d->min < 0)
return g_strdup_printf("%+.*f%s", d->digits, dt_bauhaus_slider_get_val(w), d->format);
else
return g_strdup_printf("%.*f%s", d->digits, dt_bauhaus_slider_get_val(w), d->format);

The question is, should the number of digits automatically be adjusted if the step size is set/changed. dt_bauhause_slider_from_params does this, but it doesn't take "small" steps (holding CTRL) into account (and is overwritten if an explicit format with extension is set).

Maybe it would be nice to dynamically adjust digits. So for example by default it could be %.0f if min-max=0-100 and step is 1. But then if the user moves a small step (ctrl-scroll) to 0.1 (or enters it directly) an extra digits would be added (and kept until moving to a different picture/reentering darkroom/resetting the slider). Or if a picture is loaded that has a value 0.123 set, digits would be adjusted to 3.

I think this could work well (also when users set a custom multiplication factor for ctrl-scroll). Except for values that are calculated by formula and that would have infinite digits...

@dterrahe
Copy link
Member Author

dterrahe commented Jul 9, 2020

To be clear; this would be a larger refactor again, touching quite a few bits in bauhaus and needing changes in all modules in setting formats/steps/digits. As part of that I would also like to make soft-min/-max user configurable (both by UI and via lua). And this should be based on a revised input layer exposing this functionality, so quite a few dependencies here. I'll of course write all of this up for discussion/commenting before I start.

@TurboGit
Copy link
Member

TurboGit commented Jul 9, 2020

Understood, so maybe we should live with the "issue" I reported until 3.4 is out and close #5724 ?

@dterrahe
Copy link
Member Author

dterrahe commented Jul 9, 2020

Either way. I have no objection to #5724. I was basically just explaining why I hadn't made that change myself. (So that you didn't think I was just lazy ;-) )

But now that the PR is there, you might as well merge it if you think it looks ok.

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
Projects
None yet
Development

Successfully merging this pull request may close these issues.

basic adjustments: black level correction
2 participants