search

ImGuiNET / ImGui.NET

An ImGui wrapper for .NET.
MIT License
1.8k stars 298 forks source link

Overloads with string parameters should be included in .NET 6.0 build to maintain binary backwards compatibility with .NET Standard 2.0 #436

Closed NoahStolk closed 8 months ago

NoahStolk commented 8 months ago

From this comment: https://github.com/ImGuiNET/ImGui.NET/pull/412#issuecomment-1763295315

Since overloads for ReadOnlySpan<char> have been added, methods using string are no longer included in the .NET 6.0 build, which causes runtime errors in binaries where both versions are used through different projects.

How to reproduce:

To fix this we'd need to include string overloads again. However, this results in the string overloads taking precedence over ReadOnlySpan<char> which means you'd have to write ImGui.Text("Hello, world!".AsSpan()); instead of ImGui.Text("Hello, world!"); (I think).

Related issue: https://github.com/zaafar/ClickableTransparentOverlay/issues/48

zaafar commented 8 months ago

Is there any reason why someone would use both 2.0 and 6.0 at the same time in different projects?

NoahStolk commented 8 months ago

I feel like there could be many reasons. I guess it could make sense to build a .NET Standard 2.0 library with common or shared ImGui rendering logic that can then be re-used in any other project regardless of whether it is using .NET Standard 2.0, .NET 7, or .NET Framework.

I realize now from the other thread that if you multi-target a library, you need to be binary backwards compatible (meaning the method signatures need to match) with each "lower" target. So in this case, everything that is available for .NET Standard 2.0 should also be available for .NET 6.0. (Obviously not the other way around, since you can't target a .NET 6 project from a .NET Standard 2.0 one.)

I think I've found a workaround for anyone not able to upgrade to .NET 6 running into this issue:

Remove ImGui.NET from the other project that is on .NET 6 or 7. This makes sure ImGui.NET will be coming as a transitive dependency from the other .NET Standard 2.0 project. I'm fairly sure the .NET Standard 2.0 build of ImGui.NET would then be resolved and there would not be any method signature mismatches.

Perksey commented 8 months ago

We are getting widespread reports from Silk.NET users that this change has broken our downstream consumers.

Consider this:

  1. Helper library targets .NET Standard 2.0, expecting the string variant to be there
  2. When the helper library is published, it compiles successfully with a metadata reference to the string version.
  3. Because NuGet is smart, it automatically picks the most appropriate TFM for each transitive dependency.
  4. The helper library is pulled in targeting .NET Standard 2.0, with a string variant metadata reference.
  5. The ImGui.NET library is pulled in targeting .NET 6 (or whatever), with a span variant metadata reference.

Please consider all implications of having multi-targeted APIs like these. For compatibility, there should always be a lowest common denominator API set to prevent weirdness like this. For maintenance reasons, if this is not rectified and/or more changes like these slip through the cracks, we will sunset our official integrations with ImGui.NET and possibly make our own bindings. I appreciate that everyone's doing this in their free time and may not be able to watch that closely, but so are we and if it becomes more trouble than it's worth we'll just remove it.

NoahStolk commented 8 months ago

I have opened a PR to fix this. The sample now includes a .NET Standard 2.0 library with a single method that calls ImGui.Text. This currently fails on the master branch, but works on my branch with the updated code generator.

Compatibility with ReadOnlySpan<char> is also still included, so this doesn't break anything on the other end either.

Please let me know if I've missed anything else, and apologies for the issues.

@zaafar I'll let you handle the version bump and the release if everything looks good to you.

zaafar commented 8 months ago

Overall it's looking good, I will merge it and release a new version by end of today.

zaafar commented 8 months ago

PR merged, I am going to release a new version tomorrow.

NoahStolk commented 8 months ago

Thanks @zaafar 👍

zaafar commented 8 months ago

@Perksey try ImGui.NET.1.89.9.3 and let me know if there are issues.

Perksey commented 8 months ago

I don't use Silk.NET, but you can look at the +1s here to determine some testers (I can't see them I'm on mobile)

NoahStolk commented 8 months ago

Thanks for the release! I'll try to get in touch with some users and do my own tests as well.