-
-
Notifications
You must be signed in to change notification settings - Fork 529
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
Conversation
b638909
to
3c29e0a
Compare
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! |
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. |
This comment was marked as abuse.
This comment was marked as abuse.
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. |
Can't figure out why Github won't let me re-run the actions, will try closing and reopening the PR |
Is this a clean copy of @ptitSeb's Very nice work! @zturtleman Didn't you do a lot of changes that make |
@kungfooman yeah. I have a working OpenGL ES 2 port of the opengl2 renderer. #664 |
@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 |
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! |
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. |
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:
I don't wanna be too crazy, but I'm also thinking about libangle GLES to Vulkan... that may help performance as well. |
This comment was marked as abuse.
This comment was marked as abuse.
This comment was marked as abuse.
This comment was marked as abuse.
This enables several things: * Optionally load pk3 files from a web server at runtime instead of bundling them with Emscripten at build time * Set command line arguments via URL param * It's not ugly
This comment was marked as abuse.
This comment was marked as abuse.
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 |
@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. |
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 |
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]>
Co-authored-by: James Darpinian <[email protected]>
Co-authored-by: James Darpinian <[email protected]>
OK I've removed GL4ES from this PR. Still some changes remaining:
|
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. |
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. |
6150708
to
add1363
Compare
It looks good, thank you. |
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.