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

[cubing.js issue] Unable to generate screenshots simultaneously #359

Open
ryanb opened this issue Jan 18, 2025 · 12 comments
Open

[cubing.js issue] Unable to generate screenshots simultaneously #359

ryanb opened this issue Jan 18, 2025 · 12 comments
Labels
🐞 bug Something isn't working

Comments

@ryanb
Copy link

ryanb commented Jan 18, 2025

Steps to reproduce the issue

I get the same screenshot when I try to generate multiple ones simultaneously. Here's some HTML.

<html>
  <body>
    <img id="screenshot-1" />
    <img id="screenshot-2" />
    <script type="module">
      import {TwistyPlayer} from "https://cdn.cubing.net/v0/js/cubing/twisty";

      async function screenshot({alg, imgId}) {
        const twistyPlayer = new TwistyPlayer({alg});
        const img = document.getElementById(imgId);
        img.src = await twistyPlayer.experimentalScreenshot({width: 200, height: 200});
      }

      screenshot({alg: "R", imgId: "screenshot-1"})
      screenshot({alg: "F", imgId: "screenshot-2"})
    </script>
  </body>
</html>

Both screenshots look the same matching the last twisty player alg. If I add await screenshot(...) then it generates them correctly. Perhaps there's some global shared state in .experimentalScreenshot?

Observed behaviour

Screenshots look the same.

🖼 Screenshots

No response

Expected behaviour

Screenshot should match the player it's called on.

Environment

Static HTML.

Additional info

No response

@ryanb ryanb added the 🐞 bug Something isn't working label Jan 18, 2025
@lgarron
Copy link
Member

lgarron commented Jan 18, 2025

Since you made your function async, I think you meant to await?

await screenshot({ alg: "R", imgId: "screenshot-1" });
await screenshot({ alg: "F", imgId: "screenshot-2" });

As conveyed by the function signature and name, this is an async screenshot feature that is also experimental. 😛
If you think of your code as manipulating a physical cube or manipulating a player, you have replaced the alg on the second screenshot(…) line before the first screenshot had a chance to run any of its own async code.

It is theoretically possible for us to offer a version of this with different semantics, but that comes with its own complications and compromises. So await is your best option for the time being. (Or you could use a second <twisty-player> element.)

@ryanb
Copy link
Author

ryanb commented Jan 19, 2025

@lgarron

Since you made your function async, I think you meant to await?

This bug is specifically for the case of generating screenshots asynchronous . I'm reproducing a more complex scenario that I can't await on (rendering Svelte components).

(Or you could use a second <twisty-player> element.)

I'm using a new TwistyPlayer({alg}) per screenshot. Doesn't that do the same thing as separate <twisty-player> elements?

@lgarron
Copy link
Member

lgarron commented Jan 19, 2025

@lgarron

Since you made your function async, I think you meant to await?

This bug is specifically for the case of generating screenshots asynchronous . I'm reproducing a more complex scenario that I can't await on (rendering Svelte components).

Ah, Svelte strikes again! I assume this is for SSR? I'd definitely like to enable that.

If you can't await in Svelte, though, how are you able to take screenshots there at all? It would definitely be good to have a repro for that.

(Or you could use a second element.)

I'm using a new TwistyPlayer({alg}) per screenshot. Doesn't that do the same thing as separate <twisty-player> elements?

Ah, yeah, they're the same thing. So I refer to them interchangeably, sorry. 😅

@ryanb
Copy link
Author

ryanb commented Jan 20, 2025

@lgarron

Ah, Svelte strikes again! I assume this is for SSR? I'd definitely like to enable that.

I'm not doing SSR. This issue also isn't Svelte-specific. The HTML provided in the issue is a static HTML file you can open directly in the browser to see the bug. I've updated the code to include the html/body tag to make it more clear.

Ah, yeah, they're the same thing. So I refer to them interchangeably, sorry. 😅

The code does a new TwistyPlayer for each screenshot so something is shared in the experimentalScreenshot code causing one screenshot to override the other.

@lgarron
Copy link
Member

lgarron commented Jan 20, 2025

Ah, Svelte strikes again! I assume this is for SSR? I'd definitely like to enable that.

