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

Upgrade to SDL3 #6234

Merged
merged 45 commits into from
Apr 9, 2024
Merged

Upgrade to SDL3 #6234

merged 45 commits into from
Apr 9, 2024

Conversation

Susko3
Copy link
Member

@Susko3 Susko3 commented Apr 8, 2024

osu! changes: https://github.com/ppy/osu/compare/master...Susko3:osu:wip-SDL3?expand=1

Changes

  • (SDL3-CS) from namespace SDL2 & class SDL to namespace SDL & class SDL3 (made more sense as the functions are imported from SDL3.dll)
  • (SDL3-CS) structs and enums now live in the namespace instead of being nested in the SDL3 class
  • (SDL3-CS) pointer types are now proper pointers instead of IntPtr, which means more "unsafe" code
  • (SDL3-CS) minor changes to return/struct types, reducing need for casts
  • (SDL3-CS) simple function signature changes: from ref, in & out to pointers (unfortunate, but automatically generating these would be hell)
  • (SDL3-CS) function that take strings now take ReadOnlySpan<byte>, requiring Encoding.UTF8.GetBytes() calls or UTF-8 string literals
  • (SDL3-CS) function pointers instead of delegates in native callback functions
  • simple renames done via SDL-provided build-scripits/rename_symbols.py
  • migrations from the Migrating to SDL 3.0 guide
  • minor update of SDL3ControllerBindings logic (basically per SDL3 migration guide)
  • removal of borderless fullscreen on macOS and Linux (merged into fullscreen with SDL3)
  • updated windowing logic to account for SDL_DisplayID instead of previously used display index
  • osu!framework DisplayMode.RefreshRate changed from int to float
  • SDL relative mouse mode instead of custom WindowsMouseHandler mouse logic
    • including removal of custom tablet handling logic. SDL_pen.h would be the replacement for that, but it's currently only supported on Linux
  • updated WindowsWindow to account for SDL_SYSWMEVENT removal
    • has caused some minor regressions in IME handling (the new method isn't giving us all the expected messages)
  • updated JetBrains.Annotations to latest version
  • renamed SDL2 → SDL3

Issues

  • IOpenGLGraphicsSurface.BackbufferFramebuffer used to return useful information on iOS
  • minor IME regressions on windows
  • releasing mouse button outside window with relative mouse mode causes next click to be ignored (SDL issue, it seems)
  • (possible issue) non-OTD tablet input with relative mouse mode on windows (WindowsMouseHandler logic was removed)

Future work

Testing

  • windows
  • linux (x11)
  • linux (wayland)
  • macos
  • ios
  • android

I've tested mouse, keyboard

Susko3 added 30 commits April 7, 2024 12:24
This helps with migrating enums, as ClangSharp doesn't put them in the `SDL3` class.
This renames SDL2 symbols to SDL3 names.
Also adds a bunch of `unsafe` /shrug
`WindowsMouseHandler` is not updated as it's on the chopping block.
This is done by SDL now, and the raw input event don't get reported by `SDL_SetWindowsMessageHook`.
Both osuTK and SDL3 use floats.
Still doesn't fix all compile problems.
@smoogipoo
Copy link
Contributor

Code quality wise this looks good to me.

@smoogipoo
Copy link
Contributor

smoogipoo commented Apr 8, 2024

Linux: Tested on Wayland, and via XWayland, with the configs:

OSU_GRAPHICS_RENDERER=deferred OSU_GRAPHICS_SURFACE=vulkan SDL_VIDEO_DRIVER=wayland
OSU_GRAPHICS_RENDERER=deferred OSU_GRAPHICS_SURFACE=vulkan SDL_VIDEO_DRIVER=x11
OSU_GRAPHICS_RENDERER=opengl OSU_GRAPHICS_SURFACE=opengl SDL_VIDEO_DRIVER=wayland
OSU_GRAPHICS_RENDERER=opengl OSU_GRAPHICS_SURFACE=opengl SDL_VIDEO_DRIVER=x11

Haven't tested native X11, but that's probably okay. Resolved one issue in the process.

@smoogipoo
Copy link
Contributor

smoogipoo commented Apr 8, 2024

iOS: OpenGL is broken, but we've disabled that renderer. Metal works fine.

@smoogipoo
Copy link
Contributor

smoogipoo commented Apr 8, 2024

Android: Works fine.

@smoogipoo smoogipoo requested a review from peppy April 8, 2024 17:36
@smoogipoo
Copy link
Contributor

@peppy Maybe look to merge after the next release?

@peppy
Copy link
Member

peppy commented Apr 9, 2024

I wouldn't be against getting it in this week's release, thoughts?

@smoogipoo
Copy link
Contributor

Can give it a shot. My concern would be the issues mentioned in the OP that I'm not fully sure the scope of.

@peppy
Copy link
Member

peppy commented Apr 9, 2024

Yeah let's leave it one release.

Copy link
Member

@peppy peppy left a comment

Choose a reason for hiding this comment

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

Code quality pass ✅
Tested on macOS ✅

@@ -29,11 +29,11 @@ internal LinuxGameHost(string gameName, HostOptions? options)

protected override void SetupForRun()
{
SDL.SDL_SetHint(SDL.SDL_VIDEO_X11_NET_WM_BYPASS_COMPOSITOR, BypassCompositor ? "1" : "0");
SDL3.SDL_SetHint(SDL3.SDL_HINT_VIDEO_X11_NET_WM_BYPASS_COMPOSITOR, BypassCompositor ? "1"u8 : "0"u8);
Copy link
Member

Choose a reason for hiding this comment

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

Never seen this before 🤯

@peppy
Copy link
Member

peppy commented Apr 9, 2024

I think we can merge this at any point. We won't be doing another framework release / update before the osu! release this week.

That said, I'd probably want @bdach to sign off on this one at whatever depth he feels comfortable doing so. A bit of secondary windows testing is probably most important here, if anything.

@bdach
Copy link
Collaborator

bdach commented Apr 9, 2024

I'm just going to test that this launches in all environments that I have and call it a day. I don't think further review is productive.

Copy link
Collaborator

@bdach bdach left a comment

Choose a reason for hiding this comment

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

doesn't die. have not read code. expecting 30 issues that were really upstream issues to be fixed and another 30 to regress

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants