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

WebXR: Update for Emscripten 3.1.71 and later #100489

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

dsnopek
Copy link
Contributor

@dsnopek dsnopek commented Dec 16, 2024

Emscripten moved some internal APIs that were on Browser over to the new MainLoop. This updates Godot's webxr module for those changes, otherwise it won't work if built with Emscripten 3.1.71 or newer.

Unfortunately, because MainLoop is new and we have to put it in $GodotWebXR__deps, Godot will no longer compile with Emscripten versions earlier than 3.1.71 with these changes. I'm not sure there's an easy way to make it compatible with both old and new versions?

In any case, as it is now, we shouldn't merge this until after we update Emscripten in the official build containers - see godotengine/build-containers#150

@dsnopek dsnopek added this to the 4.x milestone Dec 16, 2024
@dsnopek dsnopek requested a review from a team December 16, 2024 20:59
@dsnopek dsnopek requested a review from a team as a code owner December 16, 2024 20:59
@dsnopek dsnopek force-pushed the webxr-emscripten-3173 branch 2 times, most recently from e3dd967 to b99d784 Compare December 16, 2024 21:23
@dsnopek dsnopek requested a review from a team as a code owner December 16, 2024 21:23
@dsnopek
Copy link
Contributor Author

dsnopek commented Dec 16, 2024

CI needs to use a newer Emscripten for this to build, so this won't pass CI until after PR #100482 is merged

@adamscott
Copy link
Member

I'll try to see what they mean by JS macros here:

emscripten-core/emscripten#22510

@dsnopek
Copy link
Contributor Author

dsnopek commented Dec 16, 2024

I'll try to see what they mean by JS macros here:

emscripten-core/emscripten#22510

Oh, interesting! I think they're talking about the preprocessor macros used throughout Emscripten's JS library code, like #if ASSERTIONS. I had thought that those all corresponded to settings (ie -s arguments like -sASSERTIONS, also defined in settings.js), but if we can define our own macrcos, we could use that to have different code for different Emscripten versions

@dsnopek
Copy link
Contributor Author

dsnopek commented Dec 16, 2024

Linked from the issue Adam pointed to above, someone claimed to have success using a macro like this:

#if EMSCRIPTEN_VERSION.split('.').map(parseFloat)[0] < 3 || (EMSCRIPTEN_VERSION.split('.').map(parseFloat)[0] == 3 && EMSCRIPTEN_VERSION.split('.').map(parseFloat)[1] < 1) || (EMSCRIPTEN_VERSION.split('.').map(parseFloat)[0] == 3 && EMSCRIPTEN_VERSION.split('.').map(parseFloat)[1] == 1 && EMSCRIPTEN_VERSION.split('.').map(parseFloat)[2] < 71)

Aside from being pretty terrible, it didn't actually work me. :-/ That seems to compile fine with 3.1.74, but I get an error (with no error message) when trying with 3.1.64.

@adamscott
Copy link
Member

I created some macros to use in library_godot_webxr.js. Here's the diff.

Note that I just compiled successfully in 3.1.64 and 3.1.74. I didn't test if my fix worked (i.e. open a WebXR project).

diff --git a/modules/webxr/native/library_godot_webxr.js b/modules/webxr/native/library_godot_webxr.js
index 40400272bf..3130e73ea2 100644
--- a/modules/webxr/native/library_godot_webxr.js
+++ b/modules/webxr/native/library_godot_webxr.js
@@ -29,7 +29,24 @@
 /**************************************************************************/
 
 const GodotWebXR = {
-	$GodotWebXR__deps: ['$MainLoop', '$GL', '$GodotRuntime', '$runtimeKeepalivePush', '$runtimeKeepalivePop'],
+	$GodotWebXR__deps: [
+#if EMSCRIPTEN_VERSION_IS_SMALLER_THAN(3,1,71)
+		'$Browser',
+#else 
+		'$MainLoop',
+#endif
+		'$GL', 
+		'$GodotRuntime', 
+		'$runtimeKeepalivePush', 
+		'$runtimeKeepalivePop'
+	],
+	$GodotWebXR__postset: `
+#if EMSCRIPTEN_VERSION_IS_SMALLER_THAN(3,1,71)
+		GodotWebXR.mainLoop = Browser;
+#else 
+		GodotWebXR.mainLoop = MainLoop;
+#endif
+	`,
 	$GodotWebXR: {
 		gl: null,
 
@@ -44,6 +61,8 @@ const GodotWebXR = {
 		touches: new Array(5),
 		onsimpleevent: null,
 
+		mainLoop: null,
+
 		// Monkey-patch the requestAnimationFrame() used by Emscripten for the main
 		// loop, so that we can swap it out for XRSession.requestAnimationFrame()
 		// when an XR session is started.
@@ -64,9 +83,9 @@ const GodotWebXR = {
 		},
 		monkeyPatchRequestAnimationFrame: (enable) => {
 			if (GodotWebXR.orig_requestAnimationFrame === null) {
-				GodotWebXR.orig_requestAnimationFrame = MainLoop.requestAnimationFrame;
+				GodotWebXR.orig_requestAnimationFrame = GodotWebXR.mainLoop.requestAnimationFrame;
 			}
-			MainLoop.requestAnimationFrame = enable
+			GodotWebXR.mainLoop.requestAnimationFrame = enable
 				? GodotWebXR.requestAnimationFrame
 				: GodotWebXR.orig_requestAnimationFrame;
 		},
@@ -76,11 +95,11 @@ const GodotWebXR = {
 			// enabled or disabled. When using the WebXR API Emulator, this
 			// gets picked up automatically, however, in the Oculus Browser
 			// on the Quest, we need to pause and resume the main loop.
-			MainLoop.pause();
+			GodotWebXR.mainLoop.pause();
 			runtimeKeepalivePush();
 			window.setTimeout(function () {
 				runtimeKeepalivePop();
-				MainLoop.resume();
+				GodotWebXR.mainLoop.resume();
 			}, 0);
 		},
 
diff --git a/platform/web/SCsub b/platform/web/SCsub
index 9a2eea9e07..15ba28c131 100644
--- a/platform/web/SCsub
+++ b/platform/web/SCsub
@@ -33,6 +33,7 @@ if env["target"] == "editor":
     env.add_source_files(web_files, "editor/*.cpp")
 
 sys_env = env.Clone()
+sys_env.AddJSLibraries(["js/libs/library_godot_macros.js"], prepend=True)
 sys_env.AddJSLibraries(
     [
         "js/libs/library_godot_audio.js",
diff --git a/platform/web/emscripten_helpers.py b/platform/web/emscripten_helpers.py
index 6a3855da84..dbb8143a85 100644
--- a/platform/web/emscripten_helpers.py
+++ b/platform/web/emscripten_helpers.py
@@ -115,8 +115,11 @@ def get_template_zip_path(env):
     return "#bin/.web_zip"
 
 
-def add_js_libraries(env, libraries):
-    env.Append(JS_LIBS=env.File(libraries))
+def add_js_libraries(env, libraries, prepend=False):
+    if prepend:
+        env.Prepend(JS_LIBS=env.File(libraries))
+    else:
+        env.Append(JS_LIBS=env.File(libraries))
 
 
 def add_js_pre(env, js_pre):
diff --git a/platform/web/js/libs/library_godot_macros.js b/platform/web/js/libs/library_godot_macros.js
new file mode 100644
index 0000000000..76f3ca9e79
--- /dev/null
+++ b/platform/web/js/libs/library_godot_macros.js
@@ -0,0 +1,36 @@
+{{{
+	globalThis.___EMSCRIPTEN_VERSION_PARSED = globalThis.EMSCRIPTEN_VERSION.split(".").map((n) => parseInt(n));
+
+	globalThis.___SEMVER_IS_GREATER_THAN = (a, b, { orEqual = false } = {}) => {
+		const [aMajor, aMinor, aPatch] = a;
+		const [bMajor, bMinor, bPatch] = b;
+		if ((orEqual && aMajor >= bMajor) || aMajor > bMajor) {
+			if ((orEqual && aMinor >= bMinor) || aMinor > bMinor) {
+				if ((orEqual && aPatch >= bPatch) || aPatch > bPatch) {
+					return true;
+				}
+			}
+		}
+		return false;
+	};
+
+	globalThis.EMSCRIPTEN_VERSION_IS_GREATER_THAN = (major, minor, patch) => {
+		const isGreater =  globalThis.___SEMVER_IS_GREATER_THAN(globalThis.___EMSCRIPTEN_VERSION_PARSED, [major, minor, patch]);
+		return isGreater;
+	};
+
+	globalThis.EMSCRIPTEN_VERSION_IS_GREATER_THAN_OR_EQUAL = (major, minor, patch) => {
+		const isGreaterOrEqual = globalThis.___SEMVER_IS_GREATER_THAN(globalThis.___EMSCRIPTEN_VERSION_PARSED, [major, minor, patch], { orEqual: true });
+		return isGreaterOrEqual;
+	};
+
+	globalThis.EMSCRIPTEN_VERSION_IS_SMALLER_THAN = (major, minor, patch) => {
+		const isGreaterOrEqual = globalThis.___SEMVER_IS_GREATER_THAN(globalThis.___EMSCRIPTEN_VERSION_PARSED, [major, minor, patch], { orEqual: true });
+		return !isGreaterOrEqual;
+	};
+
+	globalThis.EMSCRIPTEN_VERSION_IS_SMALLER_THAN_OR_EQUAL = (major, minor, patch) => {
+		const isGreater = globalThis.___SEMVER_IS_GREATER_THAN(globalThis.___EMSCRIPTEN_VERSION_PARSED, [major, minor, patch]);
+		return !isGreater;
+	};
+}}}

@dsnopek dsnopek force-pushed the webxr-emscripten-3173 branch from b99d784 to d85ab6c Compare December 17, 2024 03:57
@dsnopek
Copy link
Contributor Author

dsnopek commented Dec 17, 2024

@adamscott Thanks! I added your patch with some slight modifications to get it working, and it is working for me with both Emscripten 3.1.64 and 3.1.74. However, eslint hates it (at least locally) and so I expect this will fail the static checks. I'm not sure if we can get eslint to understand these JS preprocessor macros?

@dsnopek dsnopek force-pushed the webxr-emscripten-3173 branch from d85ab6c to 5a18b0f Compare December 17, 2024 04:02
@adamscott
Copy link
Member

adamscott commented Dec 17, 2024

I took the liberty to add a commit to your PR. It adds an eslint processor to filter out C preprocessors. (yo dawg)

I also renamed EMSCRIPTEN_VERSION_IS_SMALLER_THAN to EMSCRIPTEN_VERSION_IS_LESS_THAN. More appropriate.

@adamscott adamscott force-pushed the webxr-emscripten-3173 branch 9 times, most recently from 6ff0387 to 1be5a20 Compare December 17, 2024 11:52
@adamscott
Copy link
Member

adamscott commented Dec 17, 2024

It was super complicated to integrate preprocessor to ignore the C preprocessors, but I did it.

Here's the repo: https://github.com/godotengine/eslint-plugin-ignore-c-preprocessors

@akien-mga
Copy link
Member

I question whether doing all this was really needed...

We could just stay on an older version, and eventually upgrade to >3.1.71 and drop support for older versions.

@dsnopek dsnopek force-pushed the webxr-emscripten-3173 branch from 1be5a20 to 9c288ef Compare December 17, 2024 14:13
@dsnopek
Copy link
Contributor Author

dsnopek commented Dec 17, 2024

Thanks @adamscott - the macros and eslint configuration were very cool. :-) Perhaps we'll have a critical situation in the future where we'll really need to use those

Per @akien-mga's comment, I've reverted this PR back to its original state, where it only works with Emscripten 3.1.71 and newer. We can save it until we do decide to upgrade Emscripten

@adamscott
Copy link
Member

adamscott commented Dec 17, 2024

I'll remove the repo from the Godot org.

Here's my personal repo, for archives.
https://github.com/adamscott/eslint-plugin-ignore-c-preprocessors

@dsnopek dsnopek force-pushed the webxr-emscripten-3173 branch from 9c288ef to 4b0e2eb Compare January 7, 2025 11:11
Comment on lines 2 to 4
"name": "godot",
"private": true,
"version": "1.0.0",
Copy link
Member

Choose a reason for hiding this comment

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

Is this a node/npm update that causes the indentation change from 2 spaces to a tab? Or is that a personal IDE config?

We should probably make sure we pick a standard and stick to it to avoid having needless diffs. I'm fine changing to tabs if this is the recommended way, as long as we don't change it back again.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This was part of @adamscott's changes, so I'm not sure the motivation, but for now, I've switched it back to spaces - it makes the diff much easier to review

@dsnopek dsnopek force-pushed the webxr-emscripten-3173 branch from 4b0e2eb to aa96564 Compare January 7, 2025 12:44
@dsnopek dsnopek force-pushed the webxr-emscripten-3173 branch from aa96564 to 97cae55 Compare January 7, 2025 12:47
@dsnopek
Copy link
Contributor Author

dsnopek commented Jan 7, 2025

As discussed on RocketChat, I've pushed the version of this PR that we had earlier, which included @adamscott's changes that allow using macros to target code to specific Emscripten versions.

@adamscott I think you'll need to move your https://github.com/adamscott/eslint-plugin-ignore-c-preprocessors back into the godotengine organization, or at least make a release, in order for CI to work

@adamscott
Copy link
Member

I think this PR needs also #100525, as I was not able to build Godot with Closure without it for 3.1.71 and later.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants