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

Javascript Web WASM Fix #1247

Merged

Conversation

nicholas-maltbie
Copy link
Contributor

@nicholas-maltbie nicholas-maltbie commented Sep 21, 2023

The current tools/javascript.py has some errors that don't work with godot 4.x, this seems to be due to the SharedArrayBuffer memory not being enabled properly when compiled for wasm.

This can be fixed by adding the PThread flags.

https://github.com/godotengine/godot/blob/59139df16e7a10c3b9176f697d23b557af46601e/platform/web/detect.py#L195-L200

# Thread support (via SharedArrayBuffer).
env.Append(CPPDEFINES=["PTHREAD_NO_RENAME"])
env.Append(CCFLAGS=["-s", "USE_PTHREADS=1"])
env.Append(LINKFLAGS=["-s", "USE_PTHREADS=1"])
env.Append(LINKFLAGS=["-s", "PTHREAD_POOL_SIZE=8"])
env.Append(LINKFLAGS=["-s", "WASM_MEM_MAX=2048MB"])

Additionally, I added tests via the .github\workflows\ci.yml as a new job: 🌐 Web (wasm32)

  • For the automated job I selected EM version 3.1.45 which should be the same as version used by the main godot repo.

I have verified and tested the changes with a sample project that uses GDExtensions - https://github.com/nicholas-maltbie/godot_debug_draw_3d/ (it uses the changes as a path to the godot-cpp repo). You can checkout the demo hosted here - https://nickmaltbie.com/godot_debug_draw_3d/

If you have any comments or suggestions for the change let me know.

This should help address issue #1103 as I was running into that error before adding the fix.

@dsnopek
Copy link
Contributor

dsnopek commented Sep 22, 2023

Thanks!

So, wait... You actually have GDExtensions working in a web build!? My understanding was that we were still blocked by an Emscripten bug. I'll try to test this myself when I have a chance.

@Calinou Calinou added bug This has been identified as a bug platform:web topic:buildsystem Related to the buildsystem or CI setup topic:gdextension This relates to the new Godot 4 extension implementation cherrypick:4.1 labels Sep 22, 2023
@Calinou Calinou added this to the 4.2 milestone Sep 22, 2023
@dsnopek
Copy link
Contributor

dsnopek commented Sep 22, 2023

I just tried using your PR with the Summator example - everything seems to have compiled and built fine (using Emscripten 3.1.45). I tested this with web export templates built from Godot master @ godotengine/godot@c12d635 and I'm getting a whole bunch of errors like this when the page loads:

Selection_075

Am I missing a step?

Your hosted demo seems to be working for me.

@nicholas-maltbie
Copy link
Contributor Author

I just tried using your PR with the Summator example - everything seems to have compiled and built fine (using Emscripten 3.1.45). I tested this with web export templates built from Godot master @ godotengine/godot@c12d635 and I'm getting a whole bunch of errors like this when the page loads:

Selection_075

Am I missing a step?

I had built with the version 4.1.1-stable version of Godot, haven't tested with master branch yet. You need to enable dlink as part of the web template. I'll test with the latest master branch and see if I run into any error. Is this the Summator repo you were using? - https://github.com/paddy-exe/GDExtensionSummator

@dsnopek
Copy link
Contributor

dsnopek commented Sep 22, 2023

You need to enable dlink as part of the web template.

Yep, there's a different error about loadDynamicLibrary not being supported without doing that, and my testing got past that point.

Is this the Summator repo you were using? - https://github.com/paddy-exe/GDExtensionSummator

Yes

@DmitriySalnikov
Copy link
Contributor

DmitriySalnikov commented Sep 23, 2023

Selection_075

Could it be that this is not a problem in my demo due to the fact that some classes are not registered? Is there a limit on the number of functions or classes in WASM?

@nicholas-maltbie
Copy link
Contributor Author

Added some changes to get build working with web export for demo - https://github.com/nicholas-maltbie/GDExtensionSummator/tree/nickmaltbie/javascript-wasm-fix

