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

DISTS: Updated Webassembly / Emscripten Port #3766

Merged
merged 9 commits into from Jun 12, 2022
Merged

Conversation

chkuendig
Copy link
Contributor

@chkuendig chkuendig commented Mar 21, 2022

I kept working on the Webassembly / Emscripten port in the past months and I feel it's ready now for another PR:

Changes include a few critical improvements:

  • Dynamic Plugins: It's now possible to dynamically load plugins, making it possible to bundle all engines without the binary growing into unreasonable sizes. This depends on emscripten-core/emscripten#15893 which hasn't been merged, so it's manually patched in during the setup of the build environment.
  • General Build Improvements: The build script is now fully self-contained and can be run with Github actions, this includes automatic download of games/demos as well as generating the default scummvm.ini for a demo page. See chkuendig/scummvm-demo on how a ScummVM demo app can be built (incl. playable demo).
  • ScummvmFS: I added a custom filesystem layer which in the future can be extended to support range requests and persisting downloaded data for offline usage. (also, jvilk/BrowserFS is not maintained anymore)

Minor Improvements:

  • CLI arguments: It's now possible to pass CLI arguments as a anchor link. (this is used to generate the initial scummvm.ini file by opening http://localhost:8080/scummvm.html#--add --path=/games --recursive in a headless browser)
  • HiDPI fix: SDL_GetDisplayDPI has been added to SDL for emscripten with libsdl-org/SDL#5383 - but this hasn't been released yet so I switched to using an unreleased version of SDL during the setup of the build environment.
  • Gamepad API: There was a minor bug when opening the app on a non-secure context (i.e. HTTP instead of HTTPS) due to SDL wanting to use the Gamepad Web API, there's now a redirect to avoid this.
  • Parts of the closed PR #3686 are added to the SDL backend for fullscreen support, no exit buttons and proper URL opening.
  • Director Engine: Yield back to the browser in the mail loop

I also added myself to the credits. Let me know if there's anything else required for me to officially take the maintainer role for this port.

As before, the latest version is running at http://scummvm.kuendig.io/ (now automatically built with Github Actions, see chkuendig/scummvm-demo/).

chkuendig added a commit to chkuendig/scummvm-demo that referenced this issue Mar 21, 2022
@@ -27,6 +27,9 @@
#define FORBIDDEN_SYMBOL_EXCEPTION_system
#define FORBIDDEN_SYMBOL_EXCEPTION_random
#define FORBIDDEN_SYMBOL_EXCEPTION_srandom
#ifdef __EMSCRIPTEN__
Copy link
Member

@ccawley2011 ccawley2011 Mar 21, 2022

Choose a reason for hiding this comment

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

