Since there are a lot of repeated ifdefs relates to functions that have simply been renamed or only have minor changes to the parameters, I'd recommend using wrapper functions where possible to make the changes easier to read and less fragile to maintain.

Since there are a lot of repeated ifdefs relates to functions that have simply been renamed or only have minor changes to the parameters, I'd recommend using wrapper functions where possible to make the changes easier to read and less fragile to maintain.

Thanks for your advice.
Do you prefer #define, static functions or member functions?

Here is an example with static functions:

Before:

#if SDL_VERSION_ATLEAST(3, 0, 0)
    if (!SDL_AddGamepadMappingsFromFile(file.getPath().toString(Common::Path::kNativeSeparator).c_str()))
#else
    if (SDL_GameControllerAddMappingsFromFile(file.getPath().toString(Common::Path::kNativeSeparator).c_str()) < 0)
#endif
        error("File %s not valid: %s", file.getPath().toString(Common::Path::kNativeSeparator).c_str(), SDL_GetError());

After with static functions:

static bool gameControllerAddMappingsFromFile(const char *file) {
#if SDL_VERSION_ATLEAST(3, 0, 0)
	return SDL_AddGamepadMappingsFromFile(file);
#else
	return SDL_GameControllerAddMappingsFromFile(file) >= 0;
#endif
}

...

if (!gameControllerAddMappingsFromFile(file.getPath().toString(Common::Path::kNativeSeparator).c_str()))
        error("File %s not valid: %s", file.getPath().toString(Common::Path::kNativeSeparator).c_str(), SDL_GetError());
    

Regarding the imgui update, did you add the files from the matching git commit as rest of the code?

Regarding the imgui update, did you add the files from the matching git commit as rest of the code?

Yes I used the commit ocornut/imgui@cb16568 corresponding to the tag https://github.com/ocornut/imgui/releases/tag/v1.91.3

About this, I see that there is a backends/imgui/scummvm.patch file, I should update this file too, no?

Since there are a lot of repeated ifdefs relates to functions that have simply been renamed or only have minor changes to the parameters, I'd recommend using wrapper functions where possible to make the changes easier to read and less fragile to maintain.

Tell me if it's OK now or did you think something different?

About this, I see that there is a backends/imgui/scummvm.patch file, I should update this file too, no?
Yes. I use it for updating imgui from time to time. Just make a diff against the imgui git commit.

About this, I see that there is a backends/imgui/scummvm.patch file, I should update this file too, no?
Yes. I use it for updating imgui from time to time. Just make a diff against the imgui git commit.

I updated the patch file, but I have a lot more changes than the previous one, can you have a look @sev- please?

Since there are a lot of repeated ifdefs relates to functions that have simply been renamed or only have minor changes to the parameters, I'd recommend using wrapper functions where possible to make the changes easier to read and less fragile to maintain.

Tell me if it's OK now or did you think something different?

There is no definite answer here... use whatever makes the code easier to read and maintain.

From the three options you provided:

• static wrapper functions would be a very straightforward way
• defines are a bit uglier than function wrappers, IMHO
• member functions are also a nice idea, we could keep all of these functions neatly placed inside classes, and instantiate a different class per SDL version

So, from the options above, we could either use a C-style implementation (static wrapper functions) or a C++-style implementation (different class per SDL version, inherited from a common parent class, with member functions). I personally like the C++ approach better, but function wrappers would also work just fine :)

Since there are a lot of repeated ifdefs relates to functions that have simply been renamed or only have minor changes to the parameters, I'd recommend using wrapper functions where possible to make the changes easier to read and less fragile to maintain.

Tell me if it's OK now or did you think something different?

There is no definite answer here... use whatever makes the code easier to read and maintain.

From the three options you provided:

• static wrapper functions would be a very straightforward way
• defines are a bit uglier than function wrappers, IMHO
• member functions are also a nice idea, we could keep all of these functions neatly placed inside classes, and instantiate a different class per SDL version

So, from the options above, we could either use a C-style implementation (static wrapper functions) or a C++-style implementation (different class per SDL version, inherited from a common parent class, with member functions). I personally like the C++ approach better, but function wrappers would also work just fine :)

I made my choice already, sorry I'm not a huge fan of inheritance and I don't see the benefit of creating 1 class by version.

I made my choice already, sorry I'm not a huge fan of inheritance and I don't see the benefit of creating 1 class by version.

That's fine :) Nice implementation!

Thank you for looking into this!

I wanted to give it a try on Mingw64, and currently it seems that Win32 isn't fully supported yet:

In file included from backends/platform/sdl/win32/win32-window.cpp:27:
./backends/platform/sdl/win32/win32-window.h:32:9: error: 'HWND' does not name a type
   32 |         HWND getHwnd();
      |         ^~~~
backends/platform/sdl/win32/win32-main.cpp: In function 'int WinMain(HINSTANCE, HINSTANCE, LPSTR, int)':
backends/platform/sdl/win32/win32-main.cpp:54:16: error: 'main' was not declared in this scope
   54 |         return main(__argc, __argv);
      |                ^~~~
make: *** [Makefile.common:176: backends/platform/sdl/win32/win32-main.o] Error 1
make: *** Waiting for unfinished jobs....
    C++      engines/scumm/boxes.o
    C++      engines/scumm/camera.o
backends/platform/sdl/win32/win32-window.cpp: In member function 'virtual void SdlWindow_Win32::setupIcon()':
backends/platform/sdl/win32/win32-window.cpp:36:29: error: 'getHwnd' was not declared in this scope; did you mean 'getwc'?
   36 |                 HWND hwnd = getHwnd();
      |                             ^~~~~~~
      |                             getwc
backends/platform/sdl/win32/win32-window.cpp: At global scope:
backends/platform/sdl/win32/win32-window.cpp:53:6: error: no declaration matches 'HWND__* SdlWindow_Win32::getHwnd()'
   53 | HWND SdlWindow_Win32::getHwnd() {
      |      ^~~~~~~~~~~~~~~
backends/platform/sdl/win32/win32-window.cpp:53:6: note: no functions named 'HWND__* SdlWindow_Win32::getHwnd()'
./backends/platform/sdl/win32/win32-window.h:29:7: note: 'class SdlWindow_Win32' defined here
   29 | class SdlWindow_Win32 final : public SdlWindow {
      |       ^~~~~~~~~~~~~~~
make: *** [Makefile.common:176: backends/platform/sdl/win32/win32-window.o] Error 1

I quickly went through the release notes and indeed it looks like backends/platform/sdl/win32/win32-window.cpp is missing something - I'll check if I can adapt it from the other changes.

I quickly went through the release notes and indeed it looks like backends/platform/sdl/win32/win32-window.cpp is missing something - I'll check if I can adapt it from the other changes.

That's nice of you, thank you.

Colossal work, thank you!

Have number of comments, mainly on formatting and suggestion to add more wrappers.

I am concerned that the code now becomes much harder to maintain with this amount of ifdefs, so maybe splitting files/adding more wrappers could make it easier to read and less fragile?

Sorry I've been sick for a few days, as soon as I feel better I'll take care of it.

@scemino for generating patch, please use the same command as outlined in cff87d8

diff -rbu . ../scummvm/scummvm/backends/imgui/|grep -v ^Only >../scummvm/scummvm/backends/imgui/scummvm.patch

This will remove those whitespace and file path changes.

I like the sdl-events.cpp splut, the code looks much much cleaner, though, of course, somebody has to modify it in three places if any enhancements are added (relatively unlikely).

I feel that the code is less clean after the split because of the amount of duplication that could be avoided by simply using defines for the renamed functions, similarly to what we did when we first moved to SDL2 in commit 8530997.

Also, did you test the backend with testbed?

I need to do the test with SDL1, then SDL2, then SDL3, right ?
That's a good idea, I found some bugs already with SDL3: SDL_BlitSurface failed: The surface is not indexed format!.
This means I need to check every function where a test is done like thisif (SDL_... != 0)
I will fix this ASAP.
There are 2 places, I don't know if I need to make the changes in backends/platform/openpandora/op-backend.cpp and backends/platform/sdl/amigaos/amigaos.cpp, what do you recommend ?

Another bug: the ImGui window can't be moved outside the main window.

And also, perhaps testbed needs to be enhanced with test for the multifinger taps?

I'm not sure I'm the best person to do that, but I can give a try if you want.

amigaos4 has just gotten an sdl3 rc
https://www.amigans.net/modules/newbb/viewtopic.php?post_id=152728#forumpost152728

not tested and i won't be able to do it anymore, but i guess sdl3 support will be possible

amigaos4 has just gotten an sdl3 rc https://www.amigans.net/modules/newbb/viewtopic.php?post_id=152728#forumpost152728

not tested and i won't be able to do it anymore, but i guess sdl3 support will be possible

So it could be useful to do these changes for amigaos, I did the changes, I have no way to test it ;)

thank you very much
someone will be happy I'm sure 😀

Another bug: the ImGui window can't be moved outside the main window.

This one is fixed now!

@scemino any plans to regenerate the patch so it shows less diff?

I would like to merge it, so I could subsequently update ImGui, and the patch stays in the way.

@scemino any plans to regenerate the patch so it shows less diff?

For me it seems correct, the only big differences is the addition of imgui_impl_sdlrenderer3.cpp and imgui_impl_sdlrenderer3.h, can you point me out what's wrong?

I would like to merge it, so I could subsequently update ImGui, and the patch stays in the way.

I can drop my modifications on the patch, you want me to do that?

@scemino any plans to regenerate the patch so it shows less diff?

For me it seems correct, the only big differences is the addition of imgui_impl_sdlrenderer3.cpp and imgui_impl_sdlrenderer3.h, can you point me out what's wrong?

No, it is not correct. You did not provide the paths exactly as it was before, as a result, we have a lot of noise like:

- diff --color -rbu ./backends/imgui_impl_opengl3.cpp - ../scummvm/scummvm/backends/imgui/backends/imgui_impl_opengl3.cpp
- --- ./backends/imgui_impl_opengl3.cpp	2024-10-07 18:21:01
- +++ ../scummvm/scummvm/backends/imgui/backends/imgui_impl_opengl3.cpp	2024-10-07 18:30:56
+ diff --git a/backends/imgui_impl_opengl3.cpp b/backends/imgui_impl_opengl3.cpp
+ index 770f85f4..b363ba07 100644
+ --- a/backends/imgui_impl_opengl3.cpp
+ +++ b/backends/imgui_impl_opengl3.cpp
@@ -114,7 +114,7 @@
 #define _CRT_SECURE_NO_WARNINGS
 #endif
-  
+

I am asking to do this for conistency, otherwise we end up with changing it every time.

@scemino any plans to regenerate the patch so it shows less diff?

For me it seems correct, the only big differences is the addition of imgui_impl_sdlrenderer3.cpp and imgui_impl_sdlrenderer3.h, can you point me out what's wrong?

No, it is not correct. You did not provide the paths exactly as it was before, as a result, we have a lot of noise like:

- diff --color -rbu ./backends/imgui_impl_opengl3.cpp - ../scummvm/scummvm/backends/imgui/backends/imgui_impl_opengl3.cpp
- --- ./backends/imgui_impl_opengl3.cpp	2024-10-07 18:21:01
- +++ ../scummvm/scummvm/backends/imgui/backends/imgui_impl_opengl3.cpp	2024-10-07 18:30:56
+ diff --git a/backends/imgui_impl_opengl3.cpp b/backends/imgui_impl_opengl3.cpp
+ index 770f85f4..b363ba07 100644
+ --- a/backends/imgui_impl_opengl3.cpp
+ +++ b/backends/imgui_impl_opengl3.cpp
@@ -114,7 +114,7 @@
 #define _CRT_SECURE_NO_WARNINGS
 #endif
-  
+

I am asking to do this for conistency, otherwise we end up with changing it every time.

Sorry I'm not able to do it:

git diff --color -rbu > scummvm.patch
error: invalid option: -rbu

I removed the modifications on the patch.

git diff --color -rbu > scummvm.patch
error: invalid option: -rbu

because it is NOT git. it is plain diff :D git diff shows you the difference between commits, while here you need to store differences between ScummVM and ImGui repos.

Quoting my early reply:

@scemino for generating patch, please use the same command as outlined in cff87d8

diff -rbu . ../scummvm/scummvm/backends/imgui/|grep -v ^Only >../scummvm/scummvm/backends/imgui/scummvm.patch

This will remove those whitespace and file path changes.

Also, there are conflicts now, so the thing has to be rebased.

I removed the modifications on the patch.

I am using the patch for applying the changes on the ImGui upgrade. What is your proposed solution to this?

git diff --color -rbu > scummvm.patch
error: invalid option: -rbu

because it is NOT git. it is plain diff :D git diff shows you the difference between commits, while here you need to store differences between ScummVM and ImGui repos.

Quoting my early reply:

@scemino for generating patch, please use the same command as outlined in cff87d8

diff -rbu . ../scummvm/scummvm/backends/imgui/|grep -v ^Only >../scummvm/scummvm/backends/imgui/scummvm.patch

This will remove those whitespace and file path changes.

Also, there are conflicts now, so the thing has to be rebased.

OK I understand now, sorry about that.

Thank you, merging.

Thank you! Do you plan to add it also for create_project?