Discussion PR: SDL3 port for cross-platform support

[ThePuzzlemaker]

There's been some talk of getting Orbiter to run on Linux previously, and I thought I'd take another stab at it. I've created a draft PR which will, once completed, implement a port to use SDL3 instead of the Windows API. D3D9Client will still be supported for Windows-only targets, though.

Feel free to ask me any questions or (kindly) berate my code here or (preferably) on GitHub: https://github.com/orbitersim/orbiter/pull/561

 

[Gondos]

Welcome to the forum
First, I think you'll find most people here have a neutral attitude toward having a native linux version or are satisfied with using wine.
If we introduce too many breaking changes in support of having a cross platform Orbiter without adding some value elsewhere, I'm not sure it willl receive a positive feedback.
Second, my linux work has been put on the backburner for 2 years now but was not abondonned, it was waiting for the official release which has now happened and here is the plan I had in mind afterward :

• integrate ImGui to prepare for future work (almost done)
• migrate core dialogs to ImGui (in progress, most of the work is already done in the linux branch so it's just a matter of cleaning things up)
• replace all usage of HDC in the core with Sketchpad (the DragonFly will be problematic because it uses OpenGL to render its navball to an HDC, replacing it with the ADIBall from the ShuttleA could help).
• introduce SDL (just enough to remove the DirectInput dependency and replace the event loop, I used glfw previously but it's not too different from SDL). If possible at this step, old win dialogs should still be working (oapiOpenDialog) but I haven't checked how to mix native HWND with SDL_Windows
• port the Launchpad to ImGui (at this point we break oapiRegisterLaunchpadItem compatibility). This is already done in the linux branch so apart from a community veto, it should not be an issue.

At this point it should not have introduced too much disruption with regards to the OAPI compatibility for addons developers.
Ideally we need community feedback before merging each of these steps, but most users here don't have a github account or the will/knowledge/time to compile branches.
@Xyon : do you know if it's possible to generate the binary zip for some PR branches before merging ? Or is it something that must be done in the contributor's repo ?

Next comes the tricky part, if the next version is 64bit as has been hinted, only Lua addons will work right off the bat, everything else will need to be recompiled.
Orbiter's main advantage is its community and amount of available addons. The switch to 64bit will necessitate a lot of work from the devs to recompile their addons, I'm afraid if we introduce breaking API changes on top of that, it just won't be done and people will stick with Orbiter 2024.
That's the main reason I tried to push as much Lua support as possible before the release, so that the option exists to make modules that have a chance to work with the next Orbiter iteration.
Some more steps required for cross platform support :

• rework the module API (special flags are needed for dlopen to work with plugins, not sure it's available with the SDL objects API)
• add a new Graphics Client. That's the biggest challenge in my opinion, I guess more than 60% of the time I spent on the linux branch was on the OpenGL client. I suck at it and if someone can work on a proper client, it would be nice. Beware, the most painful aspect is not drawing planets or vessels, it's implementing the Sketchpad API and having the MFDs look the same as with the current GC.
• some more...

In summary, I think the work to port to SDL3 should be limited to using SDL3, changes related to cross platform support should be done in other PRs. I'm not sure replacing the MSVC types at this point should be a priority. In theory yes, and it feels so good everytime you remove a #include <Windows.h>, in practice it introduces a lot of noise when you compare branches, it may also confuse addon devs but that's another issue.

 

[ThePuzzlemaker]

@Gondos thanks for your feedback!

If we introduce too many breaking changes in support of having a cross platform Orbiter without adding some value elsewhere, I'm not sure it willl receive a positive feedback.

This was my worry, and ultimately I do agree I might've gone a bit too far right now

If possible at this step, old win dialogs should still be working (oapiOpenDialog) but I haven't checked how to mix native HWND with SDL_Windows

I can make that work.

port the Launchpad to ImGui (at this point we break oapiRegisterLaunchpadItem compatibility). This is already done in the linux branch so apart from a community veto, it should not be an issue.

I suppose I've duplicated work then... oops; I'm willing to adopt your existing launchpad if you think it's better, of course, but there are some things I do particularly like about mine (particularly, the use of Markdown for scn descriptions, module info, and a few other things). Ultimately though I will probably split this into another PR once this is done, and I'll do my best to minimize the breaking changes I've already made to even fewer

special flags are needed for dlopen to work with plugins, not sure it's available with the SDL objects API

I was not aware of that, but I can probably make it work eventually. I think it's probably worth just bear the #ifdefs rather than trying to hack this into SDL's object API though.

In summary, I think the work to port to SDL3 should be limited to using SDL3, changes related to cross platform support should be done in other PRs. I'm not sure replacing the MSVC types at this point should be a priority. In theory yes, and it feels so good everytime you remove a #include <Windows.h>, in practice it introduces a lot of noise when you compare branches, it may also confuse addon devs but that's another issue.

Yeah in hindsight that makes sense entirely. I'm going to work on undoing some of those changes since they are probably not constructive at the moment.

Ultimately, with this feedback in mind, I'm probably going to copy over my existing work to a new branch, then cherry-pick and revert as necessary to get just the SDL3-relevant stuff into one PR. Then I'll also make another PR with the ImGui launchpad stuff (along with the scenario and module info changes, since those are semi-breaking changes that go along with it). Of course, if people prefer your launchpad @Gondos I'm not gonna be too salty

 

[Gondos]

Don't worry about the launchpad, mine was a placeholder, yours looks gorgeous
We'll have to synchronize for the option tabs because the code is shared between the launchpad and the ingame options dialog.

 

[ThePuzzlemaker]

Don't worry about the launchpad, mine was a placeholder, yours looks gorgeous
We'll have to synchronize for the option tabs because the code is shared between the launchpad and the ingame options dialog.

What I did is make an ImCtxBase struct which handles part of the ImGui setup (and common styling); in the Launchpad this is overridden to use SDLGPU functions to render stuff. To handle the issue of global contexts, I made an RAII wrapper WithImCtx which saves the current context, sets the new context, then restores the old context when it's destroyed. This is always passed by reference and cannot be copied (only std::moved), so it doesn't cause any issues. The Options tab uses WithImCtx and not WithLpImCtx (the Launchpad-specific context wrapper), allowing it to be used in both cases. You're welcome to steal that code for your ImGui branch of course

 

[ThePuzzlemaker]

Basically, graphics clients will introduce their own ImCtxBase subclass that uses whatever backend they use (DirectX, OpenGL, Vulkan, Metal, Win32, etc. etc. etc.) and then they can make their own WithImCtx subclass for any specific functions. It isn't the best solution (imo using templates would be a bit better) but using templates was a bit janky cause of the lack of C++20 concepts and also my lack of knowledge on them

I have yet to write the code to get D3D9Client working, but after I'm done reworking these PRs per your and jarmonik's feedback, I'll make sure it works and get all that stuff working again.

 

[ThePuzzlemaker]

Basically, graphics clients will introduce their own ImCtxBase subclass that uses whatever backend they use (DirectX, OpenGL, Vulkan, Metal, Win32, etc. etc. etc.) and then they can make their own WithImCtx subclass for any specific functions. It isn't the best solution (imo using templates would be a bit better) but using templates was a bit janky cause of the lack of C++20 concepts and also my lack of knowledge on them

I have yet to write the code to get D3D9Client working, but after I'm done reworking these PRs per your and jarmonik's feedback, I'll make sure it works and get all that stuff working again.

Looking more at it, I think your solution with the dialog manager works quite well; I'll integrate that into it. The main issue is just getting a WithImCtx in the gclient when necessary (i.e., for render window event handling...). For now I just have an oapi function but that might not be the best solution.

 

[ThePuzzlemaker]

Okay... I'm at a loss. For some reason with the new SDL3 window management code (see the PR, it's now up-to-date), everything is super overexposed in D3D9Client:


(These images are from the stock "Delta-glider/Docked at ISS" scenario)

@jarmonik any clues as to why this could be happening? Unfortunately since this is DX9 I can't use RenderDoc and I'm not aware of any similar tools :/

It doesn't happen in the splash screen which means it's probably not entirely my fault... maybe... I don't know...

 

[ThePuzzlemaker]

This appears to just be happening with bodies with atmospheres... but the D3D9Client.cfg and Orbiter.cfg I've copied from a working O2024 install, as well as the surface textures and other configs, and it's still happening.

 

[ThePuzzlemaker]

I've narrowed this down to just a subset of the atmospheric bodies: Earth, Mars, Jupiter, and Io

 

[Gondos]

Have you looked at how I did it on linux? To be able to draw the Launchpad I initialized the render window right at startup, not at scenario loading.
The drawback is that you have to restart to change the Graphics Client but I don't think that's that big of a deal. That way you don't have to juggle between several contexts.
For your blooming trouble, did you check the various D3D9 debug windows to see if the configuration is loaded properly?

 

[ThePuzzlemaker]

That's probably a good idea honestly. I'll look into it. As for the glowing atmospheres/terrain issue, it seems to be some weird interaction with SDLGPU I think, as when I removed that (so that the launchpad could be split into another PR), it isn't occurring anymore.

 

[ThePuzzlemaker]

Update: I've split the Launchpad into a separate branch (have yet to make it into a PR yet, though), and have ported keyboard and mouse input to use SDL3. This comes with one breaking change that if necessary, can be undone but is quite simple to port (just porting WM_ mouse event IDs to SDL_EVENT_MOUSE_ event IDs)

 

[jarmonik]

Update: I've split the Launchpad into a separate branch (have yet to make it into a PR yet, though), and have ported keyboard and mouse input to use SDL3. This comes with one breaking change that if necessary, can be undone but is quite simple to port (just porting WM_ mouse event IDs to SDL_EVENT_MOUSE_ event IDs)

So, you mean that an add-on would need to change WM_MOUSE events to a corresponding SDL_EVENTS ?

 

[ThePuzzlemaker]

So, you mean that an add-on would need to change WM_MOUSE events to a corresponding SDL_EVENTS ?

Yes. The translation is straightforward, and SDL is very well-documented so I doubt there will be much confusion. Take this diff for example: https://github.com/orbitersim/orbiter/pull/561/commits/a62ae38a051af2b868d2272c49af3dd2c3fedd28

Mainly I think any issues could stem from people not using the override qualifier on their functions, thus leading to the overridden function not being detected as an error when the signature is incorrect. But release notes could adequately solve that problem.

It took maybe 5-10 minutes maximum to port the D3D9Client event handling code to use SDL (which I think is probably the most complex out of any of these handlers; that handles more than mouse support anyway), but I'm willing to write a porting guide for this for the release notes if you need me to.

 

[ThePuzzlemaker]

Additionally, the OAPI_KEY_ constants have been redefined to their SDL scancode equivalents, as otherwise there's a lot of translation code in between that would be necessary, just complicating things and potentially slowing things down. Code that uses these definitions works as-is, but code that uses the keycode constants directly (i.e., 0x1A instead of OAPI_KEY_LBRACKET) will not be backwards-compatible. Additionally, code that, for example, loads numbers directly from a config file rather than translating names like Src/Orbiter/Keymap.{cpp,h}, will also not be backwards-compatible.

I did just realize I overlooked the keynames array in Src/Orbiter/Keymap.cpp, but that will be fixed in a future commit. Nevermind, that works as-is too.

 

[ThePuzzlemaker]

One thing that I will have to look into is linking with SDL. Right now every module has to link with it, but I'll definitely have to see if there's a way I can include it in the core Orbiter SDK. I'm prone to think that we both would agree that having to add another dependency to module compilation would be quite annoying and potentially unacceptable.

 

[ThePuzzlemaker]

One thing that I will have to look into is linking with SDL. Right now every module has to link with it, but I'll definitely have to see if there's a way I can include it in the core Orbiter SDK. I'm prone to think that we both would agree that having to add another dependency to module compilation would be quite annoying and potentially unacceptable.

It appears there is no easy way to do this. Because of how MSVC operates, re-exporting the symbols from a statically-linked SDL is non-trivial. Building the source directly with Orbiter is also non-trivial, because of how SDL is coded. I'm open to input on how you think we should proceed wrt this.

 

[ThePuzzlemaker]

Copying my reply on GitHub:

This PR is now ready for review, but will not be ready for merge until #550 (et seq.) are merged

This will probably need some shakeout testing to ensure it doesn't silently break certain things, but major functionality which was touched by this code is appearing to work well for me.

There is one unresolved FIXME, in Src/Orbiter/Orbiter.cpp:2004, which I'm open for suggestions on how to resolve; ultimately it's hard to simply force-destroy a std::shared_ptr<T> without crashing. I think a saner option might be to run CloseSession but I'm not familiar enough with Orbiter core code to know if that would cause issues if KillVessels failed.

 

[Gondos]

OK nice, I'll have a look at it.