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

Emscripten build improvements #658

Merged
merged 13 commits into from
Jun 10, 2024
Merged

Emscripten build improvements #658

merged 13 commits into from
Jun 10, 2024

Conversation

jdarpinian
Copy link

@jdarpinian jdarpinian commented May 3, 2024

EDIT: This PR used to include GL4ES but I've removed it now that GLES is supported directly. Check the latest comments for updates.

This adds a web build using Emscripten to build a wasm binary and GL4ES to translate OpenGL 1 to WebGL. The vast majority of this code is GL4ES, imported as a dependency. To review just the ioquake3 changes, which are minor besides the Makefile, check the "Web build support with Emscripten" commit 7618f10.

@jdarpinian jdarpinian force-pushed the main branch 2 times, most recently from b638909 to 3c29e0a Compare May 3, 2024 08:14
@jdarpinian
Copy link
Author

jdarpinian commented May 5, 2024

I made a GitHub Actions workflow for the web build. You can download the generated artifacts and try out the web build yourself! https://github.com/jdarpinian/ioq3/actions/runs/8964848533/artifacts/1475480879

To run the web build you'll need to start a local webserver that serves both your baseq3 directory and the ioquake.html file. Then load ioquake.html?pakPathname=/path/to/your/baseq3 in your browser and play!

@jdarpinian
Copy link
Author

jdarpinian commented May 6, 2024

I fixed a GL4ES issue that was causing the text in the console to be invisible as well as the mouse pointer in the main menu. Those were the only rendering issues I was aware of. As far as I know all rendering should be working 100%. I think this should be mergeable.

@briancullinan2

This comment was marked as abuse.

@NuclearMonster
Copy link
Member

Thanks James, this looks great in general, I have created #663 to look to the future and solve the underlying issue that causes ioq3 to require gl4es at this time, but a great solution now is better than the wishes and dreams of feature requests.

@NuclearMonster
Copy link
Member

Can't figure out why Github won't let me re-run the actions, will try closing and reopening the PR

@kungfooman
Copy link

Is this a clean copy of @ptitSeb's gl4es?

Very nice work!

@zturtleman Didn't you do a lot of changes that make rend2 nearly GLES2? I ported it to WebGL some years ago and I don't know what changed in the meantime, but my starting point was rend2 and it more or less worked with some glitches tho out of the box (IIRC it was picky about defining the correct shader versions and stuff like that mostly).

@zturtleman
Copy link
Member

@kungfooman yeah. I have a working OpenGL ES 2 port of the opengl2 renderer. #664

@jdarpinian
Copy link
Author

@briancullinan2 Does your WASI build run in the browser easily? I haven't profiled this build to see what's using CPU.

@NuclearMonster Thanks! I have noticed one issue that probably should be investigated before merging. I mentioned that I fixed a GL4ES issue that was causing console text to be invisible, in commit 7701715. Unfortunately the fix was to disable a big optimization that GL4ES pretty much relies on for performance. Although rendering is correct, it is slow on anything but very high-end hardware. So I think the underlying issue with that optimization ought to be fixed before merging this. Or, I see you just merged ES2 support so maybe we could just use that and drop GL4ES? It might be nice to have GL4ES anyway so both the GL1 and ES2 backends could be used.

@kungfooman Thanks! Yes, it's a clean copy of GL4ES, just with some nonessential files removed. The commit ID is in code/gl4es/README.ioq3.md

@NuclearMonster
Copy link
Member

Or, I see you just merged ES2 support so maybe we could just use that and drop GL4ES? It might be nice to have GL4ES anyway so both the GL1 and ES2 backends could be used.

With the unexpected ES2 PR thanks to Zack Middleton, I'd prefer to drop GL4ES for fewer dependencies and focus improvements on performance of the "OpenGL2/ES2" renderer if that looks possible. Sorry to disrupt your flow here. I really appreciate the work you put into bringing GL4ES over. Didn't expect to have wild dreams of dedicated ES2 support realized in a day!

@jdarpinian
Copy link
Author

