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

Browser Runtime Implementation #11

Merged
merged 14 commits into from
Sep 2, 2022

Conversation

zicklag
Copy link
Contributor

@zicklag zicklag commented Aug 9, 2022

This is somewhat work-in-progress, since it's missing a type declaration on the expected return type of the init() function, but I wanted to run this by you to see what you think.

This moves the current implementation of the JS runtime into it's own module named native to distinguish it from the wasm module which I'll be adding in a follow up PR if you like the look of this.

It switches to using an interface for scripts that will be possible to produce the same behavior in the browser, when we don't have access to independent runtimes or JsRealms or Contexts.

We expect each script to have an init() function that runs once when the asset is loaded, and again for each hot-reload. It needs to return an object with a function for every stage that it wants to execute code during.

This is allows one script to easily run code in multiple stages.

We now maintain only one runtime, allowing scripts to share a globalThis, but I don't think that's really an issue, and it makes it consistent with the browser. It also probably saves us some memory, because we don't create a full-blown runtime for every script.

Let me know if this looks like a reasonable direction to keep moving in.

I have the wasm runtime implemented already, I just have to migrate it from the game I was working on, and then I have to implement the ECS bindings, which will be the most of the remaining work to get first-class browser support.

Copy link
Owner

@jakobhellermann jakobhellermann left a comment

Choose a reason for hiding this comment

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

Ideally I would like to share the .js files between all runtimes.
That would mean that the .js files use a backend-agnostic "op" mechanism, so instead of

            return Deno.core.opSync("op_world_components", this.rid);

they would

            return bevyModJsScriptingOpSync("op_world_components", this.rid);

and each backend/runtime would define its own bevyModJsScriptingOpSync. Do you think this is possible even on wasm, or would we need a different common API betwenn the runtimes?

The common API I'm thinking of entails

  • having a way to map "resource ids" to arbitrary host data
  • having a way to call synchronous "ops" with arbitrary serialized parameters
    • currently values are not represented by resource IDs, but by a v8::Local::<v8::External> in an internal field of an object because I wasn't sure whether u32 resource ids were enough. Can this be done in wasm bindgen as well?

src/lib.rs Show resolved Hide resolved
src/runtime/mod.rs Show resolved Hide resolved
@zicklag
Copy link
Contributor Author

zicklag commented Aug 9, 2022

Ideally I would like to share the .js files between all runtimes.

I had vaguely thought about doing something like that. That's definitely preferable to duplicating the JavaScript for both platforms with annoying little tweaks between the two of them.

Do you think this is possible even on wasm, or would we need a different common API betwenn the runtimes?

That should be totally possible. On the browser we'd probably just make use of a global object on the window as a replacement for Deno's OpState.

currently values are not represented by resource IDs, but by a v8::Local::v8::External in an internal field of an object because I wasn't sure whether u32 resource ids were enough. Can this be done in wasm bindgen as well?

Yep. wasm_bindgen has it's equivalent pattern for wrapping private data in JavaScript facing, opaque objects.


Sounds like we're going in the right direction. I'll keep moving on this then! :)

@zicklag zicklag force-pushed the prepare-for-browser branch 3 times, most recently from 72f2f1b to 74b8ac2 Compare August 13, 2022 02:12
@zicklag
Copy link
Contributor Author

zicklag commented Aug 13, 2022

@jakobhellermann I noticed that we get errors in VSCode because our files aren't recognized as modules, so we can't redeclare the existing variables that are defined in other typescript files.

This isn't a problem at runtime because we scope the modules with closures, but VSCode doesn't know that. The only way to fix the error is to add an export statement to the files to make it know that they are scoped modules not global scripts.

The issue is that we don't actually support modules.

To fix this I went back to using SWC to transpile the JS/TS and added an extra step that transforms the export default statement to a return statement that makes sense in the scope of our closure we wrap the scripts in. Now runtime and VSCode time are happy!

And I like this syntax much more than having to make a function specifically called init(). It feels like kind of a nice workflow to be expected to export default [something that implements BevyScript].

But I'd like your feedback on that workflow and whether or not you think that's a good solution.


On a side-note, this also proves out the concept of rewriting import/export statements during transpiling, which can be used to make a bevy compatible module system.

@zicklag zicklag force-pushed the prepare-for-browser branch from 74b8ac2 to 6ac75fc Compare August 13, 2022 03:11
@zicklag zicklag changed the title Prepare For Browser Runtime Implementation Browser Runtime Implementation Aug 13, 2022
@zicklag zicklag force-pushed the prepare-for-browser branch 2 times, most recently from a3d3da0 to 6ee88cc Compare August 13, 2022 22:01
Revert "use deno_ast for typescript transpilation"

This reverts commit ae334f5.

Use Module Syntax For Scripts

WIP Browser Runtime
@zicklag zicklag force-pushed the prepare-for-browser branch from 6ee88cc to 362bdcb Compare August 14, 2022 00:10
@zicklag
Copy link
Contributor Author

