Project

General

Profile

Bug #11594

Bauhaus: step is incorrect after expanding bounds

Added by Roman Lebedev about 2 years ago. Updated about 2 years ago.

Status:
Fixed
Priority:
Medium
Assignee:
Developers
Category:
-
Target version:
Start date:
04/27/2017
Due date:
% Done:

100%

Affected Version:
git master branch
System:
Debian
bitness:
64-bit
hardware architecture:
amd64/x86

Description

0. select some image
1. reset history stack
2. open image in darkroom
3. open exposure iop
4. scroll exposure slider one step. (no modifiers, use mouse scrollwheel)
5. verify that one step changes exposure in 0.02 increments
6. right-click on the slider
7. type 18, press enter
8. reset exposure iop
9. scroll exposure slider one step. (no modifiers, use mouse scrollwheel)
10. one step changes exposure in 0.07 increments
11. ???
12. PROFIT!

i suspect this is a regression from https://github.com/darktable-org/darktable/pull/1463

Associated revisions

Revision 3646e7b9
Added by Dan Torop about 2 years ago

bauhaus: keep slider step constant

Fixes #11594. When slider bounds expand, the scale needs to be
recalculated, as it is proportional to the bounds.

(cherry picked from commit ab9136462f6135d888137739f81c58bb06ee3126)

Revision d623b59c
Added by Dan Torop about 2 years ago

bauhaus: keep slider step constant

Fixes #11594. When slider bounds expand, the scale needs to be
recalculated, as it is proportional to the bounds.

(cherry picked from commit ab9136462f6135d888137739f81c58bb06ee3126)
(cherry picked from commit 3646e7b9c2630af275f83186682ce6aa492a3cf6)

History

#1 Updated by Roman Lebedev about 2 years ago

  • Target version set to 2.4.0

#2 Updated by Dan Torop about 2 years ago

Odd! I'll check this out. Gave it quick glance and can't see something obvious, but there has to be something... More in a bit.

#3 Updated by Dan Torop about 2 years ago

This appears to actually be a longstanding behavior, perhaps even a feature... Nothing to do with PR 1463.

The sliders are given a step size on init (0.02 in the case of the exposure). Then bauhaus, when setting up the new slider (in dt_bauhaus_slider_from_widget()) converts the step to an internal scale factor by multiplying step by a fixed factor of 5 then dividing it by the range of the slider (upper bound minus lower bound). In the case of exposure, the 0.02 step becomes a scale factor of approx. 0.016667 (= 5 * 0.02 / 6).

Internally the slider sees its position within the range [0,1]. On a scroll event, dt_bauhaus_slider_scroll() multiplies the scroll delta by the scale factor (then divides by 5...!), which will give the effect of the requested step once the internal [0,1] range gets converted to whatever the actual range of the slider is (say, [-3,3] for exposure).

When the maximum of the slider goes up to 18, the internal scale value doesn't change. Hence, a scroll's delta has a much greater effect on the slider value, and exposure increment goes up to 0.07.

This may well be the right thing to do in UI terms. If, when the bounds expanded, the internal scale changed to keep the step at 0.02, then a scroll on the slider would suddenly result in a much smaller motion of the slider indicator. In the current implementation, once the step is set (usually at widget init, though potential via dt_bauhaus_slider_set_step()), a scroll on the slider will always result in the same motion of the slider indicator, even if expanding the bounds makes for a much greater change to the slider value.

#4 Updated by Dan Torop about 2 years ago

All that said, https://github.com/darktable-org/darktable/compare/master...dtorop:bauhaus-keep-step?expand=1 should fix this bug, if this seems like an improvement?

There's something else awry in the exposure module, at least for me. Setting the exposure to 18 via the keyboard to expand the bounds actually results in an exposure of 9.965784. This is something to do with exposure_set_white() resetting the exposure?

#5 Updated by Roman Lebedev about 2 years ago

  • Status changed from New to In Progress
  • % Done changed from 0 to 50

@Dan: ok, thank you the patch makes sense according to houz, and seems to work, will push.

And yes, sorry, not a regression from your soft scroll pr.

#6 Updated by Dan Torop about 2 years ago

  • Status changed from In Progress to Fixed
  • % Done changed from 50 to 100

#7 Updated by Dan Torop about 2 years ago

Thanks! It's definitely a judgment call: is it better to keep the scroll step constant or keep consistent the visible rate of change of the slider. Glad houz made a call. As most people won't be expanding slider bounds, it does makes sense to go with the former, which is more useful for "advanced" users...

Also available in: Atom PDF