I'm not doing SSR. This issue also isn't Svelte-specific. The HTML provided in the issue is a static HTML file you can open directly in the browser to see the bug. I've updated the code to include the html/body tag to make it more clear.

Ah, I see. If you need lots of actual screenshots images, then the correct solution is definitely to await each screenshot from a given player before setting up the next.

However, I'm still a bit unclear on your use case — why do you need the screenshots? If they're being shown to visualize e.g. cases on a page, you should be able to create hundreds or thousands of players on a page with reasonable efficiency.

@ryanb
Copy link
Author

ryanb commented Jan 20, 2025

@lgarron

Ah, I see. If you need lots of actual screenshots images, then the correct solution is definitely to await each screenshot from a given player before setting up the next.

Is this the intended behavior of screenshots? Seems strange that one player would influence the screenshot from another player. If it's intentional perhaps worth documenting.

However, I'm still a bit unclear on your use case — why do you need the screenshots? If they're being shown to visualize e.g. cases on a page, you should be able to create hundreds or thousands of players on a page with reasonable efficiency.

Glad to know hundreds of players on one page will be efficient. The use case being I want to display a static cube image and am currently using a player for each case. I wanted to generate a screenshot and cache it locally so it will be more efficient on future loads (no need to even use a player). Each player is in its own svelte component so each one renders asynchronously. This is why it's difficult to await but I could use a global object to queue the image generation.

I think in the long-run I'll create an SVG template and swap out the colors based on the end state of a given algorithm. That would be most efficient. I'm uncertain if cubing.js has a way of helping with this.

Thanks for your work on this project!

@lgarron
Copy link
Member

lgarron commented Jan 21, 2025

Ah, I see. If you need lots of actual screenshots images, then the correct solution is definitely to await each screenshot from a given player before setting up the next.

Is this the intended behavior of screenshots?

Yes.

Seems strange that one player would influence the screenshot from another player. If it's intentional perhaps worth documenting.

That should definitely not be happening. If you run into that, please do let us know!

However, I'm still a bit unclear on your use case — why do you need the screenshots? If they're being shown to visualize e.g. cases on a page, you should be able to create hundreds or thousands of players on a page with reasonable efficiency.

Glad to know hundreds of players on one page will be efficient. The use case being I want to display a static cube image and am currently using a player for each case.

Yeah, our architecture is designed for this. There's more room to optimize, but this issue motivated me finally implement the most low-hanging fruit: 34836d6
https://experiments.cubing.net/cubing.js/twisty/ZBLL.html should show that you can easily load hundreds of players on a page. (The page loads in about half a second for me, including initialization of all 493 players.)

I wanted to generate a screenshot and cache it locally so it will be more efficient on future loads (no need to even use a player).

Hmm, have you fully modeled situations where this would be beneficial? I would definitely see the benefit of being able to show something useful even when JavaScript is not working (and this is something I want to have good tools for in the future1), but by far the most robust and efficient way to achieve that is either SSR or rendering the images at site deployment time.

Each player is in its own svelte component so each one renders asynchronously. This is why it's difficult to await but I could use a global object to queue the image generation.

Yeah, a global queue would work. At that point, though, you're actually doing the same as each player does under the hood. The first two players on a page have their own exclusive rendering context, but all the others actually reuse the same shared rendering context. So they are actually queueing to generate screenshots using the exact same mechanism as .experimentalScreenshot(…) and storing them individually on their canvas until you press "play".