Hey @dsnopek , I was looking into that bug from the latest master and I was able to repo the error, however it is working for build version 4.2-dev5 (I included some notes below).

Current build with no modifications

# checkout default commit
cd godot-cpp
git fetch origin
git checkout godot-4.1-stable
cd ..

# build godot-cpp
scons --directory godot-cpp
scons platform=javascript --directory godot-cpp
scons
scons platform=javascript

# export project using template
# I have godot-v4.1.1-stable installed in my cli
godot -v -e --headless --path game --quit
mkdir -p game/builds/web-gl
godot --headless --path game --export-debug Web

# Host local server for test
npx local-web-server --cors.embedder-policy "require-corp" --cors.opener-policy "same-origin" --directory .\game\builds\web-gl

image

Attempt failed with v4.1.1-stable and no modifications to godot-cpp

GDExtensionSummator.html:212 
WebAssembly.instantiate(): Import #15230 module="env" function="memory": mismatch in shared state of memory, declared = 0, imported = 1

When I added the patch, it seems to work fine:

# Add patch I wrote to v4.1.1-stable
cd godot-cpp
git clean -xdf

# checkout default commit
git fetch origin
git checkout godot-4.1-stable

# Add remote for my patch for v4.1.1-stable
git remote add nickmaltbie git@github.com:nicholas-maltbie/godot-cpp.git
git fetch nickmaltbie
git cherry-pick 8cc7abd45f9a8821b8ac8f6454842043228544c7
cd ..

# then build binaries
scons platform=windows --directory godot-cpp
scons platform=windows
scons platform=javascript --directory godot-cpp
scons platform=javascript

# export project using template
godot -v -e --headless --path game --quit
mkdir -p game/builds/web-gl
godot --headless --path game --export-debug Web

# Host local server for test
npx local-web-server --cors.embedder-policy "require-corp" --cors.opener-policy "same-origin" --directory .\game\builds\web-gl

image

Works just fine

Building via godot v4.2-dev (most recent development release according to download archives for v4.2 - https://godotengine.org/download/archive/).

# Reset any files
git clean -xdf

# Setup godot-cpp files for most recent fix
cd godot-cpp
git clean -xdf
git checkout nickmaltbie/javascript-wasm-fix
cd ..

# Rebuild godot-cpp
scons --directory godot-cpp
scons platform=javascript --directory godot-cpp
scons
scons platform=javascript

# build project with godot_4_2_dev
$godot_4_2_dev = "C:\Users\nickd\OneDrive\Programs\Godot\Godot_v4.2-dev5_win64.exe\Godot_v4.2-dev5_win64.exe"
&$godot_4_2_dev -v -e --headless --path game --quit
mkdir -p game/builds/web-gl
&$godot_4_2_dev --headless --path game --export-debug Web

# Host local server for test
npx local-web-server --cors.embedder-policy "require-corp" --cors.opener-policy "same-origin" --directory .\game\builds\web-gl

image

Also works just fine

Build godot from master branch

# From the godot repo
scons target=editor
scons platform=web dlink_enabled=yes target=template_debug

# From the GDExtensionSummator repo
# Reset any files
git clean -xdf

# Setup godot-cpp files for most recent fix
cd godot-cpp
git clean -xdf
git checkout nickmaltbie/javascript-wasm-fix
cd ..

# Rebuild godot-cpp
scons --directory godot-cpp
scons platform=javascript --directory godot-cpp
scons
scons platform=javascript

# build project with latest master build
$godot_latest = "D:\Projects\godot\bin\godot.windows.editor.x86_64.exe"
&$godot_latest -v -e --headless --path game --quit
mkdir -p game/builds/web-gl
&$godot_latest --headless --path game --export-debug Web

# Host local server for test
npx local-web-server --cors.embedder-policy "require-corp" --cors.opener-policy "same-origin" --directory .\game\builds\web-gl

image

Was able to repro the error

RangeError: WebAssembly.Table.get(): invalid index 64992 into funcref table of size 7744
    at getWasmTableEntry (GDExtensionSummator.js:4196:47)
    at Object.invokeEntryPoint (GDExtensionSummator.js:4207:15)
    at handleMessage (GDExtensionSummator.worker.js:123:35)

Windows build works just fine however

PS D:\Projects\GDExtensionSummator> D:\Projects\GDExtensionSummator\game\builds\windows\GDExtensionSummator.exe
PS D:\Projects\GDExtensionSummator> Godot Engine v4.2.dev.custom_build.c12d63556 - https://godotengine.org/
Vulkan API 1.3.242 - Forward+ - Using Vulkan Device #0: NVIDIA - NVIDIA GeForce RTX 3080 Ti
 
60

@nicholas-maltbie
Copy link
Contributor Author

Looking at diff between version 4.2-dev5 and latest master of godot repo

https://github.com/godotengine/godot/compare/e3e2528ba7f6e85ac167d687dd6312b35f558591..master#diff-959cadda0cc0092767d78eace8e61559a4e135f93e00e3d818334d6f7d7de936R204-R212

Some changes in platform/web/detect.py

        # Embree is heavy and requires too much memory (GH-70621).
        ("module_raycast_enabled", False),
    # Get version info for checks below.
    cc_version = get_compiler_version(env)
    cc_semver = (int(cc_version["major"]), int(cc_version["minor"]), int(cc_version["patch"]))

    if env["lto"] != "none":
        # Workaround https://github.com/emscripten-core/emscripten/issues/19781.
        if cc_semver >= (3, 1, 42) and cc_semver < (3, 1, 46):
            env.Append(LINKFLAGS=["-Wl,-u,scalbnf"])

I'm not sure exactly what these changes mean, however, we are in that EM version range (at 3.1.45) so it could be affected by that change. Looking up the issue, it seems like this is related to some import errors and excluding libraries that were causing problems - emscripten-core/emscripten#19781

This might be the cause of the error, however I'm not sure. It will take some more time to investigate but it seems like it works with build v4.2-dev5 but not with the latest master. @dsnopek are you familiar with this part of the godot codebase?

@nicholas-maltbie
Copy link
Contributor Author

Ok, so I gave it another shot this morning after some rest.

This might be the cause of the error, however I'm not sure. It will take some more time to investigate but it seems like it works with build v4.2-dev5 but not with the latest master. @dsnopek are you familiar with this part of the godot codebase?

I was able to confirm my suspicion, I rebuilt godot master branch with EM version 3.1.14 and it works just fine

I selected 3.1.14 as it's the lowest version that supports dlink and web. https://github.com/godotengine/godot/blob/c12d63556b5c1da03a00dd4c45c40e60bd8d68c2/platform/web/detect.py#L213-L216

    if env["dlink_enabled"]:
        if cc_semver < (3, 1, 14):
            print("GDExtension support requires emscripten >= 3.1.14, detected: %s.%s.%s" % cc_semver)
            sys.exit(255)

Then install emsdk v3.1.14

# install emsdk v3.1.14
cd 'D:\Projects\emsdk\'
.\emsdk.bat install 3.1.14
.\emsdk.bat activate 3.1.14

Then we can rebuild the target for godot with emcc v3.1.14

emcc --version
emcc (Emscripten gcc/clang-like replacement + linker emulating GNU ld) 3.1.14 (4343cbec72b7db283ea3bda1adc6cb1811ae9a73)
Copyright (C) 2014 the Emscripten authors (see AUTHORS.txt)
This is free and open source software under the MIT license.
There is NO warranty; not even for MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.

Once we have the right version, we can rebuild the export tempalte.

# in the godot repo, rebuild the web_dlink_debug.zip
scons platform=web dlink_enabled=yes target=template_debug

# I then had to copy it to the export templates folder and rename it to web_dlink_debug.zip
# for windows, it's here - %APPDATA%\Godot\export_templates\4.2.dev

# From the GDExtensionSummator project, we can rebuild
# Reset any files
git clean -xdf

# Setup godot-cpp files for most recent fix
cd godot-cpp
git clean -xdf
git checkout nickmaltbie/javascript-wasm-fix
cd ..

# Rebuild godot-cpp
scons --directory godot-cpp
scons platform=javascript --directory godot-cpp
scons
scons platform=javascript

# build project with latest master build
$godot_latest = "D:\Projects\godot\bin\godot.windows.editor.x86_64.exe"
&$godot_latest -v -e --headless --path game --quit
mkdir -p game/builds/web-gl
&$godot_latest --headless --path game --export-debug Web

# Host local server for test
npx local-web-server --cors.embedder-policy "require-corp" --cors.opener-policy "same-origin" --directory .\game\builds\web-gl

image

@nicholas-maltbie
Copy link
Contributor Author

Given this information that it works with emscripten version 3.1.14, I think the simplest option to handle this issue going forward would be to make a PR to main godot repo to build export templates with EM version 3.1.14 when building the dlink export since we know that 3.1.14 is stable while there are known issues with emscripten between versions 3.1.42 and 3.1.45 that cause the dlink build to fail. Although this is my first time contributing to the godot projects, is there any protocol that should be followed when making these kins of fixes?

@KoltPenny
Copy link

KoltPenny commented Sep 27, 2023

I was trying to build for web and I successfully accomplished it with your changes. However, I'm running into the following error:

image

Linux.
emcc: 3.1.46
Godot 4.1.1

It seems that the final runtime is not building properly since it's missing that function. Also, if I print that rtenv Object, a bunch of errors show up.

Screenshot from 2023-09-26 20-56-52

Which suggests there are many env variables missing. Any pointers as to why this is happening?

@Virus-Axel
Copy link

Virus-Axel commented Sep 27, 2023

I was trying to build for web and I successfully accomplished it with your changes. However, I'm running into the following error:

image

Linux. emcc: 3.1.46 Godot 4.1.1

It seems that the final runtime is not building properly since it's missing that function. Also, if I print that rtenv Object, a bunch of errors show up.

Screenshot from 2023-09-26 20-56-52

Which suggests there are many env variables missing. Any pointers as to why this is happening?
I removed my

I got rid of this by checking "Extension Support" in export settings. See if that helps!

Screenshot from 2023-09-27 15-01-15

@KoltPenny
Copy link

KoltPenny commented Sep 27, 2023

@Virus-Axel at least there's progress!
image

[UPDATE]
Deleted the previous export and made a clean one. Found this:
image

Which I believe is where everyone is at this point in time. I'll dive into this later to see if there's any solution.

@nicholas-maltbie
Copy link
Contributor Author

I got rid of this by checking "Extension Support" in export settings. See if that helps!

Hey @Virus-Axel, I was able to get it working, I posted notes and followed the same steps, happy you were able to get it working as well.

[UPDATE] Deleted the previous export and made a clean one. Found this: image

Which I believe is where everyone is at this point in time. I'll dive into this later to see if there's any solution.

Also, @KoltPenny, I believe those messages are warnings, it seems like the engine.js for web builds with 4.x as this error occurs even when GDExtensions is disabled. Reading up on the wiki for exporting for web there seems to be some parts still in progress. https://docs.godotengine.org/en/stable/tutorials/export/exporting_for_web.html#audio

@Virus-Axel
Copy link

Hi @nicholas-maltbie! I did get a gdextension running on web. However, it seems like it does not work on Firefox on linux. I tried your demo too but it does not work either. Works great on Chrome and chromium.

What happens on firefox: it just keeps loading and no error response. I tried firefox 117.0.1 and 118.0.1 (latest ATM).

Does it work on firefox-windows?

@Faless
Copy link
Contributor

Faless commented Oct 1, 2023

Last time I checked (emcc 3.1.45), threads + dynamic linking was broken.
Building Godot 4.x with scons p=web dlink_enabled=yes (which is required for GDExtension to work) will result in broken builds, most likely due to this: emscripten-core/emscripten#19425 .
See also: godotengine/godot#79578 .
Until we can get a working threads + dlink build of godot, gdextensions will not work.

I did get a gdextension running on web. However, it seems like it does not work on Firefox on linux. I tried your demo too but it does not work either. Works great on Chrome and chromium.

@Virus-Axel what version of emscripten did you use to make the build? which godot branch/commit did you build? Which flags did you use?

@Faless
Copy link
Contributor

Faless commented Oct 1, 2023

I just rebuilt godot with dlink support and the latest emcc version (3.1.46), and it seems the error is indeed still there.
Running in chrom* or firefox will get you a bunch of (which again, seems to be due to the emscripten issue linked above):

tmp_js_export.worker.js:24 worker.js onmessage() captured an uncaught exception: RangeError: WebAssembly.Table.get(): invalid index 65365 into funcref table of size 7744
threadPrintErr @ tmp_js_export.worker.js:24
tmp_js_export.worker.js:24 RangeError: WebAssembly.Table.get(): invalid index 65365 into funcref table of size 7744
    at getWasmTableEntry (tmp_js_export.js:4196:47)
    at Object.invokeEntryPoint (tmp_js_export.js:4207:15)
    at handleMessage (tmp_js_export.worker.js:123:35)
threadPrintErr @ tmp_js_export.worker.js:24
tmp_js_export.worker.js:24 worker.js onmessage() captured an uncaught exception: RangeError: WebAssembly.Table.get(): invalid index 65365 into funcref table of size 7744
threadPrintErr @ tmp_js_export.worker.js:24
tmp_js_export.worker.js:24 RangeError: WebAssembly.Table.get(): invalid index 65365 into funcref table of size 7744
    at getWasmTableEntry (tmp_js_export.js:4196:47)
    at Object.invokeEntryPoint (tmp_js_export.js:4207:15)
    at handleMessage (tmp_js_export.worker.js:123:35)

@dsnopek
Copy link
Contributor

dsnopek commented Oct 1, 2023

@Faless I haven't had a chance to retest and verify, but it appears that some folks have reported success with Emscripten 3.1.14 and Godot 4.1.1 (with the godot-cpp for 4.1). Apparently, it doesn't work with either (a) newer Emscripten and (b) Godot master.

@Virus-Axel
Copy link

@Faless I am using emcc version 3.1.18 in a fedora container. I don't know anything about the dynamic linking, I have a static library this is linked into my .wasm module. Like this:

em++ -o bin/libgodot-solana-sdk.javascript.template_release.wasm32.wasm -s SIDE_MODULE=1 -s USE_PTHREADS=1 -s PTHREAD_POOL_SIZE=8 -s src/account.bc src/account_meta.bc src/curve25519.bc src/hash.bc src/instruction.bc src/keypair.bc src/pubkey.bc src/register_types.bc src/solana_sdk.bc src/transaction.bc src/utils.bc BLAKE3/c/blake3.bc BLAKE3/c/blake3_dispatch.bc BLAKE3/c/blake3_portable.bc sha256/sha256.bc godot-cpp/bin/libgodot-cpp.javascript.template_release.wasm32.a

Flags when compiling:
-s USE_PTHREADS=1 -s SHARED_MEMORY=1 -O3 -fPIC -s SIDE_MODULE=1 -DPTHREAD_NO_RENAME -DWEB_ENABLED -DUNIX_ENABLED -DNDEBUG

@Faless
Copy link
Contributor

Faless commented Oct 1, 2023

@Virus-Axel sorry, I meant the godot compilation flags, not the godot-cpp ones.
In all my tests, I never got Godot to run after building it with dlink_enabled=yes.
It compiles fine, but exporting a game (even without using any extension) crashes immediately with the error above.

@dsnopek I will try Godot 3.1.14 and godot 4.1, but I'm pretty sure I did try that at some point and resulted in the same crash as above (IIRC 3.1.14 was the version we originally used in 4.x branch).

@Faless
Copy link
Contributor

Faless commented Oct 1, 2023

To clarify, if you use a regular godot build, i.e. not built with the dlink_enabled=yes option, the engine will start, but GDExtensions WILL NOT be loaded because dynamic linking (used to open the extension) will be disabled.

@Faless
Copy link
Contributor

Faless commented Oct 1, 2023

Godot's 4.1 branch dlink_enabled builds doesn't work at all as they are missing godotengine/godot#79578 which is required to have an acceptable export count (see emscripten-core/emscripten#15487 (comment) ) or what you get is this error spammed (and godot not starting):

Uncaught (in promise) CompileError: WebAssembly.instantiate(): imports count of 210840 exceeds internal limit of 100000 @+8005

@Faless
Copy link
Contributor

Faless commented Oct 1, 2023

Well, it seems that the official build of 4.1.1 actually works (and does not spit the imports count of N exceeds internal limit of 100000). I wonder if it's LTO doing miracles...

@Faless
Copy link
Contributor

Faless commented Oct 1, 2023

I am using regular 4.1.1 yes. But do you get it to work for firefox too?

Yes actually (FF 118.0.1 on linux), but I had to clear my cache first (which is weird, as we should have a no-cache option for local dev)

@Faless
Copy link
Contributor

Faless commented Oct 1, 2023

Yes actually (FF 118.0.1 on linux), but I had to clear my cache first (which is weird, as we should have a no-cache option for local dev)

Well, never mind, while the godot build with gdextension support works on firefox, loading the test extension in godot-cpp does not, stalling during load with no error for me (high CPU usage, dev tools unresponsive)

Copy link
Contributor

@Faless Faless left a comment

Choose a reason for hiding this comment

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

Nice job, and thanks a lot everybody for pointing out that this works on Chrom* with official 4.1.1 production builds (requires LTO to reduce the number of exported/impoerted symbols).

Could you apply this patch?

diff --git a/tools/javascript.py b/tools/javascript.py
index 3bab7de..e8f6af1 100644
--- a/tools/javascript.py
+++ b/tools/javascript.py
@@ -25,11 +25,8 @@ def generate(env):
     env["ARCOM"] = "${TEMPFILE(ARCOM_POSIX)}"
 
     # Thread support (via SharedArrayBuffer).
-    env.Append(CPPDEFINES=["PTHREAD_NO_RENAME"])
     env.Append(CCFLAGS=["-s", "USE_PTHREADS=1"])
     env.Append(LINKFLAGS=["-s", "USE_PTHREADS=1"])
-    env.Append(LINKFLAGS=["-s", "PTHREAD_POOL_SIZE=8"])
-    env.Append(LINKFLAGS=["-s", "WASM_MEM_MAX=2048MB"])
 
     # All intermediate files are just LLVM bitcode.
     env["OBJPREFIX"] = ""
@@ -44,9 +41,4 @@ def generate(env):
     env.Replace(SHLINKFLAGS="$LINKFLAGS")
     env.Replace(SHLINKFLAGS="$LINKFLAGS")
 
-    if env["target"] in ["editor", "template_debug"]:
-        env.Append(CCFLAGS=["-O0", "-g"])
-    elif env["target"] == "template_release":
-        env.Append(CCFLAGS=["-O3"])
-
     env.Append(CPPDEFINES=["WEB_ENABLED", "UNIX_ENABLED"])

The optimization flags are now handled by tools/targets.py and are no longer supposed to stay in tools/javascript.py.
The WASM_MEM_MAX, and PTHREAD_POOL_SIZE flags should only be relevant for the main module.
The PTHREAD_NO_RENAME is used in internal Godot threading code (do we want to still set it in godot-cpp for consistency? CC @dsnopek )

@Faless
Copy link
Contributor

Faless commented Oct 1, 2023

The tool and platform should also probably be renamed web, but let's do that in a separate PR.

@Virus-Axel
Copy link

Well, never mind, while the godot build with gdextension support works on firefox, loading the test extension in godot-cpp does not, stalling during load with no error for me (high CPU usage, dev tools unresponsive)

Ok same for me. Should it be solved extensions are buildable for web too, wow. Let's get to it.

@Faless
Copy link
Contributor

Faless commented Oct 1, 2023

For reference, I've opened godotengine/godot#82633 which should fix 4.2 builds (and builds without LTO in general)

@Faless Faless linked an issue Oct 1, 2023 that may be closed by this pull request
@nicholas-maltbie
Copy link
Contributor Author

Hi @nicholas-maltbie! I did get a gdextension running on web. However, it seems like it does not work on Firefox on linux. I tried your demo too but it does not work either. Works great on Chrome and chromium.

What happens on firefox: it just keeps loading and no error response. I tried firefox 117.0.1 and 118.0.1 (latest ATM).

Does it work on firefox-windows?

Hey, trying it out with the latest firefox on windows and it doesn't seem to be working either. but chromium based browsers are working fine.

Nice job, and thanks a lot everybody for pointing out that this works on Chrom* with official 4.1.1 production builds (requires LTO to reduce the number of exported/impoerted symbols).

Could you apply this patch?

diff --git a/tools/javascript.py b/tools/javascript.py
index 3bab7de..e8f6af1 100644
--- a/tools/javascript.py
+++ b/tools/javascript.py
@@ -25,11 +25,8 @@ def generate(env):
     env["ARCOM"] = "${TEMPFILE(ARCOM_POSIX)}"
 
     # Thread support (via SharedArrayBuffer).
-    env.Append(CPPDEFINES=["PTHREAD_NO_RENAME"])
     env.Append(CCFLAGS=["-s", "USE_PTHREADS=1"])
     env.Append(LINKFLAGS=["-s", "USE_PTHREADS=1"])
-    env.Append(LINKFLAGS=["-s", "PTHREAD_POOL_SIZE=8"])
-    env.Append(LINKFLAGS=["-s", "WASM_MEM_MAX=2048MB"])
 
     # All intermediate files are just LLVM bitcode.
     env["OBJPREFIX"] = ""
@@ -44,9 +41,4 @@ def generate(env):
     env.Replace(SHLINKFLAGS="$LINKFLAGS")
     env.Replace(SHLINKFLAGS="$LINKFLAGS")
 
-    if env["target"] in ["editor", "template_debug"]:
-        env.Append(CCFLAGS=["-O0", "-g"])
-    elif env["target"] == "template_release":
-        env.Append(CCFLAGS=["-O3"])
-
     env.Append(CPPDEFINES=["WEB_ENABLED", "UNIX_ENABLED"])

The optimization flags are now handled by tools/targets.py and are no longer supposed to stay in tools/javascript.py. The WASM_MEM_MAX, and PTHREAD_POOL_SIZE flags should only be relevant for the main module. The PTHREAD_NO_RENAME is used in internal Godot threading code (do we want to still set it in godot-cpp for consistency? CC @dsnopek )

Thanks for the suggestion @Faless, it seemed odd to include them in the file, but I wanted to make the smallest change to have the fix :) I'll apply the fix and test locally.

Added changes to tools/javascript.py to add PFlags to fix SharedArrayBuffer memory error.
Corrected some small errors in tools/javascript.py to support new target names.
Also updated ci to include validation for web build.
@nicholas-maltbie
Copy link
Contributor Author

Thanks for the help and review @Faless. If you need any other updates let me know.

Happy to help working on renaming platform to web in future as well as a follow up once we merge this change.

@bobziuchkovski
Copy link

bobziuchkovski commented Oct 1, 2023

What happens on firefox: it just keeps loading and no error response. I tried firefox 117.0.1 and 118.0.1 (latest ATM).

Does it work on firefox-windows?

Hey, trying it out with the latest firefox on windows and it doesn't seem to be working either.

Just to verify: does Firefox work for you when exporting a non-gdextension project to web?

Context: I just started experimenting with Web exports for my own project on Godot 4.1 a couple days ago. Even with pure gdscript, I found Firefox would hang. Chrome would also hang if the ANGLE backend was set to OpenGL but worked fine if the ANGLE backend was set to Metal. Safari just worked.

I'm hesitant to post this because I don't want to distract from the main conversation on this PR. That said, from what I've seen while testing, it's possible any remaining firefox issues are actually unrelated to gdextension or any of the changes in this PR. It seemed like the current GA Godot 4.1 web exports might fail on firefox as-is. 🤷‍♂️ I had planned to open an issue on the main godot repo, but wanted to test some more first.

