fix: Don't continue playing when a model is reset #1796
Conversation
Codecov ReportPatch coverage has no change and project coverage change:
Additional details and impacted files@@ Coverage Diff @@
## main #1796 +/- ##
==========================================
- Coverage 81.79% 81.68% -0.12%
==========================================
Files 15 15
Lines 890 890
Branches 191 192 +1
==========================================
- Hits 728 727 -1
- Misses 138 139 +1
Partials 24 24
☔ View full report in Codecov by Sentry. |
298788e to
971edab
Compare
| # clicking the reset button or changing the parameters. If previous_step > | ||
| # current_step, it means a model reset happens while the simulation is | ||
| # still playing. | ||
| previous_step = solara.use_reactive(0) |
There was a problem hiding this comment.
Have you seen the use previous hook? Might be usable here
There was a problem hiding this comment.
I just tried, but with that, the simulation will always step once after the reset. Not sure why. At least this PR works, and the code is not that much different from if it were using use_previous.
There was a problem hiding this comment.
It's still a bit verbose. The step+1 got me curious and I was playing around a bit.
I think a simpler solution would be to use use_previous on the model and then look if the previous model is equal to the current model. If it's not set playing to False and reset current step.
There was a problem hiding this comment.
It's problematic to store 2 copies of model with different specs at any given time. Consumes 2x RAM.
971edab to
712b1d1
Compare
|
@tpike3 @jackiekazil can we disable the blocking the out-of-date branch? It is not necessary. |
|
@rht not sure I understand.... what branch that is out of date is being blocked? and what are you trying to do? |
|
This screenshot:
|
|
Thank you for the clarification. It is updated now. Let me know if anything else needs to be done. |
|
Can this PR be merged first for the upcoming release that is by the end of this month? It fixes a very problematic autoplay bug. There is no obvious solution regarding with |
|
|
||
| def on_value_play(change): | ||
| if model.running: |
There was a problem hiding this comment.
We should check for model.running and playing.value. That way you won't see any unwanted step+1 on reset
|
Sure, no this was never blocking |
Fixes #1782.