ImGuiNET / ImGui.NET

An ImGui wrapper for .NET.
MIT License
1.87k stars 304 forks source link

Generate overloads that accept parameters of type ReadOnlySpan<char> rather than string #405

Closed NoahStolk closed 1 year ago

NoahStolk commented 1 year ago

Related issue: https://github.com/ImGuiNET/ImGui.NET/issues/397

Allowing to pass ReadOnlySpan<char> instead of string can reduce a lot of memory allocations on the heap. For example, this allocates 32 bytes:

int value = Random.Shared.Next(0, 10000);
ImGui.Text(value.ToString());

Of course, 32 bytes isn't much, but when you are drawing a lot of text, every frame, this quickly adds up to a lot of allocations resulting in many garbage collections.

With this PR it becomes possible to do things like this:

int value = Random.Shared.Next(0, 10000);
value.TryFormat(_charBuffer, out _);
ImGui.Text(_charBuffer.AsSpan());

This doesn't allocate any memory (tested with GC.GetAllocatedBytesForCurrentThread).

I've made changes to the code generator; no manual changes were done to the generated files. I've also tested the Text method with the ReadOnlySpan<char> overload in the ImGui.NET.SampleProgram. Do I need to do any more manual testing? Also, if this gets released, which version number do I pick?

Please let me know if I've missed anything.

zaafar commented 1 year ago

this is amazing!!! just 1 small feedback before i can merge it.

The way this PR does it is not ideal since it's going to break everyones project who is using imgui.net (including mine, author of this lib, etc etc). This is because this PR replaced the old string overloads with the new span ones. IMO, they should be added as an extra set of methods.

Also, in a different PR/Issue, i am thinking of supporting ReadOnlySpan<byte> as well, that will reduce the unnecessary UTF16->UTF8 conversions (basically the following block can be removed). Again, not part of this PR.

                filename_byteCount = Encoding.UTF8.GetByteCount(filename);
                if (filename_byteCount > Util.StackAllocationSizeLimit)
                {
                    native_filename = Util.Allocate(filename_byteCount + 1);
                }
                else
                {
                    byte* native_filename_stackBytes = stackalloc byte[filename_byteCount + 1];
                    native_filename = native_filename_stackBytes;
                }
                int native_filename_offset = Util.GetUtf8(filename, native_filename, filename_byteCount);
                native_filename[native_filename_offset] = 0;
NoahStolk commented 1 year ago

The way this PR does it is not ideal since it's going to break everyones project who is using imgui.net (including mine, author of this lib, etc etc). This is because this PR replaced the old string overloads with the new span ones. IMO, they should be added as an extra set of methods.

Are you sure it's breaking? There is an implicit conversion from string to ReadOnlySpan<char>, so I believe everything should still compile. https://learn.microsoft.com/en-us/dotnet/api/system.string.op_implicit?view=net-7.0

The samples are still compiling and working as expected as well:

image

CodingMadness commented 1 year ago

functions accepting ReadOnlySpan can accept strings, so its all fine actually.

zaafar commented 1 year ago

sounds good, on the weekend, I am going to test it and then get this merged. thank you!

NoahStolk commented 1 year ago

Hi @zaafar

Do you think we could get this merged soon? Let me know if there's anything I can do to help (like more testing).

zaafar commented 1 year ago

I have released version v1.89.5b since cimgui isn't updated to 1.89.6.

NoahStolk commented 1 year ago

Thanks for the merge! I think I broke the CI because the code generator was updated to .NET 7. Will have a look.

NoahStolk commented 1 year ago

I think this should fix the CI. https://github.com/ImGuiNET/ImGui.NET/pull/408/files