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

Add ratelimit to Param updates #190

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

EmmaSimon
Copy link
Contributor

Description

Add a rate limit to the update calls for UI text inputs.
This allows the user to quickly change inputs quickly
(like holding up or down), and instead of trying to draw
each value change, updates will only be sent every .3s.

It might make sense to make the limit period longer
or configurable, but this combination felt good
in my testing.

Checklist

  • feature/fix implemented
  • mypy returns no error
  • tests added/updated and pytest --runslow succeeds
  • documentation added/updated and building with no error (make clean && make html in docs/)
  • examples added/updated
  • code formatting ok (black and isort)

@abey79
Copy link
Owner

abey79 commented Dec 29, 2021

I haven't tested this one yet, but, if I understand correctly, two param changes within the period would lead to only the first param to go through and be displayed, leading to a discrepancy between UI and display. Or am I missing something?

The idea is good though. Maybe we should have a decorator that delays the function execution and let it through only if no other call is received within the window.

@EmmaSimon
Copy link
Contributor Author

I haven't tested this one yet, but, if I understand correctly, two param changes within the period would lead to only the first param to go through and be displayed, leading to a discrepancy between UI and display. Or am I missing something?

The idea is good though. Maybe we should have a decorator that delays the function execution and let it through only if no other call is received within the window.

Ahhh, yeah I think I see what you mean. Let me play around with it later today.

Param updates from UI text inputs are now sent every .3s.
This allows the user to quickly change inputs quickly
(like holding up or down), and instead of trying to draw
each value change, updates will only be sent periodically.

It might make sense to make the limit period longer
or configurable, but this felt good in my testing.
@EmmaSimon
Copy link
Contributor Author

Ok @abey79, ready for another pass now. Got rid of the ratelimit dependency and instead stole this decorator from the comments in a gist and tweaked it a little bit to make sure the final value is always called at the end.

@abey79
Copy link
Owner

abey79 commented Dec 30, 2021

This is looking better indeed 👍🏻 Two points though.

First, if I understand that logic logic we have:

  • single click: immediate execution
  • double click: first action is immediately executed, the second one is executed with a delay w.r.t. the first click
  • triple+ click: first action is immediately executed, all further actions are dropped, except the last which is executed with a delay w.r.t the one-but-last click

Did I get that correctly? If so, this is nice for the single-click scenario (no delay), but yield a "spurious" execution in all other scenarii. I'm wondering if an alternate scheme where a delay is applied to all clicks wouldn't be better. Single click would have a delayed execution (is that annoying?) but multi-click would have no spurious executions.

I assume this would do the trick (untested):

def delay_throttle(wait):
    """Decorator that will postpone a functions
    execution until after wait seconds
    have elapsed since the last time it was invoked."""

    def decorator(fn):
        def throttled(*args, **kwargs):
            def call_it():
                throttled._timer = None
                return fn(*args, **kwargs)

            if throttled._timer:
                throttled._timer.cancel()
            throttled._timer = threading.Timer(wait, call_it)
            throttled._timer.start()

        throttled._timer = None
        return throttled

    return decorator

Care to give it a spin and see how it feels?

Second point is that the throttled function is executed in a separate thread and I have a hard time wrapping my head around this being a potential issue or not. Admittedly, the throttled function consists of just emitting a signal, but doing so from a different thread from where the object "lives". It appears that this may not be an issue, if I read this correctly. Maybe the same could be implemented with a QTimer instead, just to make sure?

@abey79
Copy link
Owner

abey79 commented Dec 30, 2021

Minor third point: def throttled() should probably be decorated with @functools.wraps(fn) as is good practice in such instance.

@EmmaSimon
Copy link
Contributor Author

EmmaSimon commented Dec 30, 2021

I won't be able to try this out until after work tonight, but just to respond to a few points,

  • Here's a little demo for throttling and debouncing. In theory this decorator should work like the throttle one, the first call happens immediately if it hasn't been called within the time frame, then if the input is changed continuously, it will continue to be fired every x seconds, until the input stops, and the final call is executed after the delay time after the previous time the function was called, not the time from the previous click. I wanted the function to be throttled and not debounced so that you can watch the sketch change as the value changes, the extra executions don't make sense if you're only doing 2-3 clicks or so, but when you're just holding up on a number field, you probably want to watch the sketch move. That said, I don't think an initial delay would be noticable, let me see how it feels. I think single click is still going to be the most common input though, so definitely don't want to make things feel weird for people doing that.
  • Good point about the thread stuff, I'll do a little more research. I think I saw some people saying that it was inappropriate to use a regular Timer but I don't totally understand why.
  • Thanks, I'll add that! I don't really do any "real" work with python, so I'm not as familiar as I'd like to be with best practices and conventions.

@abey79
Copy link
Owner

abey79 commented Dec 30, 2021

Nice demo -- it's helpful to formalise throttle vs. debounce. I see your point that what we want is throttling such as to have live feedback, so let's stick with this.

For the timer, I don't see why either. Worth trying IMO.

As for the decorator thing, no worries -- I haven't done real work with python either :) Just trying to learn on the way.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants