Bring back SDL2 by default, lock SDL3 behind environment variable #6292

smoogipoo · 2024-05-21T11:06:20Z

Environment variable: OSU_SDL3=1.

The process of making this PR involved:

Going through every file in Upgrade to SDL3 #6234
For every file that's changed, grab the original from https://github.com/ppy/osu-framework/tree/20d55fad04f78f36aafa3036c5540ed3bcd2ddc1

All of which culminates in a 99% 1:1 reimplementation of SDL2. Only supporting logic has been added. We should be able to delete the SDL2 pathway with little effort at any point.

As far as osu! is concerned, this is a non-breaking change.

Important changes/things to look out for:

Namespace osu.Framework.Platform.SDL -> osu.Framework.Platform.SDL3.
- Platform.SDL2 added to hold SDL2 variant classes.
- Can possibly combine both variants back into the SDL namespace if people believe that's better?
SDL3GameHost -> SDLGameHost.
- It now creates SDL2/SDL3 window variants depending on env variable.
WindowMouseHandler contains both SDL2 and SDL3 implementations.
- Required to not break deserialisation of input.json thus losing settings.
SDL3ReadableKeyCombinationProvider does not have an SDL2 variant.
- Since the keycodes look identical, I think it's fine to use SDL3's implementation.
using static SDL2.SDL and using static SDL.SDL3 are used to avoid namespace conflicts.
Both iOS and Android use SDL3.

Testing:

Using `using SDL.SDL3` to fix namespace conflicts.

smoogipoo · 2024-05-21T11:23:19Z

Not sure about the DesktopGameHost : SDLGameHost inheritance change. Only just realised DesktopGameHost is used in a few other places, like tests. It looks fine and passes the CI tests, but the prospect is a bit scary. Hard to do this otherwise...

Edit: Nevermind, I think I was thinking about a prior version of my changes. Nothing's actually changed in this PR.

Susko3

Tested to boot fine on Android, and Windows SDL2 & SDL3.

Susko3 · 2024-05-21T12:39:40Z

osu.Framework/Platform/SDL3/SDL3ReadableKeyCombinationProvider.cs

Readable key combinations don't work on SDL2. Can be easily fixed in an ugly way as the SDL3 and SDL2 enums are compatible.

diff --git a/osu.Framework/Platform/SDL3/SDL3ReadableKeyCombinationProvider.cs b/osu.Framework/Platform/SDL3/SDL3ReadableKeyCombinationProvider.cs index 126a73e56..9d8d81dab 100644 --- a/osu.Framework/Platform/SDL3/SDL3ReadableKeyCombinationProvider.cs +++ b/osu.Framework/Platform/SDL3/SDL3ReadableKeyCombinationProvider.cs @@ -11,9 +11,25 @@ namespace osu.Framework.Platform.SDL3 { public class SDL3ReadableKeyCombinationProvider : ReadableKeyCombinationProvider { + private static SDL_Keycode getKeyFromScancode(SDL_Scancode scancode) + { + if (FrameworkEnvironment.UseSDL3) + return SDL_GetKeyFromScancode(scancode); + + return (SDL_Keycode)global::SDL2.SDL.SDL_GetKeyFromScancode((global::SDL2.SDL.SDL_Scancode)scancode); + } + + private static string? getKeyName(SDL_Keycode keycode) + { + if (FrameworkEnvironment.UseSDL3) + return SDL_GetKeyName(keycode); + + return global::SDL2.SDL.SDL_GetKeyName((global::SDL2.SDL.SDL_Keycode)keycode); + } + protected override string GetReadableKey(InputKey key) { - var keycode = SDL_GetKeyFromScancode(key.ToScancode()); + var keycode = getKeyFromScancode(key.ToScancode()); // early return if unknown. probably because key isn't a keyboard key, or doesn't map to an `SDL_Scancode`. if (keycode == SDL_Keycode.SDLK_UNKNOWN) @@ -25,7 +41,7 @@ protected override string GetReadableKey(InputKey key) if (TryGetNameFromKeycode(keycode, out name)) return name; - name = SDL_GetKeyName(keycode); + name = getKeyName(keycode); // fall back if SDL couldn't find a name. if (string.IsNullOrEmpty(name))

Good catch. Although it's a bit messy, I think this is fine for now.

Susko3 · 2024-05-21T13:18:32Z

osu.Framework/Platform/Windows/WindowsMouseHandler.cs

Could be split into multiple files containing the same partial class. Makes removal easier and better separates the SDL2 and generic implementation. (Plus the future SDL3 impl for #6291.)

The SDL2 impl should then use window is (not) SDL2WindowsWindow :-)

Ditto for Native.Input.

I've tried to do this in 6f0702a, I hope it's what you intended as I'm unsure whether the wndproc handler can truly be shared between the two so I just assumed that it can.

Native.Input isn't sdl2/sdl3 specific and it's internal. I'm trying to avoid making too many changes and just followed the two steps in the PR description that can be verified.

The wndproc handler doesn't work for raw input on windows. I meant to split out the code so that the SDL2 part can just be deleted. Best illustrated by the below diff. With it, the changes of WindowsMouseHandler.cs relative to master is the smallest. There's no SDL3 code yet so that partial is removed.

Expected final diff to master:

diff --git a/osu.Framework/Platform/Windows/WindowsMouseHandler.cs b/osu.Framework/Platform/Windows/WindowsMouseHandler.cs index b74535b78..4abe150a6 100644 --- a/osu.Framework/Platform/Windows/WindowsMouseHandler.cs +++ b/osu.Framework/Platform/Windows/WindowsMouseHandler.cs @@ -1,4 +1,4 @@ -// Copyright (c) ppy Pty Ltd <[email protected]>. Licensed under the MIT Licence. +// Copyright (c) ppy Pty Ltd <[email protected]>. Licensed under the MIT Licence. // See the LICENCE file in the repository root for full licence text. using System.Runtime.Versioning; @@ -12,16 +12,22 @@ namespace osu.Framework.Platform.Windows /// This is done to better handle quirks of some devices. /// </summary> [SupportedOSPlatform("windows")] - internal class WindowsMouseHandler : MouseHandler + internal partial class WindowsMouseHandler : MouseHandler { - private WindowsWindow window = null!; + private IWindowsWindow window = null!; + + public override bool IsActive => Enabled.Value; public override bool Initialize(GameHost host) { - if (!(host.Window is WindowsWindow desktopWindow)) + if (host.Window is not IWindowsWindow windowsWindow) return false; - window = desktopWindow; + window = windowsWindow; + + if (window is SDL2WindowsWindow) + initialiseSDL2(host); + return base.Initialize(host); }

Diff

diff --git a/osu.Framework/Platform/Windows/WindowsMouseHandler.cs b/osu.Framework/Platform/Windows/WindowsMouseHandler.cs index ef2cdcaad..4abe150a6 100644 --- a/osu.Framework/Platform/Windows/WindowsMouseHandler.cs +++ b/osu.Framework/Platform/Windows/WindowsMouseHandler.cs @@ -1,14 +1,8 @@ // Copyright (c) ppy Pty Ltd <[email protected]>. Licensed under the MIT Licence. // See the LICENCE file in the repository root for full licence text. -using System; -using System.Drawing; using System.Runtime.Versioning; -using osu.Framework.Extensions.EnumExtensions; using osu.Framework.Input.Handlers.Mouse; -using osu.Framework.Input.StateChanges; -using osu.Framework.Platform.Windows.Native; -using osu.Framework.Statistics; using osuTK; namespace osu.Framework.Platform.Windows @@ -20,17 +14,7 @@ namespace osu.Framework.Platform.Windows [SupportedOSPlatform("windows")] internal partial class WindowsMouseHandler : MouseHandler { - private static readonly GlobalStatistic<ulong> statistic_relative_events = GlobalStatistics.Get<ulong>(StatisticGroupFor<WindowsMouseHandler>(), "Relative events"); - private static readonly GlobalStatistic<ulong> statistic_absolute_events = GlobalStatistics.Get<ulong>(StatisticGroupFor<WindowsMouseHandler>(), "Absolute events"); - private static readonly GlobalStatistic<ulong> statistic_dropped_touch_inputs = GlobalStatistics.Get<ulong>(StatisticGroupFor<WindowsMouseHandler>(), "Dropped native touch inputs"); - - private static readonly GlobalStatistic<ulong> statistic_inputs_with_extra_information = - GlobalStatistics.Get<ulong>(StatisticGroupFor<WindowsMouseHandler>(), "Native inputs with ExtraInformation"); - - private const int raw_input_coordinate_space = 65535; - private IWindowsWindow window = null!; - private bool handlerBound; public override bool IsActive => Enabled.Value; @@ -40,9 +24,9 @@ public override bool Initialize(GameHost host) return false; window = windowsWindow; - handlerBound = window is SDL2WindowsWindow - ? bindHandlerSDL2(host) - : bindHandlerSDL3(host); + + if (window is SDL2WindowsWindow) + initialiseSDL2(host); return base.Initialize(host); } @@ -52,108 +36,5 @@ public override void FeedbackMousePositionChange(Vector2 position, bool isSelfFe window.LastMousePosition = position; base.FeedbackMousePositionChange(position, isSelfFeedback); } - - protected override void HandleMouseMoveRelative(Vector2 delta) - { - if (handlerBound) - { - // When bound, relative movement is reported via the WndProc handler below. - return; - } - - base.HandleMouseMoveRelative(delta); - } - - private unsafe IntPtr onWndProcSDL2(IntPtr userData, IntPtr hWnd, uint message, ulong wParam, long lParam) - { - if (!Enabled.Value) - return IntPtr.Zero; - - if (message != Native.Input.WM_INPUT) - return IntPtr.Zero; - - if (Native.Input.IsTouchEvent(Native.Input.GetMessageExtraInfo())) - { - // sometimes GetMessageExtraInfo returns 0, so additionally, mouse.ExtraInformation is checked below. - // touch events are handled by TouchHandler - statistic_dropped_touch_inputs.Value++; - return IntPtr.Zero; - } - - int payloadSize = sizeof(RawInputData); - - Native.Input.GetRawInputData((IntPtr)lParam, RawInputCommand.Input, out var data, ref payloadSize, sizeof(RawInputHeader)); - - if (data.Header.Type != RawInputType.Mouse) - return IntPtr.Zero; - - var mouse = data.Mouse; - - // `ExtraInformation` doesn't have the MI_WP_SIGNATURE set, so we have to rely solely on the touch flag. - if (Native.Input.HasTouchFlag(mouse.ExtraInformation)) - { - statistic_dropped_touch_inputs.Value++; - return IntPtr.Zero; - } - - //TODO: this isn't correct. - if (mouse.ExtraInformation > 0) - { - statistic_inputs_with_extra_information.Value++; - - // i'm not sure if there is a valid case where we need to handle packets with this present - // but the osu!tablet fires noise events with non-zero values, which we want to ignore. - // return IntPtr.Zero; - } - - var position = new Vector2(mouse.LastX, mouse.LastY); - float sensitivity = (float)Sensitivity.Value; - - if (mouse.Flags.HasFlagFast(RawMouseFlags.MoveAbsolute)) - { - var screenRect = mouse.Flags.HasFlagFast(RawMouseFlags.VirtualDesktop) ? Native.Input.VirtualScreenRect : new Rectangle(window.Position, window.ClientSize); - - Vector2 screenSize = new Vector2(screenRect.Width, screenRect.Height); - - if (mouse.LastX == 0 && mouse.LastY == 0) - { - // not sure if this is the case for all tablets, but on osu!tablet these can appear and are noise. - return IntPtr.Zero; - } - - // i am not sure what this 64 flag is, but it's set on the osu!tablet at very least. - // using it here as a method of determining where the coordinate space is incorrect. - if (((int)mouse.Flags & 64) == 0) - { - position /= raw_input_coordinate_space; - position *= screenSize; - } - - if (Sensitivity.Value != 1) - { - // apply absolute sensitivity adjustment from the centre of the screen area. - Vector2 halfScreenSize = (screenSize / 2); - - position -= halfScreenSize; - position *= (float)Sensitivity.Value; - position += halfScreenSize; - } - - // map from screen to client coordinate space. - // not using Window's PointToClient implementation to keep floating point precision here. - position -= new Vector2(window.Position.X, window.Position.Y); - position *= window.Scale; - - PendingInputs.Enqueue(new MousePositionAbsoluteInput { Position = position }); - statistic_absolute_events.Value++; - } - else - { - PendingInputs.Enqueue(new MousePositionRelativeInput { Delta = new Vector2(mouse.LastX, mouse.LastY) * sensitivity }); - statistic_relative_events.Value++; - } - - return IntPtr.Zero; - } } } diff --git a/osu.Framework/Platform/Windows/WindowsMouseHandler_SDL2.cs b/osu.Framework/Platform/Windows/WindowsMouseHandler_SDL2.cs index bd13d7b17..2d941ec17 100644 --- a/osu.Framework/Platform/Windows/WindowsMouseHandler_SDL2.cs +++ b/osu.Framework/Platform/Windows/WindowsMouseHandler_SDL2.cs @@ -2,15 +2,30 @@ // See the LICENCE file in the repository root for full licence text. using System; +using System.Drawing; +using osu.Framework.Extensions.EnumExtensions; +using osu.Framework.Input.StateChanges; +using osu.Framework.Platform.Windows.Native; +using osu.Framework.Statistics; +using osuTK; using static SDL2.SDL; namespace osu.Framework.Platform.Windows { internal partial class WindowsMouseHandler { + private static readonly GlobalStatistic<ulong> statistic_relative_events = GlobalStatistics.Get<ulong>(StatisticGroupFor<WindowsMouseHandler>(), "Relative events"); + private static readonly GlobalStatistic<ulong> statistic_absolute_events = GlobalStatistics.Get<ulong>(StatisticGroupFor<WindowsMouseHandler>(), "Absolute events"); + private static readonly GlobalStatistic<ulong> statistic_dropped_touch_inputs = GlobalStatistics.Get<ulong>(StatisticGroupFor<WindowsMouseHandler>(), "Dropped native touch inputs"); + + private static readonly GlobalStatistic<ulong> statistic_inputs_with_extra_information = + GlobalStatistics.Get<ulong>(StatisticGroupFor<WindowsMouseHandler>(), "Native inputs with ExtraInformation"); + + private const int raw_input_coordinate_space = 65535; + private SDL_WindowsMessageHook sdl2Callback = null!; - private bool bindHandlerSDL2(GameHost host) + private void initialiseSDL2(GameHost host) { // ReSharper disable once ConvertClosureToMethodGroup sdl2Callback = (ptr, wnd, u, param, l) => onWndProcSDL2(ptr, wnd, u, param, l); @@ -19,8 +34,109 @@ private bool bindHandlerSDL2(GameHost host) { host.InputThread.Scheduler.Add(() => SDL_SetWindowsMessageHook(enabled.NewValue ? sdl2Callback : null, IntPtr.Zero)); }, true); + } + + protected override void HandleMouseMoveRelative(Vector2 delta) + { + if (window is SDL2WindowsWindow) + { + // on SDL2, relative movement is reported via the WndProc handler below. + return; + } + + base.HandleMouseMoveRelative(delta); + } + + private unsafe IntPtr onWndProcSDL2(IntPtr userData, IntPtr hWnd, uint message, ulong wParam, long lParam) + { + if (!Enabled.Value) + return IntPtr.Zero; + + if (message != Native.Input.WM_INPUT) + return IntPtr.Zero; + + if (Native.Input.IsTouchEvent(Native.Input.GetMessageExtraInfo())) + { + // sometimes GetMessageExtraInfo returns 0, so additionally, mouse.ExtraInformation is checked below. + // touch events are handled by TouchHandler + statistic_dropped_touch_inputs.Value++; + return IntPtr.Zero; + } + + int payloadSize = sizeof(RawInputData); + + Native.Input.GetRawInputData((IntPtr)lParam, RawInputCommand.Input, out var data, ref payloadSize, sizeof(RawInputHeader)); + + if (data.Header.Type != RawInputType.Mouse) + return IntPtr.Zero; + + var mouse = data.Mouse; + + // `ExtraInformation` doesn't have the MI_WP_SIGNATURE set, so we have to rely solely on the touch flag. + if (Native.Input.HasTouchFlag(mouse.ExtraInformation)) + { + statistic_dropped_touch_inputs.Value++; + return IntPtr.Zero; + } + + //TODO: this isn't correct. + if (mouse.ExtraInformation > 0) + { + statistic_inputs_with_extra_information.Value++; + + // i'm not sure if there is a valid case where we need to handle packets with this present + // but the osu!tablet fires noise events with non-zero values, which we want to ignore. + // return IntPtr.Zero; + } + + var position = new Vector2(mouse.LastX, mouse.LastY); + float sensitivity = (float)Sensitivity.Value; + + if (mouse.Flags.HasFlagFast(RawMouseFlags.MoveAbsolute)) + { + var screenRect = mouse.Flags.HasFlagFast(RawMouseFlags.VirtualDesktop) ? Native.Input.VirtualScreenRect : new Rectangle(window.Position, window.ClientSize); + + Vector2 screenSize = new Vector2(screenRect.Width, screenRect.Height); + + if (mouse.LastX == 0 && mouse.LastY == 0) + { + // not sure if this is the case for all tablets, but on osu!tablet these can appear and are noise. + return IntPtr.Zero; + } + + // i am not sure what this 64 flag is, but it's set on the osu!tablet at very least. + // using it here as a method of determining where the coordinate space is incorrect. + if (((int)mouse.Flags & 64) == 0) + { + position /= raw_input_coordinate_space; + position *= screenSize; + } + + if (Sensitivity.Value != 1) + { + // apply absolute sensitivity adjustment from the centre of the screen area. + Vector2 halfScreenSize = (screenSize / 2); + + position -= halfScreenSize; + position *= (float)Sensitivity.Value; + position += halfScreenSize; + } + + // map from screen to client coordinate space. + // not using Window's PointToClient implementation to keep floating point precision here. + position -= new Vector2(window.Position.X, window.Position.Y); + position *= window.Scale; + + PendingInputs.Enqueue(new MousePositionAbsoluteInput { Position = position }); + statistic_absolute_events.Value++; + } + else + { + PendingInputs.Enqueue(new MousePositionRelativeInput { Delta = new Vector2(mouse.LastX, mouse.LastY) * sensitivity }); + statistic_relative_events.Value++; + } - return true; + return IntPtr.Zero; } } } diff --git a/osu.Framework/Platform/Windows/WindowsMouseHandler_SDL3.cs b/osu.Framework/Platform/Windows/WindowsMouseHandler_SDL3.cs deleted file mode 100644 index 0d67175e3..000000000 --- a/osu.Framework/Platform/Windows/WindowsMouseHandler_SDL3.cs +++ /dev/null @@ -1,13 +0,0 @@ -// Copyright (c) ppy Pty Ltd <[email protected]>. Licensed under the MIT Licence. -// See the LICENCE file in the repository root for full licence text. - -namespace osu.Framework.Platform.Windows -{ - internal partial class WindowsMouseHandler - { - private bool bindHandlerSDL3(GameHost host) - { - return false; - } - } -}

peppy · 2024-05-21T14:20:02Z

osu.Framework/Platform/Linux/SDL3LinuxWindow.cs

+// See the LICENCE file in the repository root for full licence text.
+
+using osu.Framework.Platform.SDL3;
+using static SDL.SDL3;


dunno how i feel about this

peppy · 2024-05-22T06:18:41Z

I think this looks OK.

@smoogipoo as discussed, can we add an SDL2 / SDL3 string here?

smoogipoo · 2024-05-22T06:39:11Z

How does this look? Wanted to keep the line somewhat short...

peppy · 2024-05-22T07:50:55Z

@bdach not sure if you want to give this a quick run on linux and make sure all is well?

as for iOS i don't really have the energy to test it but i'm sure we'll find out from @frenzibyte if it's failing at some point before next release, worst case.

smoogipoo · 2024-05-22T08:03:13Z

For linux I've tested that it runs and supports window modes with:

SDL_VIDEODRIVER=wayland
SDL_VIDEODRIVER=x11
OSU_SDL3=1;SDL_VIDEO_DRIVER=wayland
OSU_SDL3=1;SDL_VIDEO_DRIVER=x11

But note that this is via XWayland, not a true X11 impl.

bdach · 2024-05-22T08:11:51Z

Linux boots here. Appears to not be more broken than already known on all of the configurations I tested on my box (x11/wayland, sdl2/3).

Not reading source because I presume enough eyes have looked at that already.

smoogipoo added 16 commits May 21, 2024 16:09

Add SDL2 package reference

0417ca7

Renamespace classes to SDL3

7ee5f6f

Using `using SDL.SDL3` to fix namespace conflicts.

Add SDL2 classes

dd66f3e

Add app name support

8d7832a

Add ISDLWindow abstraction

21dedbb

Merge SDL2GameHost and SDL3GameHost

d72212b

Add SDL2/3 Windows window variants

1794191

Add SDL2/3 Linux window variants

14ca682

Add SDL2/3 macOS window variants

0a220b4

Add SDL2/3 Windows mouse handler variants

4aae23e

Adjust condition

58c9958

Make tests work use ISDLWindow interface

7bfc9fe

Fix iOS/Android projects, always use SDL3 on them

1b7438f

Remove unused classes

16f2dd4

Merge SDL2/3 windows mouse handler implementations

b937b04

Cleanups

d46a132

smoogipoo added the area:platform label May 21, 2024

pull-request-size bot added the size/XXL label May 21, 2024

Susko3 suggested changes May 21, 2024

View reviewed changes

Susko3 reviewed May 21, 2024

View reviewed changes

smoogipoo added 3 commits May 21, 2024 22:24

Fix readable key combinations not working in SDL2

ade299e

Split WindowsMouseHandler into generic and non-generic parts

6f0702a

Rename callback

8d5feec

peppy self-requested a review May 21, 2024 14:16

peppy reviewed May 21, 2024

View reviewed changes

Split entire SDL2 WindowsMouseHandler implementation

d5c0934

Add SDL2 initialisation string

47efee5

Add window type to frame statistics display

dc4b11f

peppy self-requested a review May 22, 2024 07:51

peppy approved these changes May 22, 2024

View reviewed changes

bdach approved these changes May 22, 2024

View reviewed changes

peppy merged commit 1f6b157 into ppy:master May 22, 2024
19 of 21 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Bring back SDL2 by default, lock SDL3 behind environment variable #6292

Bring back SDL2 by default, lock SDL3 behind environment variable #6292

smoogipoo commented May 21, 2024 •

edited

Loading

smoogipoo commented May 21, 2024 •

edited

Loading

Susko3 left a comment

Susko3 May 21, 2024

smoogipoo May 21, 2024

Susko3 May 21, 2024 •

edited

Loading

Susko3 May 21, 2024

smoogipoo May 21, 2024 •

edited

Loading

smoogipoo May 21, 2024

Susko3 May 21, 2024

peppy May 21, 2024

peppy commented May 22, 2024

smoogipoo commented May 22, 2024

peppy commented May 22, 2024

smoogipoo commented May 22, 2024 •

edited

Loading

bdach commented May 22, 2024

Bring back SDL2 by default, lock SDL3 behind environment variable #6292

Bring back SDL2 by default, lock SDL3 behind environment variable #6292

Conversation

smoogipoo commented May 21, 2024 • edited Loading

smoogipoo commented May 21, 2024 • edited Loading

Susko3 left a comment

Choose a reason for hiding this comment

Susko3 May 21, 2024

Choose a reason for hiding this comment

smoogipoo May 21, 2024

Choose a reason for hiding this comment

Susko3 May 21, 2024 • edited Loading

Choose a reason for hiding this comment

Susko3 May 21, 2024

Choose a reason for hiding this comment

smoogipoo May 21, 2024 • edited Loading

Choose a reason for hiding this comment

smoogipoo May 21, 2024

Choose a reason for hiding this comment

Susko3 May 21, 2024

Choose a reason for hiding this comment

peppy May 21, 2024

Choose a reason for hiding this comment

peppy commented May 22, 2024

smoogipoo commented May 22, 2024

peppy commented May 22, 2024

smoogipoo commented May 22, 2024 • edited Loading

bdach commented May 22, 2024

smoogipoo commented May 21, 2024 •

edited

Loading

smoogipoo commented May 21, 2024 •

edited

Loading

Susko3 May 21, 2024 •

edited

Loading

smoogipoo May 21, 2024 •

edited

Loading

smoogipoo commented May 22, 2024 •

edited

Loading