Use ReadOnlySpan<char> instead of string

[NoahStolk]

This PR is basically a rework of my original PR: https://github.com/ImGuiNET/ImGui.NET/pull/405

Changes compared to my original PR:

• Instead of generating code with preprocessor directives, the System.Memory package has been installed which allows for use of the ReadOnlySpan struct without dropping support for .NET Standard.

New changes:

• ImGui.Manual and ImDrawList.Manual now support ReadOnlySpan<char> as well.
• The overload for Encoding.UTF8.GetByteCount that accepts ReadOnlySpan<char> is unfortunately not present in System.Memory, so I had to add an internal method to the Util class to make it work. This has been applied to both generated and manual ImGui code.
• I have updated the following dependencies, otherwise we could not use the latest version of System.Memory:
  • System.Buffers: 4.4.0 -> 4.5.1
  • System.Numerics.Vectors: 4.4.0 -> 4.5.0
  • System.Runtime.CompilerServices.Unsafe: 4.4.0 -> 6.0.0

Questions:

• What should be done with the ref string parameters in ImGui.Manual.cs?
• The previous PR introduced an issue that would cause string.Empty to crash the ImGui.Text method for instance. This has been fixed by simply returning 0 when the span was empty. Perhaps we need to do this in more places.
• I feel like it might be a good idea to set up a unit test project for the Util methods. Thoughts?

Both samples are running fine (but I have not done any extensive testing).

[zaafar]

there is no point in upgrading/adding deps if we can avoid it. since .net libs specify the minimum .net core dep version. Its due to the same reason the lib is on netstandard2.0; rather than latest .net7 (i.e. so lib reaches more users).

I actually like your other two (closed) PRs, is there any reason why we move to this new method (which add more deps)? Let me try to merge them with minor modifications.

[NoahStolk]

there is no point in upgrading/adding deps if we can avoid it. since .net libs specify the minimum .net core dep version. Its due to the same reason the lib is on netstandard2.0; rather than latest .net7 (i.e. so lib reaches more users).

I'm not sure if I understand. It's still targeting .NET Standard 2.0, so we're not dropping any support and everyone can continue using the library.

since .net libs specify the minimum .net core dep version.

Could you explain this in more detail? As far as I understand, all of these dependencies are compatible with .NET Standard 2.0, and I don't think this has anything to do with .NET Core necessarily.

I can see why updating the libs might be an issue for some people that also have direct package dependencies on System.Buffers etc, and require an explicit older version of it. This update may cause a conflict in NuGet. Although that would only be applicable to projects that are also on .NET Standard I guess? Also I'm not sure what the breaking changes for the System.Runtime.CompilerServices.Unsafe dependency are.

I actually like your other two (closed) PRs, is there any reason why we move to this new method (which add more deps)?

Mainly because managing all those preprocessor directives for the manual methods is painful. I think having so many manually written functions is already a sign of poor maintainability, and I didn't want to make it worse. I thought this would be a nice opportunity to also upgrade the dependencies.

I think we have a couple options here:

1. Downgrade the dependencies to their original versions and use an older version of System.Memory that is compatible with those versions, and proceed with this PR.
2. Proceed with this PR as is. (I don't really see a downside other than some rare package version conflict (if that would happen at all), but please point out anything that I might be missing.)
3. Close this PR and reopen the previous PRs.

Let me try to merge them with minor modifications.

Fine with me, but I wouldn't merge the string.Empty fix right now as I don't think it works. I can open another one with the zero-length check.

[zaafar]

Fine with me, but I wouldn't merge the string.Empty fix right now as I don't think it works. I can open another one with the zero-length check.

this fixes the issue on my side. let me know if this doesn't work on your side. https://github.com/ImGuiNET/ImGui.NET/pull/413

[NoahStolk]

Fine with me, but I wouldn't merge the string.Empty fix right now as I don't think it works. I can open another one with the zero-length check.

this fixes the issue on my side. let me know if this doesn't work on your side. #413

Works for me. I will close this PR if using System.Memory is undesired.

[fantasydr]

I looked into this PR since, following this commit, the sample imgui project for Silk.NET starts crashing upon launch. https://github.com/dotnet/Silk.NET/commit/05019230c9bd67d9008f7183a8eae6675a9ac418 @NoahStolk

System.MissingMethodException:   
'Method not found: 'ImGuiNET.ImFontPtr  
ImGuiNET.ImFontAtlasPtr.AddFontFromFileTTF(System.String, Single, ImGuiNET.ImFontConfigPtr, IntPtr)'.'

I'm a bit unsure. I noticed that the API version number changed from 1.89.5 to 1.89.5.1 after this PR. Based on my understanding, if there's an incompatible API change, wouldn't it be more appropriate to update the major version? Or at the very least, the minor version?

Alternatively, perhaps consider introducing a new API and retaining the old one. Removing the (string) version seems a bit unconventional.

Is this something only happened on my machine? Because I also found other issues related to this API change: https://github.com/zaafar/ClickableTransparentOverlay/issues/48

From my point of view, now the library works differently on different dotnet version. On netstandard2.0, it uses (string) interfaces. On 2.1+, it uses (span) interfaces.

This will cause issues in a project targeting multi framework. In the Silk.NET scenario, the sample project is targeting net6.0, but it is referencing a lib targeting both 2.0 and 6.0. The project compiled but it will throw this exception during runtime because only the 6.0 dll are shipped with the progarm. I'm not sure why there is no compile error because obviously the interfaces are missing. @NoahStolk did you successfuly run the sample after the version upgrade?

Also this statement seem not true, the 2.1+ users won't get the old (string) interfaces, they are forced to use the new (span) interface this commit introduced.

I'm not sure if I understand. It's still targeting .NET Standard 2.0, so we're not dropping any support and everyone can continue using the library.

I'd recommend retaining the original interface rather than removing them entirely, unless there's a major version update.

[NoahStolk]

@fantasydr I found the problem. The only way I could reproduce this is by doing the following:

1. Create 2 projects; one .NET Standard 2.0 and one .NET 6 or 7. Reference the ImGui.NET in both projects, either through ProjectReference or PackageReference.
2. Call an ImGui method with a string parameter from the .NET Standard 2.0 library.
3. Call the library from the other project. It will crash because the .NET 6.0 version of ImGui.NET has been compiled and included in the binary, while the .NET Standard 2.0 library expects the methods present in the .NET Standard 2.0 version.

I'm a bit unsure. I noticed that the API version number changed from 1.89.5 to 1.89.5.1 after this PR. Based on my understanding, if there's an incompatible API change, wouldn't it be more appropriate to update the major version? Or at the very least, the minor version?

The ImGui.NET version number is kept in sync with ImGui itself, which is why that was not done. Also, this should not have been a breaking change since there is an implicit conversion from string to ReadOnlySpan<char>: https://learn.microsoft.com/en-us/dotnet/api/system.string.op_implicit?view=net-7.0

Meaning that it is source backwards compatible. However, by not including the original methods in the .NET 6.0 binary we unintentionally made it binary backwards incompatible.

I'm not a maintainer here though so I don't really make the decisions on versioning the library.

Alternatively, perhaps consider introducing a new API and retaining the old one. Removing the (string) version seems a bit unconventional.

We thought this would not be necessary, since you can still pass strings as stated above, but in the case where multiple projects with different target frameworks reference ImGui.NET it makes sense that this would be a binary breaking change. I will look into this.

@NoahStolk did you successfuly run the sample after the version upgrade?

I did, the samples still work fine right now, because there is no .NET Standard 2.0 library with calls to ImGui.NET included in the binary.

Also this statement seem not true, the 2.1+ users won't get the old (string) interfaces, they are forced to use the new (span) interface this commit introduced.

It is true from a source code perspective. Nobody is forced to use ReadOnlySpan<char> since you can still use string as stated above.

I've opened a new issue regarding this problem: https://github.com/ImGuiNET/ImGui.NET/issues/436

[fantasydr]

For me I just git clone the Silk.NET and compile the imgui sample on my Win10 machine with vs2022. It will crash at runtime.

The new interface is an improvement, especially in game development (an area I'm also working in). However, source code compatibility alone might not be sufficient for an API or a library that's referenced by many external projects. Just sharing my thoughts.

I'm glad you could reproduce this issue. I'll leave this to you and the maintainer.

[NoahStolk]

However, source code compatibility alone might not be sufficient for an API or a library that's referenced by many external projects.

I agree, but I'd like to make it clear that it was never anyone's intention to make a binary backwards incompatible change. This was just an oversight. If we realized this problem earlier I'd have solved the code generation differently. Luckily the issue shouldn't be too difficult to fix though.