casual-simulation / casualos

Casual Open Simulation for the Web
https://ab1.bot
MIT License
48 stars 8 forks source link

Expose some HTMLCanvasElement functionality to user scripts #416

Closed Blitzy closed 4 months ago

Blitzy commented 4 months ago

This pull request needs some review!!

🗒️ This was some R&D work that is turning into a pull request!

This pull request exposes some functionality for HTMLCanvasElement to user scripts. More specifically it exposes transferControlToOffscreen() and getContext() on HTMLCanvasElement so that it becomes possible to utilize it for drawing to. This made it possible to use a normal <canvas> element combined with an OffscreenCanvas to display a user script managed three.js scene for example. It also adds getBoundingClientRect() for HTMLElement.

Review Points :

  1. Its not particularly ideal that a function like getBoundingClientRect() or transferControlToOffscreen() which are just synchronous functions that return a value immediately get converted into Promise when proxied. I'm not sure if there is a better way to handle this with the current system.

  2. There is a postMessage error that occurs when trying to transfer the OffscreenCanvas. However, despite the below error, the OffscreenCanvas is still operational from the user script and can things update and render as you would expect. I do not know why this error is occuring and would like some guidance.

image

  1. Please review the AuxVMImpl modifications to support transferables. This was done ~8 months ago and for all I know is where the above error is coming from due to an oversight from me?

Test Three.js Scene AUX FIle

This AUX uses the features implemented by this pull request to show a scene with ~350 randomly generated spinng cubes. The camera smoothly swirls around the cubes, meanwhile the user is able to click on any cube to light it up and make it glow in the scene.

AUX File: rc-threejs-basic-v3-1-18-simAppFork_20240313_183455.aux.zip

threejs_casualos_simApp

KallynGowdy commented 4 months ago

@Blitzy Everything else looks good apart from those two things I mentioned. Once fixed, I think the transfer error will go away.

Blitzy commented 4 months ago

@KallynGowdy your fix commit resolved the error issue. I'd say this is ready for merge.