Cuelist slider fix #1396
Cuelist slider fix #1396
Conversation
|
I'm not really convinced by those +/- 0.00001 |
|
Yes, I'll try to pack it up tonight after work. And maybe throw a quick video together to walk through it. For now I'll try explaining with words, in case that's enough. I was testing with a super-simple workspace - 1 scene dropped into a chaser 6 times, and attached to the cuelist. Also tested with it in the chaser 18 times. I watched the debug closely (I actually changed it to qWarning temporarily so I could ignore all the other debug output). And then I used the NextCue/PrevCue buttons and keyboard arrows (up/down) to move the steps and slider. With 6 steps, each step gets 42 2/3 of the 256 DMX values (42.6666666667). First step starts at 255 and ends at 212.333333, so it "owns" the range from 255 down to 213; in other words, any value between 213 and 255 inclusive should equate to step # 1. I picked 0.00001 because floats keep 6-8 digits of precision, and the smallest # of DMX values assigned to one step is 256/255 = 1.00392. So the 0.00001 is large enough to affect the floating point numbers, but small enough to never push any one step's range over an integer boundary that it shouldn't (at least, that's my belief). |
|
p.s. I was thinking I was dealing with floating point rounding errors when I was testing the code change. I thought the value that was supposed to be an exact integer was somehow looking like it was slightly less than the integer value. But after responding to your question above, I realize that the real issue I'm solving is that an integer value comes out as the same number if you apply qCeil or qFloor, but to make sure each step owns a specific range of DMX values I needed all the possible dividing lines to return one value after qCeil() and a different value after qFloor()... So, the comment in the code I checked-in is misleading; I'll try to fix that. I'll still get you a workspace and a few pictures or a video tonight. |
Attaching a zip, containing the test qxw plus a series of PNG's to try and illustrate what I was seeing. |
kpr0th
force-pushed
the
cuelist-slider-fix
branch
from
ee5c509
to
0deb650
Compare
kpr0th
force-pushed
the
cuelist-slider-fix
branch
3 times, most recently
from
98af807
to
0e2a37f
Compare
When the Step # changed, or the SliderValue changed, in Steps mode, the calculation to convert step# to value or vice versa had some off-by-one issues. The main visible symptom was that the slider position didn't always update when clicking the PreviousCue button (in a 6-step chaser, it would update the slider position every-other-click). Also, if moving the slider position using up/down arrows, the step # didn't change at the exact same slider positions as were jumped to using the Next-Cue button. In v5 this is perhaps not necessary, because the slider position doesn't update if the Current Step changes; but if code is ever added to set the slider position on current step changing the same fix for the same reason is applicable here too.
kpr0th
force-pushed
the
cuelist-slider-fix
branch
from
c129a73
to
ad36afb
Compare
When the Step # changed, or the SliderValue changed, in Steps mode, the calculation to convert step# to value or vice versa had some off-by-one issues. The main visible symptom was that the slider position didn't always update when clicking the PreviousCue button (in a 6-step chaser, it would update the slider position every-other-click). Also, if moving the slider position using up/down arrows, the step # didn't change at the exact same slider positions as were jumped to using the Next-Cue button. In v5 this is perhaps not necessary, because the slider position doesn't update if the Current Step changes; but if code is ever added to set the slider position on current step changing the same fix for the same reason is applicable here too.
…into cuelist-slider-fix
@mcallegari: I finally took some time to get back to this. I setup a bit of quick test code to run all possible values and look for corner cases. In the process I realized there were a couple of other 255's that should have been 256's. And I found a few more corner cases that I wasn't looking at before. By rounding off the "stepSize" variable to 5 decimals I was able to eliminate all of them. So I removed the +/- 0.00001's, rebased on my feature branch, and now I'm confident this is a solid fix without the hack. Will you please re-consider this pull request? (p.s. if I somehow messed up the re-basing, let me know and I can re-create my change on a clean branch instead -- I'm not all that experienced with git yet) (p.p.s. sorry about the multiple "Checks" runs; I didn't realize I could take the PR back to a draft state until after I was done testing, re-testing etc and creating multiple commits along the way...) |
|
@mcallegari : you had asked for info on the issue I was trying to fix. I posted a zip file back on Feb 22 with pics showing the issue in the upstream/master, along with how my fix adjusted it. I just checked, and the original issue still exists in the latest upstream codebase. My updated commit still fixes it. In case the pictures aren't making the issue obvious enough, you can reproduce the problem using the Feb 22 workspace in v4: Go online with the virtual console Expected behavior: side slider moves every time previous or next cue is pressed, and the number at the top of the slider also changes. Another symptom of the issue: Go online with the virtual console Expected behavior: since 42 is the top of the 2nd cue's range, sending a 42 should stay on cue 2. |
qmlui/virtualconsole/vccuelist.cpp
Outdated
| float stepSize = 255 / float(ch->stepsCount()); | ||
| if (level >= 255 - stepSize) | ||
| float stepSize = 256 / float(ch->stepsCount()); //divide up the full 0..255 range | ||
| stepSize = (float)qFloor((stepSize * 100000) + 0.5) / 100000; //round to 5 decimals to fix corner cases |
There was a problem hiding this comment.
stepSize is already float, so no need to cast it to float again
There was a problem hiding this comment.
Yep -- but the result of calling qFloor is an int, and the resulting stepSize was getting truncated. But per your request below, I changed "100000" to "100000.0" and removed the now-unnecessary casts to float.
ui/src/virtualconsole/vccuelist.cpp
Outdated
| if (stepsCount < 256) | ||
| { | ||
| stepVal = 256.0 / (float)stepsCount; //divide up the full 0..255 range | ||
| stepVal = (float)qFloor((stepVal * 100000) + 0.5) / 100000; //round to 5 decimals to fix corner cases |
There was a problem hiding this comment.
same as above. Cast not needed
ui/src/virtualconsole/vccuelist.cpp
Outdated
| int lowerBound = qFloor(upperBound - stepVal); | ||
| //qDebug() << "Slider value:" << m_slider1->value() << "Step range:" << (255 - slValue) << (255 - slValue - stepVal); | ||
| int upperBound = 255 - qCeil(slValue); | ||
| int lowerBound = qFloor((float)256.0 - slValue - stepVal); |
There was a problem hiding this comment.
I think casting to float is not needed here too since you specified .0 which the compiler interprets as float
There was a problem hiding this comment.
Tested successfully without the (float). Removed in new commit.
ui/src/virtualconsole/vccuelist.cpp
Outdated
| float stepSize = 255.0 / (float)ch->stepsCount(); | ||
| if (value >= 255.0 - stepSize) | ||
| float stepSize = 256.0 / (float)ch->stepsCount(); //divide up the full 0..255 range | ||
| stepSize = (float)qFloor((stepSize * 100000) + 0.5) / 100000; //round to 5 decimals to fix corner cases |
There was a problem hiding this comment.
Cast not needed. Explicitly use 100000.0
There was a problem hiding this comment.
Tested this as per your recommendation and it works fine that way. Cast removed.
Fixed another formatting issue (space after if). Eliminated some casts to float by adding decimals to constants.
|
Looks good now. Thanks 👍 |
Adjust CueList slider value calculation to fully align ValueChanged->Step# and Step#Changed->Value calculations.
Previously, clicking the PreviousStep button didn't always update the slider position, due to off-by-one issues near whole-numbered calculated values. Example: with a 6-step chaser, advance to the end, then click the Previous Cue button and note that the slider only jumps up every other click. Also, if you move the slider position with keystrokes up and down (or presumably also via a midi encoder wheel up/down), note that it jumps to the next step at slightly different values than the ones that are calculated when using next/previous step buttons -- and then the displayed value at the top of the slider "jumps" to the top end of the subset of values calculated to belong to the newly selected chaser step #, which causes the visual feedback to "jump".