Configurable update interval #153
Conversation
|
Hmm, actually, I was planning to remove this interval because it feels buggy. The goal of the throttle function is to prevent having too many update successively. Indeed, there is the |
|
Also if you use '@' binding, interval will be a string... Moreover, if interval is 0 then 0 || 350 === 350 and this is not what you want. |
|
It makes sense to limit successive updates in most cases, but when users need the slider to update from changing its model in real time, I feel like it makes sense to let them set a custom interval. My personal use case is using it as a zoom slider within a webgl application, and I need the slider to react when the camera zooms via pinch. With this modification and the interval set to 16, it works perfectly. I see where you're coming from on |
|
Would I also didn't consider an interval of 0, thanks for catching that :) |
|
If I have understood correctly the library, the interval is only used when the model and high values are modified from outside the slider. Otherwise, the slider updates instantaneously on slide. So, I would remove the leading parameter (defaults to true) and keep 350 as interval since it's only used for the next updates. |
|
That's true, but doing that still leaves the slider updating every 350ms if its model or high values are modified from outside the slider. In my own personal use case, that was far too slow, as I needed the slider to react in what feels like real time. Allowing the throttle to be configurable helps. |
|
Do you need to update it several times within 350ms? Or you just want the update to be taken into account without any delay? |
|
I need it to update every frame, so yes, several times within 350ms. Not without any delay (introducing unnecessary performance issues), but without any perceptible delay. I thought about rewriting the throttle to optionally allow for throttling based on request animation frame, but I figured that letting users set the throttle interval to 16ms would give 95% of the benefit without overly modifying the codebase. |
|
Just to be sure I understand well: your webGL app needs to update the slider value at every frame or the slider needs to update a webGL value at each frame? |
|
Haha, sorry I'm not being totally clear. It's actually both. It needs to feed input into the scene (works perfectly), and needs to react to changes in the scene (works less than perfectly). The slider controls the camera's zoom. When you slide the slider in and out, the camera moves in and out. But it also needs to react if a user changes the camera's zoom with scrolling or pinching. This is where I hit a snag with the slider only updating every 350ms, as the slider wasn't smoothly reacting to its model changing when a user interacted with the scene. Changing 350ms to 16ms (approximating 60fps), resolved the issue, and the slider reacted without any perceptible delay. |
|
OK now I understand! :) So indeed, you need to change the interval! :) About the '@' or '=' and my remark about 0 || 350, I tried you code and it works but the interval is set as a string and since JS is magic (sometimes too much), I would use '=?' to ensure we get an int and then it should be: this.interval = this.scope.rzSliderInterval != null ? this.scope.rzSliderInterval : 350; |
|
I think we can use this PR to remove the leading:false parameter, can you remove it?. |
|
Yep 100%! Updated commit inbound |
|
Should be all set :) |
Configurable update interval
|
Thanks |
|
Oh, one last thing! I've not updated a package with bower or npm before. Is that your responsibility as the repo owner or my responsibility as the PR submittee? |
|
I can do it. Maybe I'll wait some times to see if there are new PRs or Le jeu. 15 oct. 2015 20:28, Eli Fitch [email protected] a écrit :
|
|
According to the gospel of semantic versioning http://semver.org/ it could probably qualify as a patch, so v. 1.0.1 |
|
I think this is a new feature so a minor release, so should be 1.1.0. |
I added an attribute,
rz-slider-interval, that allows users to set how often they want the slider to update, rather than leaving it fixed at 350ms. It still defaults to 350 if that attribute isn't set.