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

[imgui] sdl3 bindings #42850

Draft
wants to merge 8 commits into
base: master
Choose a base branch
from

Conversation

constcuriosity
Copy link

@constcuriosity constcuriosity commented Dec 22, 2024

Adds new sdl3-binding and sdl3-renderer-binding features to the imgui port.

Imgui already has backends implemented for sdl3. They just hadn't yet been exposed as options like the sdl2 backends were. This change mainly just copies the same operations that are done for sdl2 support, but swaps the 2 for a 3.

  • Changes comply with the maintainer guide.
  • SHA512s are updated for each updated download.
  • The "supports" clause reflects platforms that may be fixed by this new version.
  • Any fixed CI baseline entries are removed from that file.
  • Any patches that are no longer applied are deleted from the port's directory.
  • The version database is fixed by rerunning ./vcpkg x-add-version --all and committing the result.
  • Only one version is added to each modified port's versions file.

@constcuriosity
Copy link
Author

@microsoft-github-policy-service agree

@constcuriosity constcuriosity marked this pull request as ready for review December 23, 2024 00:54
@constcuriosity constcuriosity changed the title Imgui sdl3 bindings [imgui] sdl3 bindings Dec 23, 2024
@Cheney-W Cheney-W added category:port-feature The issue is with a library, which is requesting new capabilities that didn’t exist info:reviewed Pull Request changes follow basic guidelines labels Dec 23, 2024
@JavierMatosD
Copy link
Contributor

Hi @constcuriosity, thank you for the contribution. Unfortunately, all feature sets in ports should be side by side installable. This is implementing alternatives as features. See -> https://learn.microsoft.com/en-us/vcpkg/contributing/maintainer-guide#do-not-use-features-to-implement-alternatives

Currently, vcpkg install imgui[sdl2-binding,sdl3-binding] will not build.

Marking this vcpkg-team-review for a path forward.

@JavierMatosD JavierMatosD marked this pull request as draft December 23, 2024 17:26
@JavierMatosD JavierMatosD added the requires:vcpkg-team-review This PR or issue requires someone on the vcpkg team to take a further look. label Dec 23, 2024
@constcuriosity
Copy link
Author

Hi @JavierMatosD, thanks for taking a look at this pull request. I'll take a look at seeing if there's a way forward for these two features to compile together.

Do you know offhand if all of the directx version features are able to compile together? I'm just looking to extend the pattern that's already been established by the port.

@JavierMatosD
Copy link
Contributor

Hi @JavierMatosD, thanks for taking a look at this pull request. I'll take a look at seeing if there's a way forward for these two features to compile together.

I spoke with my coworkers and it seems that the SDL project currently calls 2.x "current". See -> https://www.libsdl.org/. We think SDL2 should remain the canonical one for now. We encourage you to create an overlay port with this added functionality for your own purposes. See -> https://learn.microsoft.com/vcpkg/concepts/overlay-ports

Do you know offhand if all of the directx version features are able to compile together? I'm just looking to extend the pattern that's already been established by the port.

These features were probably added erroneously.

I'm closing this PR for now. Please feel free to reopen the PR if you feel we are mistaken.

@constcuriosity
Copy link
Author

I spoke with my coworkers and it seems that the SDL project currently calls 2.x "current". See -> https://www.libsdl.org/. We think SDL2 should remain the canonical one for now. We encourage you to create an overlay port with this added functionality for your own purposes. See -> https://learn.microsoft.com/vcpkg/concepts/overlay-ports

SDL3 has hit API and ABI lock as listed at https://wiki.libsdl.org/SDL3/FrontPage as well as in the formation of the sdl3 port a bit ago #40867. While not official yet, projects are now encouraged to move towards adoption of SDL3.

@constcuriosity
Copy link
Author

While you can have a vcpkg.json file that includes both sdl2 and sdl3, you can't follow the usage lines for both of them in your own cmake files. Doing so will have the 2nd library inclusion complain about a difference between the INTERFACE_SDL_VERSION and the SDL_VERSION of the next to-be-included library. And this is what causes the sdl2-binding and sdl3-binding incompatibility. Those features request either sdl2 or sdl3 as dependencies, and turning on both features will attempt to enforce that both are included, which triggers the above error.

This feels like it breaks the spirit of Ports in the current baseline must be installable simultaneously, since while you can technically install both SDL2 and SDL3 simultaneously, you can't have both of them as a dependency. If that was possible, then these features would work fine together.

Practically though... I don't know when that'd really happen. I think the vast majority of projects would only ever use one or the other. And same would go for these features on the imgui port. I guess the workaround would be to remove the dependency on each of the features, and rely on the project author add the dependencies themselves, but that's counter to the goal of vcpkg.

So with the TL;DR "It's not the features' fault, it's SDL2 and SDL3 incompatibility", I'd like to request an exception to the "no mutually exclusive features" policy. Making an overlay port would certainly work but it introduces a continuous maintenance burden by forking a frequently updated port (the version was even bumped within the short lifetime of this PR), and I believe the likelihood of a project purposefully including both SDL versions and both features to be very small.

Thanks for your time.

@constcuriosity
Copy link
Author

@JavierMatosD Sorry to ping you directly but it doesn't look like I have permission to re-open the PR myself.

@Mengna-Li Mengna-Li reopened this Dec 26, 2024
@Cheney-W Cheney-W removed info:reviewed Pull Request changes follow basic guidelines requires:vcpkg-team-review This PR or issue requires someone on the vcpkg team to take a further look. labels Dec 27, 2024
@Agorath
Copy link
Contributor

Agorath commented Dec 29, 2024

This feels like it breaks the spirit of Ports in the current baseline must be installable simultaneously, since while you can technically install both SDL2 and SDL3 simultaneously, you can't have both of them as a dependency. If that was possible, then these features would work fine together.

Practically though... I don't know when that'd really happen. I think the vast majority of projects would only ever use one or the other. And same would go for these features on the imgui port. I guess the workaround would be to remove the dependency on each of the features, and rely on the project author add the dependencies themselves, but that's counter to the goal of vcpkg.

So with the TL;DR "It's not the features' fault, it's SDL2 and SDL3 incompatibility", I'd like to request an exception to the "no mutually exclusive features" policy. Making an overlay port would certainly work but it introduces a continuous maintenance burden by forking a frequently updated port (the version was even bumped within the short lifetime of this PR), and I believe the likelihood of a project purposefully including both SDL versions and both features to be very small.

Thanks for your time.

I want to voice my support for @constcuriosity's request for an exception to the abovementioned rule.
SDL3 is just about to become the stable version. If you look at SDL's GitHub repository, you will see that they already switched the main branch to SDL3 and moved SDL2 into a separate branch.
As SDL2 will undoubtedly continue to be used by many existing applications due to the complexity of completely switching from SDL2 to SDL3, not exposing both imgui backend APIs for SDL2/3 as features is, IMHO, not an option for the foreseeable future.
I'd also like to request an exception to the mentioned rule since SDL2 and SDL3 are not compatible with each other anyway, and it would make no sense to install both features simultaneously.

SDL2 and SDL3 are mutually exclusive ports. They're two versions of the same library with API differences. While the ports can be successfully installed alongside each other, you can't include both as dependencies. The SDL library has a version enforcement mechanism.

The best practice though is for vcpkg ports and features to always be installable alongside each other. But we can't do that because of above dependency conflicts. So this commit makes it so that if both the sdl2 and sdl3 features are enabled, the sdl2 ones are effecitvely ignored and a warning is logged to the user.
@constcuriosity
Copy link
Author

I've authored a compromise where the sdl2-binding, sdl3-binding, sdl2-renderer-binding, and sdl3-renderer-binding features can all be included as enabled features. When both SDL2 and SDL3 features are enabled, the SDL2 features are basically ignored and a warning is logged that says that the SDL2 dependency was not enforced.

So the ports and features now technically satisfy the maintainer guidelines, but I wouldn't say it adheres to the spirit of them. Nevertheless it continues with my statement about that including both SDL2 and SDL3 features is not expected and very much not desired behavior.

@constcuriosity constcuriosity marked this pull request as ready for review December 30, 2024 00:58
@dg0yt
Copy link
Contributor

dg0yt commented Dec 30, 2024

The problem is not with projects, but with ports in vcpkg. AFAIU the conflicting features will cause failures in vcpkg CI as soon as one port depends on feature sdl2 and another oner depends on feature sdl3.

@JavierMatosD JavierMatosD added the requires:vcpkg-team-review This PR or issue requires someone on the vcpkg team to take a further look. label Dec 30, 2024
@BillyONeal
Copy link
Member

Please note I am speaking personally, not as 'the maintainers' here.

I don't believe we have the power to grant 'exceptions' here, as the rule comes from technical realities of the situation. We can't build the 2^N universes of dependencies allowing effectively mutually exclusive features would force. We have to choose a reasonable answer to what is the canonical vcpkg answer in such cases, and users who want something else have to leave the bounds of the curated registry.

The problem is not with projects, but with ports in vcpkg. AFAIU the conflicting features will cause failures in vcpkg CI as soon as one port depends on feature sdl2 and another oner depends on feature sdl3.

In addition to what dg0yt says here, I want to note that this is in large part what vcpkg is trying to 'sell'. That you can show up to our curated registry, do vcpkg install a b for any a and b in that registry, and the resulting thing be functional. Our CI is how we test that that works, but it is more than just an infrastructure concern for us.

As long as SDL's owners consider and document SDL2 as the current version, I believe as far as our curated registry is concerned, that needs to be the version ports use when there are questions like this.

SDL2 and SDL3 are in the small group of major version breaks where the upstream developers go out of their way to make the break 'clean' and simultaneously installable. The big example in our registry that also did this was qt5 and qt6. For projects that did not implement the separation qt itself did, like qt-advanced-docking-system, we held the canonical version that downstream port used at qt5 for some time after qt6's release. It seems unlikely to me that the imgui is any different than qt-advanced-docking-system here.

SDL3 is just about to become the stable version. If you look at SDL's GitHub repository, you will see that they already switched the main branch to SDL3 and moved SDL2 into a separate branch.

It makes sense that their development repo would be a leading indicator on what they consider to be the canonical version. vcpkg, because we have to make sure downstream dependencies work when that switch is flipped, will be lagging.

@Agorath
Copy link
Contributor

Agorath commented Jan 3, 2025

Thanks for the detailed explanation! After considering it, I agree that you are correct, and I'd like to withdraw my "application" for an exception. 😅
The only other viable course of action, if we wanted to have both features be compatible, would have to be to ask the SDL developers or submit a pull request to adjust their CMake scripts so that SDL2 and SDL3 can be built simultaneously. I looked at what causes the failure, and it all seems to come down to the variable SDL_VERSION being used in both versions' CMake scripts, which leads to the build failure. If this variable were renamed to SDL_VERSION{2/3} respectively, I surmise that the build would work.
I'll try that with local forks of both SDL2 and 3 or with a vcpkg patch in a custom registry to see if that solves the issue. If it does, I'll submit a pull request to the SDL repo or vcpkg, depending on which solution works better.

@JavierMatosD JavierMatosD marked this pull request as draft January 3, 2025 17:13
@dg0yt dg0yt mentioned this pull request Jan 7, 2025
7 tasks
@Cheney-W Cheney-W removed the requires:vcpkg-team-review This PR or issue requires someone on the vcpkg team to take a further look. label Jan 10, 2025
@Agorath
Copy link
Contributor

Agorath commented Jan 12, 2025

I'd like to say I'm not exactly well-versed in CMake and only scrape by with a healthy dose of intuition when working with it.

Okay, that being said, I looked at the respective CMakeLists.txt files, and to me, modifying SDL_VERSION to be SDL{2/3]_VERSION wouldn't be near enough to fix this problem. There are many other variables in the respective CMake files that share the same name, i.e. SDL_OPENGL and many, many more, which could run into the same problems.

That said, being able to use the ImGui vcpkg port for either SDL2 or SDL3 will be a requirement for the foreseeable future. Many legacy apps use SDL2, and not exposing the bindings for SDL3 in general, and its new GPU API in particular, will be a problem for new projects or projects that want to upgrade.

Would anyone happen to have any ideas or input on the above? 🤔 (Particularly the bits about CMake)

In the meantime, I'll open an issue with the SDL project and ask for their opinion on this problem.
In case anyone wants to weigh in on the topic, this is the link to the issue I opened with the SDL project: libsdl-org/SDL#11920

@Agorath
Copy link
Contributor

Agorath commented Jan 14, 2025

Okay, after talking to the maintainers of SDL, it looks like there are three ways forward with the SDL2 <-> SDL3 problem:

  1. We wait until SDL3 is officially released and then drop SDL2 for SDL3 in the imgui port as proposed above.
    => This would, from that point onwards, prevent users with legacy SDL2 applications from upgrading to newer imgui versions via vcpkg.
  2. We split up all of the backends of imgui into independent static libraries as was initially proposed by @JackBoosY in this pull request: [imgui] Fix find dependencies #15063.
    => This would solve the linking problems and allow the imgui port to retain support for both SDL2 and SDL3. But, as stated in the pull request discussion, it could be a breaking change for existing imgui customers, leading to the customers having to modify/repair their code.
  3. We utilise the new sdl2-compat drop-in replacement library to replace the currently used SDL2 port by creating a new vcpkg port of sdl2-compat. sdl2-compat uses SDL3 behind the scenes and is supposed to be a 100% compatible replacement library for legacy SDL2 applications.
    => This would also allow the imgui port to continue to provide support for both SDL2 and SDL3 but is, of course, experimental as sdl2-compat is still pretty new and in active development as it has to follow the development of SDL3.
    I already created an sdl2-compat vcpkg port locally and successfully built an overlay port of imgui by replacing the sdl2 dependency with my experimental sdl2-compat port. But I haven't tested it with any other library/program that uses sdl2 yet.

What do you think about this? Have I missed any other options? 🙂

@Cheney-W
Copy link
Contributor

@vicroms Please take a look.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
category:port-feature The issue is with a library, which is requesting new capabilities that didn’t exist
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants