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

refactor: more principled optimization animation support in @penrose/editor #1017

Merged
merged 4 commits into from
May 25, 2022

Conversation

wodeni
Copy link
Member

@wodeni wodeni commented May 25, 2022

Description

Related issue/PR: #1000

#1000 re-introduced optimization animation in @penrose/editor by allowing "step size" configuration. However, because we handled repeated stepping by a simple while loop, there can be race conditions when the user press "resample" multiple times, i.e. the frames from the previous state might get rendered.

In general, the optimizer for a given diagram should stop running in the browser upon re-compilation and resample, but the animation is only possible if we call stepState and RenderStatic sequentially. To solve this problem, this PR uses (1) Recoil to manage the state transition between diagram states and (2) requestAnimationFrame to avoid too many recursive state updates in React and to ask the browser to render each frame. Importantly, we also call cancelAnimationFrame to make sure that there is only one pending animation request (source). Otherwise, there can be race conditions among animation frames just as before.

Implementation strategy and design decisions

  • Implemented state management and animation frames described above
  • Fixed errors on startup about roger if editor is running locally

Examples with steps to reproduce them

ide-animation

Checklist

  • I have commented my code, particularly in hard-to-understand areas
  • My changes generate no new ESLint warnings
  • I have reviewed any generated changes to the diagrams/ folder

Open questions

N/A

@wodeni wodeni marked this pull request as draft May 25, 2022 04:44
@codecov
Copy link

codecov bot commented May 25, 2022

Codecov Report

Merging #1017 (7be59c8) into main (3e7f647) will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##             main    #1017   +/-   ##
=======================================
  Coverage   63.57%   63.57%           
=======================================
  Files          62       62           
  Lines        7810     7810           
  Branches     1786     1786           
=======================================
  Hits         4965     4965           
  Misses       2729     2729           
  Partials      116      116           

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 3e7f647...7be59c8. Read the comment docs.

@cloudflare-pages
Copy link

cloudflare-pages bot commented May 25, 2022

Deploying with  Cloudflare Pages  Cloudflare Pages

Latest commit: 7be59c8
Status: ✅  Deploy successful!
Preview URL: https://07dc5d07.penrose-72l.pages.dev

View logs

@wodeni wodeni changed the title refactor: more efficient optimization animation support in @penrose/editor refactor: more principled optimization animation support in @penrose/editor May 25, 2022
@wodeni wodeni marked this pull request as ready for review May 25, 2022 06:09
@wodeni wodeni self-assigned this May 25, 2022
@wodeni wodeni merged commit a65b6af into main May 25, 2022
@wodeni wodeni deleted the ide-animate branch May 25, 2022 07:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

1 participant