In theory there could be a benefit to reducing the overhead each player element, e.g. if somehow you really needed to minimize the size of the DOM tree, or if the alg representations/puzzle states were all too large to keep in memory at once. But I think it would have to be a pretty extreme situation where this is a bottleneck (and I'd definitely love to know of such a real-world example if there is one).

If you're planning on caching or loading the images I'd also encourage you to make some measurements. The entire ZBLL page loads over the network in 248kB over the wire (973kB once uncompressed) across 4 files. A single 200×200 screenshot is 40kB (uncompressed base 64 when returned from .experimentalScreenshot(…)). It's possible that screenshots are more efficient for your use case, but you might need to put in some effort for that to actually come out ahead of just using <twisty-players> out of the box in terms of user experience.
(And if you're storing the images using JavaScript you have to keep in mind the 5MB origin storage limit and cache eviction: https://developer.mozilla.org/en-US/docs/Web/API/Storage_API/Storage_quotas_and_eviction_criteria)

I think in the long-run I'll create an SVG template and swap out the colors based on the end state of a given algorithm. That would be most efficient. I'm uncertain if cubing.js has a way of helping with this.

Yeah, if you use 2D SVG visualization then you could .experimentalScreenshot(…) to get SVGs. It's not particularly ergonomic to do so with custom SVGs right now (partially because it's hard to design an API around semi-trusted input), but the architecture is designed to support it. Let me know if you need help with that.

Footnotes

  1. See https://www.cube20.org/ for a simple image fallback example.

@ryanb
Copy link
Author

ryanb commented Jan 21, 2025

@lgarron

Thanks for the detailed response.

That should definitely not be happening. If you run into that, please do let us know!

It is happening for asynchronous screenshots. Two players are created. Try saving the HTML at the top of the issue and opening in a browser.

Image

If you're planning on caching or loading the images I'd also encourage you to make some measurements. The entire ZBLL page loads over the network in 248kB over the wire (973kB once uncompressed) across 4 files. A single 200×200 screenshot is 40kB (uncompressed base 64 when returned from .experimentalScreenshot(…)). It's possible that screenshots are more efficient for your use case, but you might need to put in some effort for that to actually come out ahead of just using out of the box in terms of user experience. (And if you're storing the images using JavaScript you have to keep in mind the 5MB origin storage limit and cache eviction: https://developer.mozilla.org/en-US/docs/Web/API/Storage_API/Storage_quotas_and_eviction_criteria)

I was considering storing it in IndexedDB which has higher storage limits. The optimization is less about bandwidth and more about client-side performance once the app has loaded. However since TwistyPlayer is so efficient I don't know if it's worth it. I haven't done any profiling. I'll probably skip this and jump to the SVG approach eventually.

Yeah, if you use 2D SVG visualization then you could .experimentalScreenshot(…) to get SVGs. It's not particularly ergonomic to do so with custom SVGs right now (partially because it's hard to design an API around semi-trusted input), but the architecture is designed to support it. Let me know if you need help with that.

I'm looking for a 3D representation. Something like https://github.com/tdecker91/visualcube

It's not priority for my project so I haven't looked into this yet.

@lgarron
Copy link
Member

lgarron commented Jan 21, 2025

It is happening for asynchronous screenshots. Two players are created. Try saving the HTML at the top of the issue and opening in a browser.

Right, that's a different thing. (You're taking two screenshots of a single player at the same time.)

I was considering storing it in IndexedDB which has higher storage limits. The optimization is less about bandwidth and more about client-side performance once the app has loaded.

Oh, interesting. If this is something you can get good perf with, that would certainly be cool

I'm looking for a 3D representation. Something like https://github.com/tdecker91/visualcube

Yeah, any of those SVGs should be compatible!

@ryanb
Copy link
Author

ryanb commented Jan 21, 2025

@lgarron

Right, that's a different thing. (You're taking two screenshots of a single player at the same time.)

Sorry to persist on this issue. But why is this a different thing? Why is creating two separate instances of new TwistyPlayer({alg}) considered a single player?

@lgarron
Copy link
Member

lgarron commented Jan 21, 2025

@lgarron

Right, that's a different thing. (You're taking two screenshots of a single player at the same time.)

Sorry to persist on this issue. But why is this a different thing? Why is creating two separate instances of new TwistyPlayer({alg}) considered a single player?

Ah, I'm so sorry. 😳

I specifically remember designing something to prevent this issue, so I thought it couldn't be happening with multiple players and misread your code. (It also didn't make sense to me that you would specifically intend to use multiple players for this, as that couldn't help with performance. But I guess that only makes sense to me because I know the implementation. 🤦)

But yeah, for some reason that code's not in place. Even if it's marked as experimental this would be nice to clean up. It seems there are now some more ecosystem options for this, I'll take a look and land some sort of stopgap as soon as I can so that you can at least screenshot multiple players in parallel.

@ryanb
Copy link
Author

ryanb commented Jan 21, 2025

@lgarron Thanks! Don't rush a fix on my account. Since you've made the player so efficient I don't know if it's worth doing the screenshot optimization anyway.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🐞 bug Something isn't working
Projects
None yet
Development

No branches or pull requests

2 participants