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 5 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
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.

4 participants