Port hlsdl to SDL3

[nspitko]

This is a fairly large/spooky change, but SDL3 offers a lot of features that are extremely useful for modern cross platform development, while also making their existing APIs a lot less gross.

For context: https://wiki.libsdl.org/SDL3/NewFeatures

This has been reworked to be API compatible. The only exception is a change in how controllers are indexed, which will require a code change in heaps to make controllers reliably detect (They still work, but may not pick up without a hot plug in some situations)

heaps PR: HeapsIO/heaps#1302

I'm able to run my jam games without issue, including the multi-window imgui tooling.

Overall the porting cost is extremely low due to how well we're able to hide changes in the hdll. I believe what cost we are going to incur here is well worth the benefits SDL3 offers, especially as we start implementing the new APIs.

I'm opening this as a draft to get more feedback and testing on this. I've tested most of the features I have code coverage for in my own works, but I'd like to get some other heaps users to sign off that their stuff isn't exploding either.

[tobil4sk]

I think it's not worth making breaking changes to the api unless all of them come at once and some other things are cleaned up as well.

For now I think it would be best to keep the signatures the same for the existing exposed functions. This would reduce the update cost and mean that you can just swap in the new sdl.hdll to an existing app to use sdl3. Where the sdl3 api is different from sdl2, we can take inspiration from sdl2-compat, e.g.:
https://github.com/libsdl-org/sdl2-compat/blob/2b0faaa1ea3c2b8275a08e3ae2a1be58642e23de/src/sdl2_compat.c#L9393

[nspitko]

Honestly I'd rather avoid that. I understand the desire to avoid breaking changes, but I've hidden most of the pain, the stuff that's been changed is largely either a significant alteration to the API that can't be easily hidden (The controller changes), or is actively fixing problems that were affecting me already (SDL window related functions not taking window pointers, making multi-window apps problematic)

I can probably reduce a break or two here and there with defaults, but I don't think this is going to be worth the time to merge if we're not willing to accept some level of API breakage, and I don't think there's any world in which a total rewrite of hlsdl makes sense right now, given the work it would put on heaps.

I fully agree that the sdl implementation in this lib is a bit of a mess. That said, right now it basically only exists to enable heaps and libs based off heaps, so unless we're going to get sign-on from the heaps side to massively rework the SDL backend, I don't see that going anywhere. The PR on the heaps is very small and heaps users already deal with API breakages regularly between sdl and fmt.

[tobil4sk]

the stuff that's been changed is largely either a significant alteration to the API that can't be easily hidden (The controller changes)

Couldn't we copy the sdl2-compat implementation? https://github.com/libsdl-org/sdl2-compat/blob/2b0faaa1ea3c2b8275a08e3ae2a1be58642e23de/src/sdl2_compat.c#L10689.

actively fixing problems that were affecting me already (SDL window related functions not taking window pointers, making multi-window apps problematic)

This can be solved by adding new functions prefixed with win_ and placing them with the other win functions. That makes the names more consistent and that way we can keep the old functions for backwards compatibility, deprecating them.

[nspitko]

Reworked things to not break API. I'm not going to go as far as to ensure no behavior has changed; doing this "right" would mean pulling in thousands of lines from sdl_compat.c and that's just way outside of the scope.

From my testing now everything works as it did before in heaps, cerastes, and imgui without code changes. The one exception is controllers require a fix in heaps due to the ID changes to detect reliably on all platforms. Fixing this requires forcing hlsdl to internally manage controllers so it can hold a stable index, which is beyond the scope of what I'm willing to do here. We should just make the code change in heaps.

this does NOT add any new functions (outside of SDL_Error which is/was useful for debugging issues that crop up during porting), those will be done in a separate PR to limit the scope of this change as much as possible.

[tobil4sk]

Reworked things to not break API

Thanks, that looks really good!

Fixing this requires forcing hlsdl to internally manage controllers so it can hold a stable index

Not sure I fully understand the details or why we'd need to manage anything. Can't we use SDL_GetJoysticks to map from index to id? Like this: tobil4sk@ffa1f61

We should just make the code change in heaps.

I think if heaps code has to change, then heaps should use a new function to directly receive the array of joystick ids and iterate using that, rather than building up the array of ids manually.

var gamepadIds = @:privateAccess GameController.getGamepadIds();
for( id in gamepadIds )
    initPad( id );

This requires less intermediate state and is closer to the raw sdl3 api, as well as more similar to the old heaps code.

[nspitko]

Not sure I fully understand the details or why we'd need to manage anything. Can't we use SDL_GetJoysticks to map from index to id? Like this: tobil4sk@ffa1f61

Because the return order of SDL_GetJoysticks is not stable across all platforms. In my testing it very specifically was not stable and did not work properly.

I'm explicitly not adding new SDL3 functions to the haxe API in this PR. The heaps change fixes the bug without breaking API, and is compatible with both SDL2 and SDL3. This only even exists because heaps doesn't initialize controllers during launch, so if there are any other consumers using this API they likely aren't affected.

I totally understand the desire to sdl3-ify stuff and clean up the API but that's out of scope for this change.

[tobil4sk]

Because the return order of SDL_GetJoysticks is not stable across all platforms. In my testing it very specifically was not stable and did not work properly.

Ah ok thanks for the explanation. So in one call to SDL_GetJoysticks you might get JoystickA, JoystickB and then a couple lines later in another call JoystickB, JoystickA?

In that case can we just store the joystick array statically after the gctrl_count call so it is then stable for subsequent calls to gctrl_open? This seems to be sufficient for sdl2-compat, so I think it should be a fairly reasonable solution.

I totally understand the desire to sdl3-ify stuff and clean up the API but that's out of scope for this change.

Understood, however, I'm not sure there is much point in making heaps compatible with this in-between state where the sdl2 count-based iteration has been broken, but the sdl3 api to replace it hasn't been exposed yet.

[nspitko]

In that case can we just store the joystick array statically after the gctrl_count call so it is then stable for subsequent calls to gctrl_open? This seems to be sufficient for sdl2-compat, so I think it should be a fairly reasonable solution.

This is fragile and relies on very specific calling conventions. heaps has multiple paths for initializing joysticks depending on whether it's at launch or at runtime, the latter of which never calls gctrl_count. You'd need to internally build and track them during the SDL connect/disconnect events, then have gctrl_count return the arraysize here; but this still doesn't solve for cases where the controller count changes between event dispatch and the gctrl_open call. Some composite devices do weird stuff like this; I imagine this is why the API was changed in the first place.

Understood, however, I'm not sure there is much point in making heaps compatible with this in-between state where the sdl2 count-based iteration has been broken, but the sdl3 api to replace it hasn't been exposed yet.

Seems fine to me? I don't expect anyone is going to be in a hurry to rewrite the heaps controller code even if we expose the SDL3 controller API immediately. It works fine, minus this one bug we're about to create. The fix is simple and doesn't break sdl2 compat.

[nspitko]

After thinking on it a bit, I've added just SDL_GetJoysticks under a separate section that can be expanded later. The Heaps PR is now just a version check that will use the new API if available, else the old path is used.

[tobil4sk]

After thinking on it a bit, I've added just SDL_GetJoysticks under a separate section that can be expanded later. The Heaps PR is now just a version check that will use the new API if available, else the old path is used.

Seems like a good compromise to me. It's annoying that the old behaviour has to break but I guess this is the best option, and as others have said this is not the first breaking change in a hashlink library.

Haxe side if we change the type to an abstract type JoystickID, we can prevent users from trying to use invalid integers as ids, which will make it more obvious that the old method is broken. Also, perhaps heaps wants to call SDL_GetGamepads instead of SDL_GetJoysticks now that SDL3 provides that. I guess these things can come in follow up PRs.