Makes sense to me, GL4ES is a big dependency. I'll drop GL4ES from the PR when I have some time to spend on it.

@ptitSeb
Copy link

ptitSeb commented Jun 6, 2024

I'm curious to know if you see any performances changes when switching from gl4es to gles2.

@kungfooman
Copy link

I'm curious to know if you see any performances changes when switching from gl4es to gles2.

Yea, me too, I just tested rend1 and rend2 on a Raspberry Pi 4 yesterday and rend1 got around 60-80 FPS and rend2 only around 10 FPS... no clue what the bottle neck is, but now I also want to test:

  • Zack's new ioq3 OpenGL ES
  • rend1 + your gl4es

I don't wanna be too crazy, but I'm also thinking about libangle GLES to Vulkan... that may help performance as well.

@zturtleman
Copy link
Member

I added basic emscripten support with the OpenGL2 renderer in #666 (f41bd37). This isn't meant to deter other changes.

@briancullinan2

This comment was marked as abuse.

@briancullinan2

This comment was marked as abuse.

@briancullinan2

This comment was marked as abuse.

@NuclearMonster
Copy link
Member

In order to keep this PR on-topic, please continue any general discussions about the web version of ioquake3 that aren't specific to this PR to our forum: https://discourse.ioquake.org/t/ioquake3-web-version-improvements-discussion/1961

@jdarpinian
Copy link
Author

@zturtleman I just tried your ES renderer and it is faster than GL4ES, perhaps unsurprisingly. Very cool! I don't think we need to keep GL4ES as an option.

CPU time is low, but I do still see random long GPU frames. I'm not sure what's causing that.

@kungfooman
Copy link

CPU time is low, but I do still see random long GPU frames. I'm not sure what's causing that.

You mean in this PR or Zacks? Good thing about e.g. Chrome browser is that you can make a very detailed performance trace analysis: https://developer.chrome.com/docs/devtools/performance

zturtleman and others added 4 commits June 9, 2024 02:12
emscripten 3.1.27 reduced the stack size from 5MB to 64KB. This caused
run-time errors: Uncaught RuntimeError: index out of bounds

Co-authored-by: James Darpinian <[email protected]>
@jdarpinian jdarpinian changed the title Web build support with Emscripten and GL4ES Emscripten build improvements Jun 9, 2024
@jdarpinian
Copy link
Author

jdarpinian commented Jun 9, 2024

OK I've removed GL4ES from this PR. Still some changes remaining:

  • Support DWARF debug builds for Emscripten, which Chrome Dev Tools can debug
  • Add a web build to GitHub Actions and fix archiving web builds
  • Add the ability to load pk3 files from a web server at runtime in addition to the current mode that packages them at build time.
  • Add ioquake3.html to replace the ugly default Emscripten HTML loader. Allows setting command line arguments with URL params, among other things.
  • Fix stack size on latest emscripten
  • Update README.md with build instructions

@zturtleman
Copy link
Member

Okay so, personally I would rather merge this manually in pieces than request a lot of changes to the PR. Do you have a preference? (I also need to do some testing to see if --preload-file code path is worth keeping.)

I would also like to merge the STACK_SIZE fix first so the present build works with present emscripten.

@zturtleman
Copy link
Member

zturtleman commented Jun 9, 2024

I guess mainly I would like the STACK_SIZE fix as the first commit and for the commits undoing changes (like gl4es) or minor fixes to your other commits (like Fix web workflow) to be combined into the original commit. It would be nice if the commits were separate commits for logically separate high-level features similar to your list of changes. Other things can be changed later.

@jdarpinian
Copy link
Author

OK I rebased this change on your PR #669. I also removed noise commits and separated features into different commits. You can merge #669 first if you want.

@jdarpinian jdarpinian force-pushed the main branch 2 times, most recently from 6150708 to add1363 Compare June 9, 2024 19:09
@zturtleman zturtleman merged commit e0ce0ce into ioquake:main Jun 10, 2024
4 checks passed
@zturtleman
Copy link
Member

It looks good, thank 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
Development

Successfully merging this pull request may close these issues.

6 participants