ImGuiNET / ImGui.NET

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

passing ReadOnlySpan<char> instead of string ?? #397

Closed CodingMadness closed 1 year ago

CodingMadness commented 1 year ago

Title.

Would be super nice if you could rewrite/adapt your C-parser of the ImGui files to accept ReadOnlySpan instead of string, cause this thing is used in Gameloop-code alot obviously and the amount of unnessecary garbage which will be created by "mySpan.ToString()" is enormous. Our code works heavily with spans to avoid tons of tons of allocations.

I hope you consider my writing here.

Best regards

zaafar commented 1 year ago

Good idea, feel free to create PR

CodingMadness commented 1 year ago

Good idea, feel free to create PR

I dont have time to work on that, would be nice if the owner or the actual collabs could work on this. I am nott familar with C

zaafar commented 1 year ago

the owner or the actual collabs could work on this

unfortunately owner won't be able to commit to this repo (in the near future) due to some clause in their contract. As 1 of the collab, i just handle the release process of ImGui.net and is not familiar with C as well.

I think what I am trying to say over here is that the quickest route would be to contribute to this repo yourself. Otherwise, feel free to wait for some collab who knows C & is part of the project that uses ReadOnlySpan a lot.

CodingMadness commented 1 year ago

@zaafar Can you direct me to atleast where would I need to do the changes, can this maybe be done only in his generated .cs files? or does it really have to be in C ?

zaafar commented 1 year ago

Well, I am not expert in this but I will try my best to explain what I know.

NoahStolk commented 1 year ago

I'm also interested in being able to use ReadOnlySpan<char> instead of string. However I think it's worth mentioning that ReadOnlySpan is not supported on .NET Standard 2.0 (only 2.1), so I think we would need to generate overloads that accept ReadOnlySpan<char> and wrap them in #if NETSTANDARD2_1_OR_GREATER directives (correct me if I'm wrong here).

I might give it a try in the future.

CodingMadness commented 1 year ago

@NoahStolk its a valuable effort i feel i currently am thinking to do the same but need to know where to start, zafaar did give a good insight in where to start but I did not yet come to the chance to begin it due to private endevors.

CodingMadness commented 1 year ago

@NoahStolk @zaafar I think I have managed to do it!

Pls check out my repo: https://github.com/Shpendicus/ImGui.NET-master

I think sofar I have done it, I did rather quick so no deep testing, but when i checked roughly the .gen files, it seemed to accept my change!

CodingMadness commented 1 year ago

Now this has only to be checked by the collabs and made a .nuget package.

zaafar commented 1 year ago

can't see your repo.

CodingMadness commented 1 year ago

can't see your repo.

can u check now?

zaafar commented 1 year ago

please create a PR to this repo so i can see what changed.

CodingMadness commented 1 year ago

yea sadly did not do that, will do tomorrow

NoahStolk commented 1 year ago

@NoahStolk @zaafar I think I have managed to do it!

Pls check out my repo: https://github.com/Shpendicus/ImGui.NET-master

I think sofar I have done it, I did rather quick so no deep testing, but when i checked roughly the .gen files, it seemed to accept my change!

Nice! Just had a look at your changes. It's fairly difficult to see what exactly has changed with all the cleaning up and style modifications (I do think the generator needs cleaning up though -- but maybe it'd be better to do it separately without any functionality changes so we have a clear diff for this feature).

I also don't think it is desirable to drop .NET Standard 2.0 support, as the README states:

ImGui.NET is a .NET Standard library, and can be used on all major .NET runtimes and operating systems.

I think we'd have to figure out how to generate overloads for ReadOnlySpan<char> and only include them if the target framework supports it.

CodingMadness commented 1 year ago

@zaafar sadly due to some family accident, I have no time whatsoever to make a PR for this endevaor. I please you both to look at the changes (its not a lot, i will find the files and call them here for easier use ) and make a PR yourself!

Would be super good if this could make it into the current release

Also, @NoahStolk you guys would need to write then conditionals for that, regarding this quote here:

I think we'd have to figure out how to generate overloads for ReadOnlySpan and only include them if the target framework supports it.

NoahStolk commented 1 year ago

I have opened a PR with minimal changes to the code generator. It now generates overloads accepting ReadOnlySpan<char> when targeting .NET Core 2.1+ / .NET Standard 2.1.

https://github.com/ImGuiNET/ImGui.NET/pull/405

zaafar commented 1 year ago

moving this and this discussion back to the original issue.

Apologies for missing that. Are you sure the IsEmpty check fixes it? Both null and string.Empty result in an AccessViolationException, even with IsEmpty. I'm not sure why yet.

yeah, null is working fine for me, it's just string.Empty that is resulting in Exception. I am basically compiling and running this project and exception is happening on this line.

What code are you running where null is also giving exception?

NoahStolk commented 1 year ago

I have opened 2 more (draft) PRs to fix some issues:

https://github.com/ImGuiNET/ImGui.NET/pull/409 https://github.com/ImGuiNET/ImGui.NET/pull/410

moving this and this discussion back to the original issue.

Apologies for missing that. Are you sure the IsEmpty check fixes it? Both null and string.Empty result in an AccessViolationException, even with IsEmpty. I'm not sure why yet.

yeah, null is working fine for me, it's just string.Empty that is resulting in Exception. I am basically compiling and running this project and exception is happening on this line.

What code are you running where null is also giving exception?

@zaafar I added some test code to ImGui.NET.SampleProgram ( https://github.com/ImGuiNET/ImGui.NET/pull/410/files ). Both null and empty string crash for me on this branch.

This is using the Text method.

image

zaafar commented 1 year ago

FYI: I have just released 1.89.5.1 with all the changes you have done up till now. Lets try to fix the rest of the issues before I release 1.89.6.0 (which is blocked on https://github.com/cimgui/cimgui still on 1.89.5)

NoahStolk commented 1 year ago

ImGui.Text(null) also crashes on 1.89.5. Calling ImGuiNative.igText with a null pointer in general just does not work.

Also, I found out that we can have all the ReadOnlySpan<char> overloads without the preprocessor directives. The System.Memory package provides the ReadOnlySpan struct and is compatible with .NET Standard.

I will probably close the two drafts and open a new PR, which:

NoahStolk commented 1 year ago

I have opened a new PR which fixes all the current issues and closed the two drafts.

https://github.com/ImGuiNET/ImGui.NET/pull/412

NoahStolk commented 1 year ago

@zaafar FYI: I finished and re-opened my PR to apply ReadOnlySpan<char> to ImGui.Manual.🙂

https://github.com/ImGuiNET/ImGui.NET/pull/409

Anything else that needs to be done before this issue can be closed?

zaafar commented 1 year ago

That's the last thing.