Skip to content
This repository has been archived by the owner on Jun 4, 2024. It is now read-only.

variable slider: Add debounce for applying global variable update #7308

Merged
merged 2 commits into from
Jan 16, 2024

Conversation

achim-k
Copy link
Contributor

@achim-k achim-k commented Jan 9, 2024

User-Facing Changes
None

Description
When adjusting the slider value, use a debounced function for applying the new value to the global variable. This will prevent the global variable from changing frequently while the slider is being moved which in turn might prevent some heavy processing depending on how the variable is being used.

Copy link
Contributor

@snosenzo snosenzo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

might be worth mentioning in the user-facing section that the variable slider only changes the value on mouse-up.
Also, did we consider using debounce instead? wondering if that might be a better UX for people who don't necessarily want to mouse up.

Copy link
Contributor

@defunctzombie defunctzombie left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree with @snosenzo that we should give debounce a try. If you don't have a large dataset or if applying the variable change is otherwise a cheap operation it could be a nicer UX to be able to "move" the slider to a value and see the display update while still holding down the mouse to maybe move the slider more.

Whichever method we ultimately choose we should leave a code comment on the tradeoff choice with ux and performance for future maintainers.

packages/studio-base/src/panels/VariableSlider/index.tsx Outdated Show resolved Hide resolved
Copy link
Member

@jtbandes jtbandes left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The original use case for this panel was allowing the user to scrub through a series of predicted time steps while watching the 3D view to see where the robot was at that time. I believe this change would make that workflow a lot worse. That said, I don't know how most folks are using the panel in reality these days.

One option could be to make a setting for it – e.g. continuous: boolean (cf. https://developer.apple.com/documentation/appkit/nscontrol/1428952-iscontinuous)

@defunctzombie
Copy link
Contributor

@jtbandes with a debounce we could still support scrubbing. The thing we want to avoid is triggering updates as you move through the various values on your way towards the one you want (which I think currently happens)?

@achim-k achim-k changed the title variable slider: Only apply changes when committed variable slider: Add debounce for applying global variable update Jan 12, 2024
@defunctzombie defunctzombie merged commit d2dfe39 into main Jan 16, 2024
13 of 14 checks passed
@defunctzombie defunctzombie deleted the achim/variable-slider-commited-changes branch January 16, 2024 20:45
@jtbandes
Copy link
Member

The thing we want to avoid is triggering updates as you move through the various values on your way towards the one you want

This behavior of showing intermediate is useful to make the workflow feel more interactive. We will just have to see if anyone complains after this is shipped.

@defunctzombie
Copy link
Contributor

This behavior of showing intermediate is useful to make the workflow feel more interactive. We will just have to see if anyone complains after this is shipped.

I played with this PR and with the debounce logic it feels "interactive". At 250ms until the change in variable I could scan over to the new variable and I saw it update in the variable sidebar.

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

Successfully merging this pull request may close these issues.

None yet

4 participants