This should ideally be done in a subclass of OSystem_POSIX (or OSystem_SDL if you don't need XDG paths or fork/exec) rather than via ifdefs.

Copy link
Contributor Author

@chkuendig chkuendig Mar 21, 2022

Choose a reason for hiding this comment

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

I might have misunderstood something, but the consensus in #3686 seemed to have been to not add another backend class. So I'm not sure what is preferred - would it be better to add a separate emscripten backend?

Copy link
Member

@ccawley2011 ccawley2011 Mar 22, 2022

Choose a reason for hiding this comment

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

The issue with that PR is that it that it duplicates a lot of stuff from the POSIX backend that looks like it wouldn't be useful on Emscripten (namely the path handling, the Linux AudioCD code and the use of fork/exec for opening the log file). Since having multiple subclasses of OSystem_SDL is intended for this exact purpose, I'd recommend one of the following approaches:

  • If the POSIX-specific code is useful for Emscripten, then the Emscripten backend can inherit from OSystem_POSIX and override specific functions where needed.
  • If the POSIX-specific code doesn't work or is overridden by the JavaScript code, then the Emscripten backend can inherit directly from OSystem_SDL, with simplified versions of displayLogFile, addSysArchivesToSearchSet, getScreenshotsPath, getDefaultConfigFileName and getDefaultLogFileName where necessary.

Copy link
Member

@bluegr bluegr Mar 22, 2022

Choose a reason for hiding this comment

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

Totally agree with @ccawley2011's suggestions

Copy link
Contributor Author

@chkuendig chkuendig Mar 22, 2022

Choose a reason for hiding this comment

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

Thank you, I'll subclass OSystem_POSIX as most POSIX stuff can be reused - Emscripten is generally considered to be POSIX (and says so on emscripten.org).

Copy link
Contributor Author

@chkuendig chkuendig Apr 5, 2022

Choose a reason for hiding this comment

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

I pushed a change implementing a new backend. Let me know what you think.

AUTHORS Show resolved Hide resolved
dists/emscripten/build.sh Outdated Show resolved Hide resolved
configure Outdated Show resolved Hide resolved
configure Outdated Show resolved Hide resolved
chkuendig added a commit to chkuendig/scummvm that referenced this issue Mar 22, 2022
- Subclass OSystem_POSIX instead of adding #ifdefs to it
- Don't disable TinyGL by default
- Move linker flags into configure script
- Cleanup of README.md (+2 squashed commits)
- Adding myself to credits.pl
@chkuendig
Copy link
Contributor Author

@chkuendig chkuendig commented Mar 22, 2022

I incorporated the first set of feedback. (i'll squash the individual changes into the right commits before merging)

FYI: I also noticed an issue with TLJ crashing when moving beyond the in game menu into the game - this came down to some weirdness with glTexParameterf which I couldn't figure out. I switched to glTexParameteri as that a) fixed the issue, b) was the only place in all of ScummVM where glTexParameterf was used and c) is anyway the appropriate function since the parameters are integer enums (based on https://stackoverflow.com/questions/29919386/what-is-the-difference-between-gltexparameterf-and-gltexparameteri)

devtools/credits.pl Outdated Show resolved Hide resolved
@chkuendig
Copy link
Contributor Author

@chkuendig chkuendig commented Apr 4, 2022

I force-pushed a new set of (cleaned up) commits which should incorporate all feedback received so far and a few other improvements I made along the way.

chkuendig added a commit to chkuendig/scummvm-demo that referenced this issue Apr 9, 2022
chkuendig added a commit to chkuendig/scummvm-demo that referenced this issue Apr 9, 2022
chkuendig added a commit to chkuendig/scummvm-demo that referenced this issue Apr 9, 2022
@chkuendig chkuendig marked this pull request as draft Apr 9, 2022
chkuendig added a commit to chkuendig/scummvm-demo that referenced this issue Apr 9, 2022
chkuendig added a commit to chkuendig/scummvm-demo that referenced this issue Apr 12, 2022
Copy link
Member

@lephilousophe lephilousophe left a comment

Some comments after skimming this PR

configure Outdated Show resolved Hide resolved
configure Outdated Show resolved Hide resolved
@chkuendig
Copy link
Contributor Author

@chkuendig chkuendig commented Apr 24, 2022

FYI: I pushed a new revision of this PR. There's a few more things I might look into, but these things can wait.

Once I've tested the demo a bit more to discover any issue, I'd suggest this can be merged (I'll remove the draft flag accordingly)

@chkuendig chkuendig force-pushed the emscripten branch 2 times, most recently from bf01cba to 0a5c23a Compare Apr 25, 2022
@chkuendig chkuendig marked this pull request as ready for review Apr 25, 2022
@chkuendig chkuendig force-pushed the emscripten branch 2 times, most recently from 2318e21 to a4fadd0 Compare Apr 25, 2022
@@ -0,0 +1,44 @@
From 2ea11b4734de5656aaf3285145831e2bb9ad53ae Mon Sep 17 00:00:00 2001
Copy link
Member

@aquadran aquadran May 3, 2022

Choose a reason for hiding this comment

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

scummvm repo is not good place to keep distro patches

Copy link
Contributor Author

@chkuendig chkuendig May 3, 2022

Choose a reason for hiding this comment

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

I agree this is a hack. Is it general not a good idea to include patches (and instead rely on only stable versions), or is this particular one a blocker since it isn't even in the PR of the upstream project?

If it's the second, I would try to get this added to PR emscripten-core/emscripten#15893 (or start a second PR), so all patches have a corresponding PR. If we in general don't want to rely on unreleased code, this will mean:

  • Either keep this PR open until all changes are merged and released in upstream
  • Accept that certain things wont work with the merged code without manually applying these patches (this can be documented and a second PR could be put in draft until the changes all have landed)

The reason that these patches are checked into this repo, is because PRs aren't 100% stable (they are often rebased against master, which means they might not work against the stable release this script is depending on)

Copy link
Member

@aquadran aquadran May 3, 2022

Choose a reason for hiding this comment

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

you can create upstream forks with patches applied and point to them. or put patches somewhere on GitHub and point to them

Copy link
Contributor Author

@chkuendig chkuendig Jun 9, 2022

Choose a reason for hiding this comment

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

I cleaned up the patches in 037536e. All patches are now downloaded from Github (I still checked them in as well as PRs aren't 100% stable which could break the build).

Copy link
Member

@aquadran aquadran Jun 9, 2022

Choose a reason for hiding this comment

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

I still see patches in PR

Copy link
Contributor Author

@chkuendig chkuendig Jun 9, 2022

Choose a reason for hiding this comment

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

I wasn't aware that patches in general aren't allowed. I removed them and uploaded them to another repo (to ensure they stay stable and don't change should the PRs change)

Copy link
Member

@aquadran aquadran left a comment

scummvm repository is not good place to keep patches

@chkuendig chkuendig force-pushed the emscripten branch 3 times, most recently from ccd8e2c to 6b39daa Compare May 20, 2022
chkuendig added 8 commits Jun 8, 2022
…abling exit buttons (openURL has been fixed in libsdl-org/SDL@15ebad6 and is not needed anymore)
- Updated Emscripten to version 3.1.8 (+ additional patches)
- Support for dynamic plugins
- Adding ScummvmFS with support for HTTP Range Requests for game data
- Automated games/demos bundling and ini config generation during build
- Allow passing CLI arguments via fragment identifier of the website (i.e. scummvm.html#—debuglevel=9 )
- UI improvements with nicer status messages, splash screen + favicon
- Fixed HiDPI handling and responsiveness
- Bugfix: Don't crash if gamepad support isn't available
Copy link
Member

@aquadran aquadran left a comment

there are still patches in PR

@sev-
Copy link
Member

@sev- sev- commented Jun 12, 2022

Looks great and I am backporting it to 2.6.0. Will you be able to build it at the time of release?

@sev- sev- merged commit 9bf564b into scummvm:master Jun 12, 2022
8 checks passed
@chkuendig
Copy link
Contributor Author

@chkuendig chkuendig commented Jun 12, 2022

Yes, this is easily built. I have Github Actions actually. I wonder through what artifacts we would provide? Im currently hosting on Github Pages and cant get more demos onto that (already hitting some limits), OTOH providing a zip file to download doesnt make much sense either.

@neuromancer
Copy link
Contributor

@neuromancer neuromancer commented Jun 12, 2022

I made some quick PR to have this deployed: #3985. It will require some adjustments, but hopefully we have it merged to allow users to test 2.6.0 release candidate. Happy to include any improvements or suggestions from you!

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