zicklag commented Aug 14, 2022

I got the browser runtime working with bindings for logging, now all I have left is the ECS binding implementation!

Progress Update:

  • World info bindings: components, resources, entities, world.toString().
  • Get Resource
  • Query
  • Value
    • Get
    • Set
    • Call

@jakobhellermann
Copy link
Owner

The issue is that we don't actually support modules.

To fix this I went back to using SWC to transpile the JS/TS and added an extra step that transforms the export default statement to a return statement that makes sense in the scope of our closure we wrap the scripts in. Now runtime and VSCode time are happy!

Alternatively, what if we replace runtime.load_script with runtime.load_main_module? When creating the JsRuntime you can also specify a module loader, which can resolve imports and load sources.
That seems like the most flexible and least hacky solution. It just needs some work, because the module loading is all async, so we need to integrate that into bevy properly.
You don't need to do all that in this PR, I'm just brainstorming about possible solutions.

@zicklag
Copy link
Contributor Author

zicklag commented Aug 14, 2022

That seems like the most flexible and least hacky solution. It just needs some work, because the module loading is all async, so we need to integrate that into bevy properly.

That was my original thought, and in fact I've already gone this route and made it work on native, with an async loader and everything, but the issue was browser support. The browser can't import typescript files, and we can't create a custom module loader like we can with Deno.

So I think it ends up being easiest to do whatever will work in the browser, because it's the least common denominator, and anything that we can get to work in the browser we should be able to make work on native, too.

Technically we could use a custom loader on native and transpiling hacks on the browser, but it's probably much simpler just to use the same technique for both of them.


Also, interestingly, I believe rewriting the export and import statements might be compilant with the ECMA Script standard, because they intentionally didn't specify what process module loading has to take in the background. They left it unspecified so that transpilers like Babel could do whatever they wanted for module loading such as minifications, combining into a single file, etc, as long as it satisfied the required behavior.

So while it seems a little hacky, it may actually be standards compliant. 😄

- Share types with native and WASM runtimes.
- Avoid errors in console by waiting until script is loaded before
running it.
- Use more idiomatic approach for calling WASM script functions.
Implements all the remaining ops for the browser runtime, other than
op_value_ref_call.
@zicklag
Copy link
Contributor Author

zicklag commented Aug 15, 2022

I just finished implementing all the remaining ops other than op_value_ref_call in the browser script runtime!

Here's the script incrementing the score in the browser.

Peek 2022-08-15 11-15

Now I just have to implement the last op and clean up the code! 🥳

@zicklag zicklag mentioned this pull request Aug 15, 2022
2 tasks
@zicklag
Copy link
Contributor Author

zicklag commented Aug 16, 2022

I got op_value_ref_call implemented! The browser runtime is now caught up to the native runtime in features.

I've got a serious code cleanup to do and then this will be ready for review.

@zicklag zicklag force-pushed the prepare-for-browser branch from 83c9b87 to aa70660 Compare August 16, 2022 14:35
@zicklag zicklag force-pushed the prepare-for-browser branch from aa70660 to 305425d Compare August 16, 2022 14:38
@zicklag
Copy link
Contributor Author

zicklag commented Aug 16, 2022

OK, this is ready for review. To help you review, here are the high-level changes I made:

  1. Added a .editorconfig file. This helps make VSCode play nice with differing indent styles and formatters for TypeScript and Rust.
  2. Transpiling changes
    • Removed the typescript feature, making it "enabled" unconditionally, because now we are doing export rewriting, even on normal JavaScript which needs the same dependencies as typescript conversion.
    • Switch back to the more manual swc process you had for typescript conversion so we could stick our export rewriting code in the middle of the process.
    • Renamed ts_to_js.rs to transpile.rs to reflect its more general purpose now.
  3. Reformatted TypeScript files. This kind of just happened from my VSCode editor because I format on save. I use Prettier for formatting.
  4. As discussed above, we now use a new format for specifying when script functions should run:
    • Each script is expected to have an export default that returns an object with functions for each stage it wants to run in: first, update, pre_update, etc.
    • The returned object is passed as the receiver to each method call, allowing you to return classes if desired.
  5. Renamed types.d.ts to lib.bevy.d.ts to be consistent with other lib.[something].d.ts files used in a similar way to provide the foundation environment in the TypeScript repo.
    • I also moved this file to the root of the repo to make it easy to find so that people can easily copy it into their own repos, which is necessary to make the types load properly for people who use this crate as a dependency.
  6. Added a bash script to compile the examples for the browser.
  7. Moved modules around and made modifications to make the native and browser runtimes share the same API. Also fixed any clippy warnings in the native runtime.
  8. Added a tsconfig.json file that makes sure that intellisense doesn't add browser types to the script environment.
  9. Enabled opt-level = 1 for dev builds because that was the minimum opt level needed to make web examples load without crashing.

