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: fixes to get bgfx working #13255

Merged
merged 4 commits into from
Feb 6, 2025

Conversation

algestam
Copy link
Contributor

This PR adds support for the bgfx video backend when building for emscripten.

In order to get bgfx build and run with mame, the following changes has been done:

  • src/osd/modules/render/bgfx/shadermanager.cpp: Change make_path_string(...) to always return the path to the essl shaders. This wasn't working properly since the call to the function is done before bgfx has been initialised and therefore returned the renderer type Noop instead (see https://bkaradzic.github.io/bgfx/bgfx.html#_CPPv4N4bgfx15getRendererTypeEv).
  • src/osd/modules/render/drawbgfx.cpp: Change set_platform_data(...) to return platform data with a window handle that includes a HTML target selector for the canvas element to use (currently set to #canvas).
  • scripts/genie.lua: Update emscripten compiler flags to match recent emscripten versions and set memory allocation to work with the drivers I've been testing with. Also embedded a file, artwork/lut-default.png, which is required for the lut and hlsl chains.

Additional changes:

  • src/osd/modules/font/font_sdl.cpp: Change SDL_ttf include to use SDL2 since recent emscripten supports this.
  • docs/source/initialsetup/compilingmame.rst: Update emscripten version to use.

The change has been tested by me and works well with the drivers I've tested. The performance is greatly improved when using bgfx compared to the soft renderer and many drivers that struggled to maintain a decent framerate are now running smoothly at 100%.

image

All builds has been done using emscripten 3.1.35. When trying later versions various other issues has been encountered which I haven't addressed or investigated further.

Setting this as a draft and ask kindly for other Mame developers with more insight into the OSD and build system to please take a look at this PR and suggest how these changes can be improved.

@algestam algestam changed the title emscripten: Fixes to get bgfx working emscripten: fixes to get bgfx working Jan 21, 2025
@DopefishJustin DopefishJustin requested review from DopefishJustin and removed request for DopefishJustin January 22, 2025 00:04
@DopefishJustin
Copy link
Member

Looks OK functionality-wise. I was able to compile with Emscripten 3.1.35 and run with both software rendering and bgfx with shader chains. bgfx had been working previously but Emscripten and bgfx are both moving targets, so thanks for chasing the issues down!

@holub
Copy link
Contributor

holub commented Jan 22, 2025

Looks good here as well. I'm glad we can move forward from 3.1.25 (my latest emscripten version which only worked without noticeable problems with MAME)

@algestam algestam marked this pull request as ready for review January 23, 2025 08:28
@algestam
Copy link
Contributor Author

Thanks for testing the change out and other comments. bgfx has been working in the past but has been non-functional for a while. Changing the state of this PR to be ready for review.

@algestam
Copy link
Contributor Author

algestam commented Feb 4, 2025

Perhaps this can be merged as-is if you guys are fine with it. Otherwise as I said, PR is ready for review.

@holub
Copy link
Contributor

holub commented Feb 5, 2025

Sorry, we are experiencing bandwidth with review authorities atm. It will take a bit time to return on track. Please be patient unless PR is verbally declined.

@@ -395,6 +395,17 @@ bool video_bgfx::set_platform_data(bgfx::PlatformData &platform_data, osd_window
platform_data.ndt = nullptr;
platform_data.nwh = GetOSWindow(dynamic_cast<mac_window_info const &>(window).platform_window());
#else // defined(OSD_*)

#if defined(SDLMAME_EMSCRIPTEN)
Copy link
Member

Choose a reason for hiding this comment

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

Please go for #elif there, compilers can get cranky when you have a pile of stuff after a return and it's bad form in any case.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for the suggestion. The emscripten specific code has been updated to an #elif statement, making the code align with the approach used by other system for setting up bgfx 👍

@galibert galibert merged commit c1bddc6 into mamedev:master Feb 6, 2025
6 checks passed
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.

5 participants