-
-
Notifications
You must be signed in to change notification settings - Fork 4k
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
Propose renaming throttleComponentChanged to throttledDebounce #5144
Comments
Having throttle and debounce in the name is confusing to me. |
I don't think it is that simple. We already have a throttle here And a debounce here Prior to #4980, componentchanged was throttled. This created issues as described in #4972, because in a scenario where there is a rapid burst of changes, the final state may not be relayed. With a debounce, the final state is always relayed reliably, but because a debounce "waits before triggering the function execution, until the events stop happening so rapidly", you can end up with a long period of no updates at all. Consider a case where I have an entity under control of my mouse, and I want it to follow the mouse. I will receive a series of mousemove events. For performance reasons, I may not want to act on every single one of these mouse move events, so I want to throttle or debounce. If I throttle, then I reduce performance load, and the entity follows my mouse pointer, but may not end up in the correct final position, as the final mousemove event may be lost. If I debounce, I also reduce performance load, and the entity is guaranteed to end up in the correct final position. But there are no interim updates as long as I keep moving my mouse pointer. So as long as I keep moving the mouse pointer, I won't get a visual experience of the entity following my mouse. What we often want in this case (and others like it) is a combination of a throttle and a debounce, which provides interim catch-up of state on a timer (like a throttle), but also guarantees correct final state (like a debounce). That's what this function provides. It's the best way to limit the frequency of emissions of componentchanged, and it's useful in a range of other cases too. If you can suggest a better name that's less confusing, I'm happy to hear that. But this is not a throttle or a debounce. It is a distinct form of rate-limiting that combines characteristics of each. Perhaps "throttledDebounce" is misleading because a debounce doesn't need to be throttled - in fact we are increasing the rate of updates vs. a regular debounce. Perhaps its better described as a "debounce with interim updates"? Or as a "convergent throttle"? I'm a little surprised there isn't an established term of art for this method, but from the reading around I have done on the topic, I haven't found one... |
Sure we can give it a more generic name. Not sure if |
Was looking at the lodash links that Vincent provided some more... This function is pretty much equivalent to their throttle, with both "leading" and "trailing" options set. So I'm persuaded this is more like a throttle than a debounce. We could modify the A-Frame utility functions to more closely mirror lodash in terms of configurability. But given the need for back-compatibility etc, I prefer leaving the functions as they are and renaming. Naming this is hard, various suggestions above, but based on what's distinct about this vs. the existing (the "finalizer" being a reference to the specific behaviour that any burst of calls to the function is followed by a final call to the function). |
It makes sense to name it as We could do something similar to lodash and add a flag to I'm leaning towards a |
Agree with not overcomplicating the existing Still finding it hard to come up with a good name. Given primary audience in JS engineers, and given almost all online articles on throttle and debounce lead back to lodash, I think the best name is one that can be immediately understood in terms of its relationship to lodash's throttle implementation. Hence: (with comments / documentation to clarify at slightly more length exactly how it behaves & why that's useful). |
Besides changing the name we can perhaps improve the docs of the function? Not sure if including lodash link might be useful. Always worried those links break or change. |
Fixed by #5151 |
Description:
Since 1.3.0, we have a new utility function:
AFRAME.utils.throttleComponentChanged
This was introduced by PR #4980 to solve #4972
This truns out to be a generically useful utility function, but it doesn't have a generic name.
Recently in an A-Frame app I wanted to do exactly the same thing with the objectChange event from THREE.TransformControls.
But using a utility function called
throttleComponentChanged
to do a debounced throttle on an event calledobjectChange
could have been confusing...!I propose renaming this utility function to a name that reflects it's generic capability, namely to debounce an event in a throttled manner, which puts a cap on the rate at which the event fires, but also guarantees a correct end state.
My suggested name is
AFRAME.utils.throttledDebounce
I'd be happy to do the PR to do all of this, assuming I can get agreement this is a good idea.
I expect we need to keep
AFRAME.utils.throttleComponentChanged
for back compatibility, but it can just be mapped toAFRAME.utils.throttledDebounce
.The text was updated successfully, but these errors were encountered: