Add SDL3 support and fix wayland issue (PR #26435)
Andy Vandijck
SDL3 support
Minor wayland fix
You can view, comment on, or merge this pull request online at:
https://github.com/wxWidgets/wxWidgets/pull/26435
Commit Summary
- aa138aa Add SDL3 support and fix wayland issue
File Changes
(7 files)
- M build/cmake/init.cmake (20)
- M build/cmake/lib/core/CMakeLists.txt (12)
- M build/cmake/setup.h.in (7)
- M configure (20207)
- M configure.ac (19)
- M setup.h.in (5)
- M src/unix/sound_sdl.cpp (128)
Patch Links:
- https://github.com/wxWidgets/wxWidgets/pull/26435.patch
- https://github.com/wxWidgets/wxWidgets/pull/26435.diff
—
Reply to this email directly, view it on GitHub, or unsubscribe.
Triage notifications on the go with GitHub Mobile for iOS or Android.
You are receiving this because you are subscribed to this thread.
VZ
@vadz commented on this pull request.
Thanks for updating the code to use SDL3, this is much appreciated!
I'd like to merge this, however I'm not sure about what the Wayland issue is and whether this is the right fix for it — maybe it should be dealt with in a separate PR?
Also, it looks configure was regenerated using a different version of autoconf, which might account for the CI build failures. Could you please follow build/tools/autoconf/README.md to update it?
> @@ -628,7 +628,7 @@
Recommended setting: 1, only set to 0 if wayland-client is not available.
*/
-#cmakedefine01 wxHAVE_WAYLAND_CLIENT
+#cmakedefine wxHAVE_WAYLAND_CLIENT 1
Could you please explain why is this needed?
—
Reply to this email directly, view it on GitHub, or unsubscribe.
Triage notifications on the go with GitHub Mobile for iOS or Android.
You are receiving this because you are subscribed to this thread.
Andy Vandijck
@andyvand commented on this pull request.
> @@ -628,7 +628,7 @@
Recommended setting: 1, only set to 0 if wayland-client is not available.
*/
-#cmakedefine01 wxHAVE_WAYLAND_CLIENT
+#cmakedefine wxHAVE_WAYLAND_CLIENT 1
It doesn't build right without this
—
Reply to this email directly, view it on GitHub, or unsubscribe.
Triage notifications on the go with GitHub Mobile for iOS or Android.
You are receiving this because you are subscribed to this thread.
VZ
@vadz commented on this pull request.
> @@ -628,7 +628,7 @@
Recommended setting: 1, only set to 0 if wayland-client is not available.
*/
-#cmakedefine01 wxHAVE_WAYLAND_CLIENT
+#cmakedefine wxHAVE_WAYLAND_CLIENT 1
Sorry, what do you mean by "right"?
In any case, maybe I should be more explicit: this change is almost certainly not correct, wxHAVE_WAYLAND_CLIENT should be set by the code in build/cmake/init.cmake and if there is something wrong with it, this should be fixed there.
But, again, let's not deal with this here, please undo this change and regenerate configure to check if this fixes the failing CI builds so that the rest of the changes could be merged.
TIA!
—
Reply to this email directly, view it on GitHub, or unsubscribe.
Triage notifications on the go with GitHub Mobile for iOS or Android.
You are receiving this because you are subscribed to this thread.
VZ
vadz left a comment (wxWidgets/wxWidgets#26435)MSW CI failures seem to be spurious, but I'm less sure about the Mac builds, it looks like something could be wrong with configure, could you please check?
Also, do we absolutely have to include SDL3 headers using #include <SDL3/SDL.h> instead of just #include <SDL.h>? If we could keep the latter, we could avoid adding wxUSE_LIBSDL3 which would be nice(r).
—
Reply to this email directly, view it on GitHub, or unsubscribe.
Triage notifications on the go with GitHub Mobile for iOS or Android.
You are receiving this because you are subscribed to this thread.
VZ
vadz left a comment (wxWidgets/wxWidgets#26435)@andyvand Do you plan to look at the CI failures here or should I try to deal with them?
—
Reply to this email directly, view it on GitHub, or unsubscribe.
Triage notifications on the go with GitHub Mobile for iOS or Android.
You are receiving this because you are subscribed to this thread.
Alexey Rassikhin
alex-rass-88 left a comment (wxWidgets/wxWidgets#26435)SDL using for sound. What about using something like MiniAudio?
—
Reply to this email directly, view it on GitHub, or unsubscribe.
Triage notifications on the go with GitHub Mobile for iOS or Android.
You are receiving this because you are subscribed to this thread.
VZ
vadz left a comment (wxWidgets/wxWidgets#26435)SDL using for sound. What about using something like MiniAudio?
If anybody is willing to contribute a wxSound implementation based on this, let's discuss it. But for now the only implementation we have under Unix is the SDL-based one.
—
Reply to this email directly, view it on GitHub, or unsubscribe.
Triage notifications on the go with GitHub Mobile for iOS or Android.
You are receiving this because you are subscribed to this thread.
Andy Vandijck
andyvand left a comment (wxWidgets/wxWidgets#26435)@andyvand Do you plan to look at the CI failures here or should I try to deal with them?
You may deal with them yourself.
I don't have the time to review them (as I use latest autoconf + automake)
—
Reply to this email directly, view it on GitHub, or unsubscribe.
Triage notifications on the go with GitHub Mobile for iOS or Android.
You are receiving this because you are subscribed to this thread.
VZ
vadz left a comment (wxWidgets/wxWidgets#26435)@andyvand Do you plan to look at the CI failures here or should I try to deal with them?
You may deal with them yourself.
This wording has killed my motivation to look at this for some reason, so I'm leaving this until somebody else is motivated enough to do it. Especially because I think it would really be better to avoid setting the global wxUSE_LIBSDL3 if possible, as mentioned above.
I don't have the time to review them (as I use latest autoconf + automake)
OK, so you didn't follow build/tools/autoconf/README.md at all, good to know.
—
Reply to this email directly, view it on GitHub, or unsubscribe.
Triage notifications on the go with GitHub Mobile for iOS or Android.
You are receiving this because you are subscribed to this thread.
Maarten
@MaartenBent commented on this pull request.
> @@ -628,7 +628,7 @@
Recommended setting: 1, only set to 0 if wayland-client is not available.
*/
-#cmakedefine01 wxHAVE_WAYLAND_CLIENT
+#cmakedefine wxHAVE_WAYLAND_CLIENT 1
See #26707
—
Reply to this email directly, view it on GitHub, or unsubscribe.
Triage notifications, keep track of coding agent tasks and review pull requests on the go with GitHub Mobile for iOS and Android. Download it today!
You are receiving this because you are subscribed to this thread.
Maarten
MaartenBent left a comment (wxWidgets/wxWidgets#26435)#include <SDL.h> does not work for SDL3, see https://wiki.libsdl.org/SDL3/FAQDevelopment#do-i-include-sdlh-or-include-sdl3sdlh-
As long as we don't support __has_include from c++17, we'll have to use wxUSE_LIBSDL3. I'll try to finish this PR.
—
Reply to this email directly, view it on GitHub, or unsubscribe.
Triage notifications, keep track of coding agent tasks and review pull requests on the go with GitHub Mobile for iOS and Android. Download it today!
You are receiving this because you are subscribed to this thread.
PB
PBfordev left a comment (wxWidgets/wxWidgets#26435)As long as we don't support
__has_includefrom c++17, we'll have to usewxUSE_LIBSDL3. I'll try to finish this PR.
You are probably aware of it, but just be sure, there is some support for it (__has_include seems to be available since GCC 5):
https://github.com/search?q=repo%3AwxWidgets%2FwxWidgets%20wxHAS_CXX17_INCLUDE&type=code
—
Reply to this email directly, view it on GitHub, or unsubscribe.
Triage notifications, keep track of coding agent tasks and review pull requests on the go with GitHub Mobile for iOS and Android. Download it today!
You are receiving this because you are subscribed to this thread.
Maarten
MaartenBent left a comment (wxWidgets/wxWidgets#26435)But while it seems that __has_include is available since GCC 5; the wxWidgets macro needs also C++17?
I don't know what the minimum supported compilers are now, but is is probably better to follow c++ standards instead of making wxHAS_CXX17_INCLUDE work differently for GCC, Clang or other compilers.
I created #26773 based on this PR with some fixes applied.
—
Reply to this email directly, view it on GitHub, or unsubscribe.
Triage notifications, keep track of coding agent tasks and review pull requests on the go with GitHub Mobile for iOS and Android. Download it today!
You are receiving this because you are subscribed to this thread.
VZ
—
Reply to this email directly, view it on GitHub, or unsubscribe.
Triage notifications, keep track of coding agent tasks and review pull requests on the go with GitHub Mobile for iOS and Android. Download it today!
You are receiving this because you are subscribed to this thread.