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

[core] Add PLATFORM_WEB_SDL #3537

Closed
wants to merge 4 commits into from
Closed

[core] Add PLATFORM_WEB_SDL #3537

wants to merge 4 commits into from

Conversation

ghost
Copy link

@ghost ghost commented Nov 13, 2023

Changes

  1. Adds the PLATFORM_WEB_SDL. Does that by:

    1. Adding a new rcore_web_sdl.c to platforms (R1);
    2. Making the necessary adjustments on rcore.c (R17-R18, R240, R492-R493, R562-R563, R3052);
    3. Making the necessary adjustments on rlgl.c (R813, R2276).
  2. Adds PLATFORM_WEB_SDL to the Makefile (R18-R19, R119, R161, R169, R238-R241, R271, R314, R330, R349, R446-R448, R607).

  3. Adds PLATFORM_WEB_SDL to the examples Makefiles:

    1. Makefile: R18-R19, R91, R142, R168, R187, R206, R210, R256, R300-R302, R303, R318-R323, R433, R619, R644.
    2. Makefile.Web: R36-R37, R53-R56, R63, R114, R140, R159, R178, R182, R228-R230, R255-R257, R258, R274-R279, R348, R1134.
  4. Applies [core] Fix missing keys on PLATFORM_DESKTOP_SDL #3539 fix to PLATFORM_WEB_SDL (R77, R178-R199).

Notes

  1. rcore_web_sdl.c at this moment is almost the same as rcore_desktop_sdl.c and have been working great on my preliminary tests. I'll now go function by function testing and inspecting what or if something needs to be changed.

  2. As with PLATFORM_DESKTOP_SDL, the user should provide the SDL2 library.

Environment

  • Compiled on Linux (Ubuntu 22.04 64-bit) with SDL2 (2.28.5) and Emscripten (3.1.48).

  • Tested on Firefox (115.3.1esr 64-bit) and Chromium (117.0.5938.149 64-bit) running on Linux (Mint 21.1 64-bit).

Edits

  • 1: added line marks.
  • 2: added Makefile changes.
  • 3: added examples Makefiles changes; editing.
  • 4: added #3539 fix.

@ghost
Copy link
Author

ghost commented Nov 14, 2023

@raysan5 If you agree with this platform addition and changes, I think this batch is ready for merge. 👍

@raysan5
Copy link
Owner

raysan5 commented Nov 14, 2023

@ubkp thanks for working on this improvement, I see that at this moment this is mostly a replica of PLATFORM_DESKTOP_SDL, only some includes changes, most of the work is done on the building side, is it really required the addition of an additional platform? is there some specific web-sdl nuances to consider? As usual, my fears are on the maintenance side, two separate codebases requrie minor changes/reviews to be implemented twice.

Maybe the SDL web support can be addressed in some other way, in building side only with some extra flag (actually PLATFORM_WEB_SDL is already an extra flag).

I keep this PR open for discussion as long as you are also testing it but for now, I'm not merging more big changes until raylib 5.0 gets released.

@ghost
Copy link
Author

ghost commented Nov 14, 2023

@raysan5 It could share the same code as rcore_desktop_sdl.c, but:

  1. Right from the start, we'd have to #ifdef out all that OpenGL code, since for web we're limited to ES2/ES3.

  2. Anything threaded (if added) would also have to be #ifdef out (as discussed on other issues).

  3. Given how fast/frequent web changes, it's hard to guess what would have to be changed in the future.

  4. SDL also has some web-specific hints that could be needed eventually.

  5. We could end up with a situation similar to the old rcore.c. Also, it kind of defeats the purposed of spliting by platform.

  6. Regarding maintenance, if anything, it would be the easiest to maintain, since maintaining one basically maintains both. As the rcore_desktop_sdl.c and rcore_web_sdl.c share almost everything, a single diff easily keep both in sync.

  7. In the end I don't expect rcore_web_sdl.c to diverge much from rcore_desktop_sdl.c, after all, that's the point of using SDL. Both are likely to always remain 95% the same.

  8. Edit: SDL also supports a sizeable amount of platforms. Any of those would also likely fall in this situation. Sharing almost everything with rcore_desktop_sdl.c but diverging in just key points. They all could be added to rcore_desktop_sdl.c, but we'd definitely end up on the old rcore.c situation.

Would you like me to proceed?

@ypujante
Copy link

ypujante commented Nov 14, 2023

It seems that there are changes to Makefile but not the other build frameworks (like CMake).

I think it is a good "example" of what is necessary to add a new platform as it clearly shows that there are multiple pieces to change (like rlgl.h) and that probably need to be revisited as it is not really practical to have to touch many parts to add a platform.

One way that comes to mind would be to have a platform definition file (format tbd, but simple), that could define what it needs and that would be turned (via a small tool) into the proper defines that way the code could remain generic. Something like:

// instead of this
#if defined(PLATFORM_DESKTOP) || defined(PLATFORM_DESKTOP_SDL) || defined(PLATFORM_WEB_SDL)

// use this (the tool adds the PLATFORM_USES_OPENGL_ES2 based on the platform definition file
#if defined(PLATFORM_USES_OPENGL_ES2)

If a new platform gets added and it uses OPENGL_ES2 the generic code does not need to change...

That is clearly beyond the scope of this PR but I find this PR interesting for that reason...

@ghost
Copy link
Author

ghost commented Nov 14, 2023

  1. AFAIK, only Makefile is "officially" supported. Cmake is user maintained.

  2. Not really. The only change actually required to rlgl.h is R813, which is absolutely minimal and just necessary because web's ES limitation.

  3. The changes rcore.c are related to timing (unavoidable) and the required rcore_web_sdl.c include (also, obviously, unavoidable).

  4. New platforms will never be completely just plug-in. And the amount of changes to entertain such raises complexity tenfold needlessly.

@ypujante
Copy link

11. The changes rcore.c are related to timing (unavoidable) and the required rcore_web_sdl.c include (also, obviously, unavoidable).

True in this instance without any changes, but not "obviously, unavoidable" in the future. I can think of another way to do it:

// PLATFORM_INCLUDE_IMPLEMENTATION is a define containing the c file implementation (replaces 16 lines of code in rcore.c...)
#include PLATFORM_INCLUDE_IMPLEMENTATION

@ghost
Copy link
Author

ghost commented Nov 14, 2023

Actually, nevermind, I'm out.

I'm spending more time having to argue against overengineering or suboptimal changes than actually fixing code. And what's the point of fixing something only to watch software regression later.

It's much easier to just continue on my own.

Good luck.

P.S.: please don't reopen this.

@ghost ghost closed this Nov 14, 2023
@ghost ghost deleted the add/web-sdl branch November 14, 2023 13:44
@ypujante
Copy link

@ubkp For what it is worth, I wasn't arguing about your changes, on the contrary I was just pointing out that this was a very interesting example that demonstrates what was truly required to add a new platform in the current version of raylib. And I was merely suggesting that, if interested, there are options to do it better, way down the line in the future (raylib 6.0?). Not let's change the world before applying your PR...

@Peter0x44
Copy link
Contributor

I don't mean it in a negative way, but this seems kind of pointless, compared to the maintainance cost it adds, it's not exposing the web APIs directly in a way that matters
What benefit does it have over library_glfw.js?

I think ideal would be a PLATFORM_WEB that doesn't use either of those, but emscripten APIs directly. Though, I don't know what kind of effort that would take, and whether it would be worth it either
Keeping the glfw status quo is the most frictionless way to go and doesn't have much downside, right?

@raysan5
Copy link
Owner

raysan5 commented Nov 14, 2023

@ubkp I'm sorry to read your last message and I don't understand how the conversation derived to this point.

You did an amazing work in those last months and if today the platform-split is a reality is thanks to your contributions. Hopefully you continue contributing to the project in a future.

At the moment I'm quite overload with the preparation of the new raylib 5.0 release but we can talk about it after that.

In any case, this kind of board-style asynchronous conversation could be a bit difficult to follow and discuss important topics, feel free to write me to ray@raylib.com or find me on Discord raylib community.

@ypujante
Copy link

@Peter0x44 I personally believe there are advantages over library_glfw.js . Looking at @ubkp implementation, I can see that a lot of calls are now implemented because they have an SDL equivalent where in the glfw implementation they are marked non implemented.

Also from my experience looking at the emscripten code, SDL seems to be more supported than GLFW. For example if you look at the number of tests for SDL vs GLFW, there is a clear winner.

Looking at the emscripten documentation:

Emscripten implements the Simple DirectMedia Layer API (SDL) for the browser environment, which provides low level access to audio, keyboard, mouse, joystick, and graphics hardware. Applications that use SDL typically require no input/output changes to run in the browser.

In addition, we have more limited support for glut, glfw, glew and xlib.

which is a clear indication that the most supported platform is SDL.

I think there is a very valid case for raylib to experiment with a different web platform implementation, like this one based on SDL. Especially since, with the platform split, it doesn't affect the other platforms. I also think that waiting after raylib 5.0 is definitely the way to go though.

This pull request was closed.
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.

None yet

3 participants