search

jakobhellermann / bevy_mod_js_scripting

Other
83 stars 6 forks source link

Create JsRuntimeOp Abstraction and Migrate To It #19

Closed zicklag closed 2 years ago

zicklag commented 2 years ago

Related Issues

jakobhellermann commented 2 years ago

Awesome, this looks great from a quick look.

What's the reason for having the JsRuntimeOp in an Arc? Couldn't it be a Box<dyn JsRuntimeOp> as well? I don't see where op would be shared.

zicklag commented 2 years ago

I think it could be a box. I was just cloning it out of the JsRuntimeConfig resource so I put it in an Arc, but since JsRuntimeConfig is only used once during initialization I could just move it instead of cloning it.

zicklag commented 2 years ago

I just pushed a commit that updates the WASM runtime, making this feature complete!

The update also moved the JsValueRefs and JsReflectFunctions out of the world and into a new TypeMap that represents a shared state for use in op implementations. This turned out to be important for garbage collection on WASM, which, unlike our native implementation, can have it's callback hook called at any time, not just at the end of the frame, where it has access to the world.

So moving the value refs into the runtime state allows us to free value refs without having to borrow the world.

I also moved some types into more appropriate modules.

jakobhellermann commented 2 years ago

Is this ready to be merged or is there anything missing?

jakobhellermann commented 2 years ago

I tried this out and it seems to be working fine so I'm gonna go ahead and merge it, if there's anything missing we can fix it in follow up PRs.