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

Remove draw limiting from the ProgressBar state #380

Merged
merged 1 commit into from Feb 18, 2022
Merged

Conversation

djc
Copy link
Collaborator

@djc djc commented Feb 17, 2022

We now have rate limiting in the draw target, which is takes care
of most of the costs of drawing. Limiting earlier is not as
helpful in terms of performance, and reduces accuracy.

@djc djc mentioned this pull request Feb 17, 2022
@chris-laplante
Copy link
Collaborator

I think ProgressState::update needs to check more than just pos to determine whether or not the draw should occur. For example I think ProgressBar::tick() broke because the pos didn't change, but tick did.

@djc
Copy link
Collaborator Author

djc commented Feb 17, 2022

What about if we just always update?

@chris-laplante
Copy link
Collaborator

chris-laplante commented Feb 17, 2022

What about if we just always update?

I feel like there is still a hole in the design. LeakyBucket should (IMHO) only come into play when pos and/or tick change. Any other change to ProgressStyle (such as changing the message, prefix, style, e.g.) or hiding/showing the progress bar should not be subject to rate limiting since they are important visual changes.

Granted, I have not encountered this issue, but I think they exist in the code.

@chris-laplante
Copy link
Collaborator

However in the meantime, I think this change is good

@djc
Copy link
Collaborator Author

djc commented Feb 18, 2022

I feel like there is still a hole in the design. LeakyBucket should (IMHO) only come into play when pos and/or tick change. Any other change to ProgressStyle (such as changing the message, prefix, style, e.g.) or hiding/showing the progress bar should not be subject to rate limiting since they are important visual changes.

I think this is already how it works today. The condition for returning a Some(Drawable::Term { .. }) for ProgressDrawTargetKind::Term is force_draw || has_capacity (where has_capacity is the outcome of the LeakyBucket limiter) -- and force_draw is what most things like changing the message/prefix/style are already using.

We now have rate limiting in the draw target, which is takes care
of most of the costs of drawing. Limiting earlier is not as
helpful in terms of performance, and reduces accuracy.
@djc djc merged commit 8946dea into main Feb 18, 2022
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.

None yet

2 participants