@Virus-Axel
Copy link

I'm hesitant to post this because I don't want to distract from the main conversation on this PR. That said, from what I've seen while testing, it's possible any remaining firefox issues are actually unrelated to gdextension or any of the changes in this PR. It seemed like the current GA Godot 4.1 web exports might fail on firefox as-is. 🤷‍♂️ I had planned to open an issue on the main godot repo, but wanted to test some more first.

Wow, you are correct. I am unable to have any godot export run on firefox. Thank you for the information. If you create the issue could you post it here? :)

@PgBiel
Copy link

PgBiel commented Oct 2, 2023

Hello,
Just wanted to mention that it seems that the -sUSE_PTHREADS flag is deprecated (or, at least, superseded) in favor of -pthread, apparently since emsdk 3.1.34 (emscripten-core/emscripten#18923):

https://github.com/emscripten-core/emscripten/blob/main/src/settings.js#L2162

My personal opinion is that it could be unwise to perform the switch immediately if compatibility with older emsdk versions is needed; this could just be something to keep in mind in the future (if the older option is ever removed).

@Faless
Copy link
Contributor

Faless commented Oct 2, 2023

Just to verify: does Firefox work for you when exporting a non-gdextension project to web?

@bobziuchkovski well, it does work in my tests, you can try this link (Godot 4.1.1 export): https://fales.itch.io/godot-4-1-test-regular (confirmed to work on FF 118.0.1 on linux)

Just wanted to mention that it seems that the -sUSE_PTHREADS flag is deprecated (or, at least, superseded) in favor of -pthread, apparently since emsdk 3.1.34

@PgBiel Given we currently build with 3.1.18 I'd wait for now, until we decide to update the build containers at least, but thanks for pointing that out!

@akien-mga
Copy link
Member

akien-mga commented Oct 2, 2023

Given we currently build with 3.1.18 I'd wait for now, until we decide to update the build containers at least, but thanks for pointing that out!

For the record, I've started doing some work on update the build containers for Godot 4.2 (if I can finish early enough to validate the changes during the beta phase): godotengine/build-containers#128

So we might want to ensure Godot 4.2 works well with latest Emscripten both in godotengine/godot and here.

Copy link
Contributor

@dsnopek dsnopek left a comment

Choose a reason for hiding this comment

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

Thanks so much for figuring this out!

Like @Faless wrote, we should rename the tool and the platform to web, but I think this is fine for now, we can (and should!) further improve web builds in follow-up PRs.

@dsnopek dsnopek merged commit 96bec61 into godotengine:master Oct 4, 2023
11 of 12 checks passed
@nicholas-maltbie nicholas-maltbie deleted the nickmaltbie/javascript-wasm-fix branch October 7, 2023 22:08
@nicholas-maltbie
Copy link
Contributor Author

Hey @Faless and @dsnopek , thanks for reviewing the PR, I see you added the tag cherrypick:4.1 to the PR. Should I create a follow up PR to merge this change (2b4bcbb) into branch for 4.1 as well, or is that something handled by the maintainers of the repo?

@dsnopek
Copy link
Contributor

dsnopek commented Oct 9, 2023

I see you added the tag cherrypick:4.1 to the PR. Should I create a follow up PR to merge this change (2b4bcbb) into branch for 4.1 as well, or is that something handled by the maintainers of the repo?

This is something the maintainers (mainly, me :-)) do in batches. I've just cherry-picked this one for 4.1 in PR #1261

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug This has been identified as a bug platform:web topic:buildsystem Related to the buildsystem or CI setup topic:gdextension This relates to the new Godot 4 extension implementation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

imported shared memory but unshared required
10 participants