It's worth noting that there's a fair bit of code duplication in the implementation of the ops for the browser and the native runtimes, but there are also difference sprinkled throughout so I don't think it's something that can be abstracted away.

@zicklag
Copy link
Contributor Author

zicklag commented Aug 16, 2022

I just realized that I handled value ref lifetimes differently in the browser runtime than you did in the native runtime.

If I understand correctly, you garbage collected value refs on the native runtime by hooking into the runtime's finalization handler. Unfortunately, on the browser, we don't have that ability because JS doesn't have destructors yet. :/

To handle this, I just deleted every value ref at the end of every script run on the Rust side, but this doesn't delete the references on the JavaScript side. This mean that you couldn't store a value ref for later. You would have to re-obtain it every frame through querying.

This feels somewhat natural because it's not like you're allowed to store references to components across frames in Bevy in Rust anyway, you always have to query to get references to the ECS data. Edit: Oh, but this means that you couldn't store the freestanding results of function calls across frames either ( unless it's a primitive ). :/

We may just have to require that scripts store any component data that must be accessed across frames in a resource or a component. Which again, isn't much different than Bevy, so it might be that bad.

Right now, storing a value reference in a global variable that can be access across frames on the JavaScript side will work on native, but it will fail with a descriptive error on WASM, because the references are deleted at the end of every script run. So we'll need to figure out how we want to move forward here.

Maybe that is actually fine. And people who only care about native will get a little bit better of an experience, but people who want to work on WASM will have to understand they have to work around that limitation? If we document it well enough, that might be fine.

@jakobhellermann
Copy link
Owner

If I understand correctly, you garbage collected value refs on the native runtime by hooking into the runtime's finalization handler. Unfortunately, on the browser, we don't have that ability because JS doesn't have destructors yet. :/

Yes, I did that because it was the only way I could find to free the allocated external data after a while and not let it accumulate indefinitely.
Clearing the data every frame is definitely an option as well, I'll have to think about that a bit.
I wouldn't block this PR on having a definite solution to the data management.

@erlend-sh
Copy link

erlend-sh commented Aug 27, 2022

I wouldn't block this PR on having a definite solution to the data management.

If you’re fine with merging this, we can proceed with our Scripting MVP and begin using it in production 🤘

@jakobhellermann
Copy link
Owner

If you’re fine with merging this, we can proceed with our Scripting MVP and begin using it in production metal

I'll give this a proper review by the end of the week (or maybe sooner if I get around to doing it), and then probably merge it.
That said, I don't think that this is production ready quite yet. I noticed that if you run a script that does a simple thing (like doubling the score in breakout) every frame, after a bit of time the app segfaults somewhere in v8. This is pretty unacceptable, especially in a language like rust where such an error usually is caused wrong usage of unsafe. I'll have to investigate whether this is something that I messed up in the integration, or if it is a problem in v8 itself or the rusty_v8 bindings.
Also I want to go over all usages of unsafe in this crate and properly document and verify their safety constraints. For example, the data associated with an ECS value reference (like a component or resource) is stored in an internal field as a v8::External that I cast to the correct type. But I have no idea if I can guarantee somehow that no one else will create an object with an internal field of an external of another type, in which case the code will do unexpected bad things:

let value = &*external.value().cast::<ReflectValueRefTransmit>();

@erlend-sh
Copy link

Cheers. To be clear, I’m stretching the term ‘in production’ here to mean ‘put to use in the ongoing development of a real-world pre-alpha application’, hehe.

@zicklag
Copy link
Contributor Author

zicklag commented Aug 28, 2022

Yeah, "production" here means we're using it in a game, but the game itself is by no means "production ready" anyway. :)

the data associated with an ECS value reference (like a component or resource) is stored in an internal field as a v8::External that I cast to the correct type.

In the web backend I used keys into a SlotMap instead of pointers for storing refs, which meant I could get away without unsafe casts. It might be something to try for the v8 backend, too.

Then even arbitrarily generated keys on the JavaScript side will just safely come back null.

@jakobhellermann
Copy link
Owner

I gave this PR another review, pushed some small changes (formatting and removing the wasm scripts in favor of wasm-server-runner), but other than that it looks good to me. There's definitely room for improvement in the future, like consistent value ref lifetime handling (#16) and sharing more code between the runtimes, but that doesn't need to be in this PR (and I'm happy to do it myself as well).
Thanks for all the work you've put into this @zicklag ❤️

@jakobhellermann jakobhellermann merged commit a50e2b7 into jakobhellermann:main Sep 2, 2022
@zicklag zicklag deleted the prepare-for-browser branch September 2, 2022 19:01
@zicklag
Copy link
Contributor Author

zicklag commented Sep 2, 2022

Yay! 🎉

I'm glad I can help, and thanks so much for your work, too! I'm excited to see how we can progress scripting in Bevy. :D

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.

3 participants