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

Custom keys on ProgressStyle do not work together with enable_steady_tick #493

Closed
shssoichiro opened this issue Nov 14, 2022 · 9 comments · Fixed by #495
Closed

Custom keys on ProgressStyle do not work together with enable_steady_tick #493

shssoichiro opened this issue Nov 14, 2022 · 9 comments · Fixed by #495

Comments

@shssoichiro
Copy link

It seems as though there are the following two interactions in indicatif 0.17.2, which were not present in 0.17.0-rc4:

  • If set_style is used before enable_steady_tick, the steady tick takes effect, but any custom keys that are set on the ProgressStyle using its with_key method are never updated.
  • If set_style is used after enable_steady_tick, then the custom keys work but the steady tick is disabled.
shssoichiro added a commit to shssoichiro/Av1an that referenced this issue Nov 14, 2022
This fixes an issue where the fps and eta would not update
until the very end of the encode.

See also console-rs/indicatif#493
shssoichiro added a commit to shssoichiro/Av1an that referenced this issue Nov 14, 2022
This fixes an issue where the fps and eta would not update
until the very end of the encode.

See also console-rs/indicatif#493
@djc
Copy link
Collaborator

djc commented Nov 14, 2022

Hmm, I find both of these behaviors quite surprising. Would you be able to run a git bisect to find which commit between -rc.4 and .2 regressed this?

@shssoichiro
Copy link
Author

shssoichiro commented Nov 14, 2022

It appears that 516c0dc is the commit that introduced the breakage. I suppose the possibility should also be considered that it may not be related to custom keys, but may be related to the per_sec and eta values. 🤔

Also to give a clearer example, here are a couple of screenshots.

Expected behavior:
Screenshot_20221114_122606

Buggy behavior:
Screenshot_20221114_122654

mergify bot pushed a commit to master-of-zen/Av1an that referenced this issue Nov 14, 2022
This fixes an issue where the fps and eta would not update
until the very end of the encode.

See also console-rs/indicatif#493
@djc
Copy link
Collaborator

djc commented Nov 15, 2022

Pff, honestly not sure what's going on here. Would you be able to dig in a bit more, since you've already built up some context? Happy to answer further questions about how stuff is supposed to work.

@shssoichiro
Copy link
Author

It seems I've narrowed it down further and it's related to ProgressState::per_sec not updating when steady tick is enabled. If I use the code from #394 (comment) and build a per_sec or eta equivalent using .pos(), .len(), and .elapsed(), it works properly with steady tick.

@aawsome
Copy link
Contributor

aawsome commented Dec 2, 2022

I observed the same issue, namely that {bytes_per_sec} always shows 0 when used with enable_steady_tick.

I think the issues is that the Ticker only calls state.draw() (progress_bar.rs, line 644) whereas
inc() without a ticker calls state.tick() which calls state.update_estimate_and_draw() (state.rs, line 121).

So, the enabled Ticker never updates the Estimator nor any ProgressTracker.

I'm wondering, is there a reason for the Ticker not to simply also call state.update_estimate_and_draw()?

@aawsome
Copy link
Contributor

aawsome commented Dec 2, 2022

#495 worked for me and shows {bytes_per_sec} with a steady ticker enabled.

@aawsome
Copy link
Contributor

aawsome commented Dec 2, 2022

@shssoichiro Would you mind testing if #495 is also able to solve your issue?

@shssoichiro
Copy link
Author

Thanks, it works for me as well. Turns out the issue only affected my custom keys because my custom keys depended on per_sec and eta.

@orhun
Copy link

orhun commented Dec 12, 2022

Having the same issue here.

orhun added a commit to spicylobstergames/SpicyLauncher that referenced this issue Dec 12, 2022
Could not update `indicatif` due to console-rs/indicatif#493
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 a pull request may close this issue.

4 participants