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

Added Initial (experimental) web support. #902

Draft
wants to merge 23 commits into
base: master
Choose a base branch
from

Conversation

tetious
Copy link

@tetious tetious commented Jun 18, 2024

This adds web support, lightly tested on with a medium sized project on:

  • MacOs (Firefox and Chrome) 🟢
  • Linux (Firefox and Chrome) 🟢
  • ChromeOS (very old and low RAM Chromebook) 🟡 (see TODO below)

A caveat:

  1. At the moment, the official web export templates do not work. I suspect this is due to this issue, and will retest once a new Emscripten release is available. I added a note to this effect in the build instructions.

Please try it out!

Option A

  1. Grab the export templates (newly updated for Godot v4.3b2 and built with Emscripten v3.1.39 but otherwise unchanged) here and unzip them over your existing templates.
  2. Grab the extension files here and unzip them over your addons/godot-jolt folder.

Option B

You can build everything yourself following the build instructions in this PR (see building.md). Note that you will need to build the web export templates using Emscripten v3.1.39 otherwise godot-jolt will crash at load.

Please report back!

Either option you pick, please let us know how it works for your project.

TODO

  • Test with Emscripten 3.1.62 when it is released (both multi and single-threaded, and against the "official" export templates)
  • Investigate warnings in godot-cpp when LTO is enabled:
wasm-ld: warning: function signature mismatch: _ZNK5godot11AudioStream21_instantiate_playbackEv
>>> defined as () -> void in External/Build/godot-cpp/EditorDistribution/libgodot-cpp.a(unity_3_cxx.cxx.o)
>>> defined as (i32, i32) -> void in lto.tmp

wasm-ld: warning: function signature mismatch: _ZNK5godot14TextureLayered15_get_layer_dataEi
>>> defined as () -> void in External/Build/godot-cpp/EditorDistribution/libgodot-cpp.a(unity_23_cxx.cxx.o)
>>> defined as (i32, i32, i32) -> void in lto.tmp
  • Investigate crash on low-memory Chromebook:
    "RuntimeError: memory access out of bounds" when using a RayCast3d.
    at JPH::MeshShape::CastRay(JPH::RayCast const&, JPH::RayCastSettings const&, JPH::SubShapeIDCreator const&, JPH::CollisionCollector<JPH::RayCastResult, JPH::CollisionCollectorTraitsCastRay>&, JPH::ShapeFilter const&) const (wasm://wasm/01209dc6:wasm-function[11603]:0x28ea83)
  • CI setup
  • Basic smoke testing on more platforms/browsers

@mihe
Copy link
Contributor

mihe commented Jun 18, 2024

I appreciate the work you've put in here, but just so there's no misunderstanding, my stance on supporting this hasn't changed much since my original reply in #477. If anything, I have much less time to dedicate to this extension these days, so I have even less bandwidth to spare for this, especially when considering whatever bug reports or CI issues that might arise from this down the road. It already costs me a full day to cut a release for this extension, due to smoke testing all the various platforms and architectures and their respective deployment methods. I'm not super excited about adding more permutations to that list.

I'm not averse to merging this (when it's done) but there needs to be some sort of commitment from somebody with regards to taking on the above mentioned extra work.

If everything goes as planned that commitment shouldn't have to extend much further than Q1-ish 2025, at which point this extension will ideally have been merged into Godot as an official module and released as part of Godot 4.4, shortly after which I will likely archive this repository. Don't quote me on that timeline though. I'm sure something will delay it.

@tetious
Copy link
Author

tetious commented Jun 18, 2024

Totally understandable. I really appreciate all the work you've put into this extension, especially all the documentation and infrastructure. It was a pleasure to work on, in no small part to the hacking document and the overall structure and build process.

needs to be some sort of commitment from somebody with regards to taking on the above mentioned extra work.

I'm happy to commit to this. I'm working on a project that relies heavily on Joints, and have found GodotPhysics sufficiently unpredictable and incomplete that I need another option. So I need to see this through one way or another, and I'd love it to be in a form that can benefit others.

Here is what I propose: Emscripten 3.1.62 will likely drop soon, and Godot 4.3 final too. I'm hoping the combination of those two (or a followup 4.3.x) will eliminate the need to custom build the export templates in the medium term, but in the mean time I can produce those semi-automatically and host them.

With the final release of 4.3 (or soon thereafter), we can add web support under an unsupported "experimental" tag to gather feedback, etc. I will handle the web-related bug reports (though I will be of limited help with any physics/math stuff, alas) and CI setup for this and any subsequent releases until Godot 4.x takes it in.

I can start getting the various CI stuff going now in a separate MR?

Thanks again for your help getting this going and for your stewardship of this extension!

@mihe
Copy link
Contributor

mihe commented Jun 18, 2024

I'm hoping the combination of those two (or a followup 4.3.x) will eliminate the need to custom build the export templates in the medium term, but in the mean time I can produce those semi-automatically and host them.

I guess we'll have to see how these things develop, but I'm not super keen on relying on third-party export templates. I would probably prefer to see that this PR remains unmerged until Godot's official export templates are fixed.

