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

feat: render typst/latex/mermaid asynchronously #273

Merged
merged 1 commit into from
Jul 1, 2024

Conversation

mfontanini
Copy link
Owner

This changes rendering of typst/latex/mermaid (now called "third party renders") to execute asynchronously. This wasn't needed for typst/latex as they're so fast you can't even tell it's rendering, but the mermaid CLI requires spinning up a browser (!!!) so it can take a couple of seconds to render. This causes the presentation experience, especially during development, to become very clunky as changing the presentation file just stalls presenterm for 2 seconds per chart until they're all rendered.

This changes that so that charts are rendered asynchronously by a pool of 2 threads (which I will make configurable in another PR). This now means you can:

  • Open a presentation and scroll through slides while typst/latex/mermaid blocks are rendered and you'll see a "Loading..." text as a placeholder until they're loaded.
  • Once loading finishes, the slide will automatically be updated to display the output image.
  • Making a change on one of these blocks will make it render asynchronously so you'd see the same "Loading" text until it does.
  • Because of the cache change on a previous PR, only the modified snippets are re-rendered.
  • Snippets from slides closer to the beginning of the presentation are rendered first (e.g. they're all FIFO'd from first to last slide).
  • Because renders can fail (e.g. mermaid syntax error) the "Loading.." can lead to an error. The error is shown the same way as before, except a bit better because it tells you which slide the error is in.
video.mp4

@mfontanini
Copy link
Owner Author

@mikavilpas I'd appreciate if you could give this a test and see if you find any quirks. The one thing I am aware of is that the errors, specifically for mermaid, are very long so lines overflow and they end up looking not so great (see the attached video, which is worse considering the window is tiny).

pub(crate) trait RenderAsync: AsRenderOperations {
/// Start the render for this operation.
///
/// Should return true iff the invocation triggered the rendering (aka if rendering wasn't
Copy link
Contributor

Choose a reason for hiding this comment

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

Looks like a minor typo here (iff -> if)

Copy link
Owner Author

Choose a reason for hiding this comment

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

see but this is probably the second or third time someone points this out so I should just stop using this :).

@mikavilpas
Copy link
Contributor

Just tried this out - the difference is like night and day. Multithreading seems to work (2 graphs seem to get rendered in one go now). Caching works too, as does the loading indicator.

Great result, I'm really impressed!

@mfontanini mfontanini force-pushed the feat/async-render branch from 290d9f3 to dba3621 Compare July 1, 2024 13:55
@mfontanini mfontanini merged commit e9c542d into master Jul 1, 2024
6 checks passed
@mfontanini mfontanini deleted the feat/async-render branch July 1, 2024 13:57
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.

2 participants