I can start getting the various CI stuff going now in a separate MR?

Any reason why you couldn't do it in this PR?

@tetious
Copy link
Author

tetious commented Jun 18, 2024

I would probably prefer to see that this PR remains unmerged until Godot's official export templates are fixed.

Totally fine with me. I'm hopeful this will happen with the 4.3 release.

Any reason why you couldn't do it in this PR?

Nope; happy to. Wasn't sure you'd want both things in one. :)

@mihe
Copy link
Contributor

mihe commented Jun 18, 2024

Nope; happy to. Wasn't sure you'd want both things in one. :)

Not a problem at all.

I'll turn on the CI stuff for you in this PR. Note that there are quite a few CI jobs, due to the combinatory explosion of platforms and architectures, so it takes roughly an hour for each CI run to complete fully, and much longer than that if there are others queued up obviously.

@mihe
Copy link
Contributor

mihe commented Jun 18, 2024

Oh, and I don't run the packaging job as part of the regular CI, so you'll have to run that manually on your own fork to test that, which you should be able to do by pressing the "Run workflow" button here.

EDIT: You'll probably want to comment out (or just remove) the macOS part of the packaging though, found here, since it will try to sign the builds, which will fail if you don't have all the credentials set up as secrets in your own fork.

@mihe
Copy link
Contributor

mihe commented Jun 18, 2024

Also, I realize this deviates from the Godot conventions, but for what could end up being a long-running PR like this one, I would appreciate if you could refrain from force-pushing and instead commit/merge like normal. It makes incremental reviews easier.

.github/workflows/build-web.yml Outdated Show resolved Hide resolved
CMakeLists.txt Outdated Show resolved Hide resolved
CMakeLists.txt Show resolved Hide resolved
CMakeLists.txt Show resolved Hide resolved
CMakeLists.txt Outdated Show resolved Hide resolved
CMakePresets.json Outdated Show resolved Hide resolved
CMakePresets.json Outdated Show resolved Hide resolved
CMakePresets.json Outdated Show resolved Hide resolved
CMakePresets.json Outdated Show resolved Hide resolved
@Faless
Copy link

Faless commented Jun 24, 2024

I've successfully tested this on single threaded builds from the latest godot master.

I've also also created a patch(*) to add a separate profile for threaded builds (the two binaries are not compatible sadly), which can works with emscripten 3.1.61+git (see godotengine/godot#93143 ).

The patch also assumes godotengine/godot#93556 to allow the two binaries to co-exists and be exported according to the chosen variant.

(*) threads.diff.txt (NOTE: In the patch, I'm abusing the cacheVariables to set our own EMSCRIPTEN_USE_PTHREADS, which I later use to set the -lpthread flag and the files suffixes, not sure if there's a better solution)

@tetious
Copy link
Author

tetious commented Jun 24, 2024

Awesome! Thanks very much, @Faless!

Do you happen to know if 4.3 final's export templates will be built with Emscripten 3.1.62 (or +git)? OR if the output of 3.1.61+git works with the existing export templates (single threaded)?

I will pull this patch into the PR, unless @mihe would prefer it split into a new PR?

I'm hopeful the export template issues can be ironed out before 4.3 so we can merge and release (at least with an experimental flag or something) near final 4.3-time.

@mihe
Copy link
Contributor

mihe commented Jun 24, 2024

I will pull this patch into the PR, unless @mihe would prefer it split into a new PR?

No, by all means, I'd prefer to see the web support finished as much as it can be in this PR.

We'll iron out whatever kinks there might be after it's been pulled in.

@mihe
Copy link
Contributor

mihe commented Jun 24, 2024

Looks like you'll likely need to add -Wno-experimental among the target_compile_options(...) in order to get around error: -sSIDE_MODULE + pthreads is experimental.

Do not use MEMORY_GROWTH with threads.
@mcjohnalds
Copy link

I tested the joins.tscn example scene on Windows and an M1 Mac on the following browsers:

  • Edge
  • Safari
  • Firefox
  • Chrome

And it worked flawlessly! Really impressed. Amazing thanks to tetious and the godot-jolt contributors. I will be using jolt in game jams from now on.

@tetious
Copy link
Author

tetious commented Jul 12, 2024

Good news! The combination of 4.3b3's (default) export templates and godot-jolt built with Emscripten 3.1.62 works perfectly in my tests, both threaded and not threaded.

Bad news! I merged master and that broke the build. Something to do with adding -mfpmath=sse in jolt. Gonna see how to work around this and push an update.

I'll have an updated zip posted for folks to try once I get the build sorted.

I think we're getting close, though!

EDIT:
I think I'm going to have to work through the build issue upstream: jrouwe/JoltPhysics#1173

@mihe
Copy link
Contributor

mihe commented Jul 12, 2024

I've been meaning to try out this PR for myself, as I was planning on moving some things around, but time has been scarce.

What I'd like to see changed is (and I'm happy to do this myself when I have time):

  1. The CMake presets aren't really set up in the way they probably/unfortunately need to be. Since this is effectively cross-compilation, the presets ought to be more like the Android presets, which (again, unfortunately) are in the format of [host]-android-[arch] and [host]-android-[arch]-[config] for the configuration and build presets respectively. Without this you're not able to build for web on anything but Linux, since they inherit from linux-base, which enforces that it's only available on that platform.
  2. This is somewhat arbitrary, but threads on/off probably shouldn't be part of the presets. I try to avoid adding to the combinatorial explosion as much as I can, and I think it's best if threading is made into a CMake option instead, much like GDJ_DOUBLE_PRECISION. It could (should?) perhaps even be a general flag that forces usage of JPH::JobSystemSingleThreaded instead of JoltJobSystem, but that'll probably require some refactoring of the code as well.
  3. The non-threaded binaries should use the newly introduced nothreads feature tag.
  4. The compilation flags that are currently being added through CMakePresets.json all need to be moved into CMakeLists.txt. It should (in theory at least) be possible to use some other CMake generator (without the presets) and just pass the toolchain file and have things work. If some of those flags need to propagate to Jolt or some other dependency then they need to be forwarded through their respective GodotJoltExternal*.cmake file or be made into an upstream feature request.
  5. The -msse4.2 flag still bothers me, and I can't imagine it's actually needed.
  6. I can't help but feel like we should just pass -sINITIAL_MEMORY for both threaded and non-threaded, instead of having different behavior for different builds. There's also the question of what the right value is for this argument.
  7. Packaging still needs to be done I guess.

I'm also curious whether the amount of threads reported by Godot is correct for threads=yes builds, considering the comment you (@tetious) made here about it returning 2 for you with threads=no.

@Faless
Copy link

Faless commented Jul 12, 2024

6. I can't help but feel like we should just pass -sINITIAL_MEMORY for both threaded and non-threaded, instead of having different behavior for different builds. There's also the question of what the right value is for this argument.

About this, we shouldn't need to specify the INITIAL_MEMORY when building extensions (SIDE_MODULEs),

@tetious
Copy link
Author

tetious commented Jul 12, 2024

@mihe I'm happy to tackle at least some of these, and I think @Faless is probably right that we can just omit all the memory related flags as a "side_module" "DLL" or whatever. I don't know enough about the low level details to really wrap my brain around how wasm dlink works and how it differs from other platforms.

The -msse4.2 flag still bothers me, and I can't imagine it's actually needed.

Possibly not, through if you pass any sorta SIMD, you also have to pass msimd128. (see https://emscripten.org/docs/porting/simd.html) Maybe we can get away with detecting when a SIMD use_ flag is on and tacking on msimd128. I don't have enough CMAKE experience to know the best/right way to accomplish this for all the third party libraries, though.

If anyone has the time and inclination, two items I could use help on:

  • The LTO warnings are still present in current Emscripten. (see the todo list in the description) Really not sure what's up there.
  • I can still reliably crash with my project and optimized builds, but only on two Chromebooks I've been testing with. I need to see if I can figure out a minimal failure case, but haven't had a chance yet.
    • It's always in CastRay (see above)
    • It only fails on these two Chromebooks with 4GB RAM. Even a VM with 1GB doesn't crash, though...
    • ASSERTIONS=2 catches an immediate stack overflow for even the godot-jolt example project. This might be a red herring.
      Some additional testing by folks with more complicated projects would be very helpful!

@tetious
Copy link
Author

tetious commented Jul 12, 2024

@mihe When you get a sec, would you mind updating the jolt fork, please? The fix is in for excluding -mfpmath=sse for Emscripten builds.

Thanks!

@jrouwe
Copy link
Contributor

jrouwe commented Jul 12, 2024

The -msse4.2 flag still bothers me, and I can't imagine it's actually needed.

@mihe It actually is, if you compile with the emscripten flags -msimd128 -msse4.2 then it will convert SSE intrinsics to WASM SIMD instructions. Without it you fall back to plain C++ math because I didn't implement the WASM SIMD intrinsics.

@mihe
Copy link
Contributor

mihe commented Jul 12, 2024

@mihe It actually is, if you compile with the emscripten flags -msimd128 -msse4.2 then it will convert SSE intrinsics to WASM SIMD instructions. Without it you fall back to plain C++ math because I didn't implement the WASM SIMD intrinsics.

Sorry, I ended up replying in that Jolt issue before I saw your reply here.

My poorly phrased comment was meant as a continuation of this review comment. My concern was more about why it's SSE 4.2 specifically and not SSE2, and why Jolt requires passing it through CXXFLAGS/CMAKE_CXX_FLAGS rather than adding it itself, like with the other platforms.

I somehow missed that WebAssembly had its own set of SIMD instructions though. Is that somehow influencing the reasoning behind passing the x86 flags manually?

@jrouwe
Copy link
Contributor

jrouwe commented Jul 13, 2024

I somehow missed that WebAssembly had its own set of SIMD instructions though. Is that somehow influencing the reasoning behind passing the x86 flags manually?

I'll reply in the other ticket.

tetious and others added 3 commits July 13, 2024 15:23
Rip out threads preset.
Rename web threads option and wasm -> wasm32.
Add nothreads feature flag.
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.

None yet

5 participants