ImStrv: Use a string span for consuming strings in API

[rokups]

EDIT Moved to features/string_view branch

WE EXPECT TO MERGE THIS IN 2021 BUT ARE WAITING FOR (1) NEW BINDING GENERATOR (2) INVESTIGATING SWITCHING TO STB_PRINTF IN ORDER TO SUPPORT NON-ZERO TERMINATED FORMATS

This PR implements a custom ImStr string span class and uses it in the API instead of const char* text and const char* text, const char* text_end. PR is a spiritual successor of #683.

Rationale: this change facilitates use of Dear ImGui from languages that do not zero-terminate their strings. So basically rust. Even though rust does have zero-terminated c strings - they are not default and therefore transparent interop between rust strings and this library would require string copying.

Things to note:

• Variadic functions are not covered. Use of custom class breaks static format string analysis of GCC. Ideally these variadic functions should get non-variadic counterparts and user should do formatting on their end (like Text() vs TestUnformatted().
• Most functions from imgui_internal.h that are not expected to be used by vast majority of users are not covered.
• Some functions from imgui_internal.h that take const char* format are not covered for performance reasons.
• Some functions taking arrays of const char* or callbacks are not covered.
• Input*(), Sider*() and Scalar*() functions are covered, but that limits size of format parameter to 63 bytes. Format is copied to a buffer on stack because internal functions expect it to be zero-terminated.
• Bunch of functions taking const char* text, const char* text_end were obsoleted, because ImStr version does exactly same.
• ImStr is implicitly constructible from const char*.
• ImStr is implicitly convertible to bool, yields false when ImStr has no string attached. Yields true if any string is attached, even if string length is 0. This is essentially same as checking if string pointer is null.

[repi]

This looks awesome, we also use IMGUI in Rust where we have string slices instead of null-terminated string pointers. And we also use IMGUI inside a WASM sandbox where we explicitly have to transfer the exact right memory between WASM and the host and where string slices with explicit length are a lot easier, faster, and safer than loose null-terminated string pointers where one have to dynamically determine the length of them for the marshalling.

So would love to see this integrated in to core IMGUI!

[rokups]

Good to hear! Would you care to try this branch with your application? This PR may have issues or things missing. Real world experience would be invaluable for finishing this PR and merging it.

[repi]

Unfortunately it would be a bit tricky for us to try it I think because we use IMGUI through cimgui through imgui-rs which we have our own WASM fork of. So quite a few layers of indirection.

But @Jasper-Bekkers and @MaikKlein in our team here at Embark have more experience with how that works and may be able to give feedback about this PR

[ocornut]

No need to immediately act upon that in PR, but as C++20 is aiming at inconveniently making UTF-8 literal u8"some text" not be const char* anymore but const char8_t*, adding the extra constructor taking const char8_t* would be an added incentive for adopting this.

Another one is that it would facilitate a path where wchar_t could be supported as the default compile-time target, to support e.g. Unreal Engine without utf-8 > wchar_t conversions.

[Jasper-Bekkers]

@ocornut Is this PR something you'd consider merge in in the near future?

[ocornut]

Hello,

Not super “near” future as I don’t have resources to review/tweak it thoroughly the way I’d like (considering concerns such as cognitive and debug overhead for c++ users) but I imagine we should be able to tackle it in 2020, which is the reason I asked Rokas to revive the old PR.

Real world feedback would also be helpful.

If there’s traction, one possibility is to first merge the changed function signatures which constitute the bulk of the diff, so the remaining diff gets easier to update for as long as this is not fulled solved.

[Jasper-Bekkers]

Getting the updated function signatures in would already help us quite a bit. Having strings the way they are now has had quite an impact on imgui-s for us so investigating changes there would also take some time.

[simonvanbernem]

Since the PR only mentions Rust, I just wanted to mention that this would also really benefit my usecase of using imgui from D.

[Kubikx]

"Real world feedback would also be helpful."
I did not go trough all the changes, but I think it can make imgui usable with C++17 and later which are now more and more used.
I found this PR when I was considering if we can use imgui for our next project or not.
So my question is if it will be merged any time soon, or we should seach for another library?
Thanks in advance for the answer.

[ice1000]

This would also be helpful for the Java binding I maintained.

[ocornut]

(moved message from #494, sane to resume talking in the newer issue)

I have a pushed an updated features/string_view branch https://github.com/ocornut/imgui/tree/features/string_view based over latest. Some changes over Rokups's recent is that I've enforced the end pointer to be valid if the begin pointer is valid. This allows us to remove late validation code, we now just expect that both pointers are set together by the constructor.

That gives the extra benefit that with the single-param constructor: literals which are frequent with raw C/C++ usage can now using compile-times strlen.

That's over the general benefit that pulling data from another language or string type which has constant time access to length can now avoid the strlen() alltogether - that's another benefit of this branch.

Some quick measurement done with our testing framework gives it a nice edge in Release build, and Debug build (with all VS security checks) gave it a minor 2-3% slow-down. This is before I made some other tweaks which perhaps made it better. Will do more thorough measurement later.

Things I would like to be able to move forward:

• 1. general testing + thorough performance measurements (Rokups and I will work on that)
• 2. looking at this from the pov public/internal api breaking changes. current version is designed to minimize those (to confirm?) but I believe some of the internal api would be saner to break. (Rokups and I can work on that, but any feedback welcome)
• 3. reestablish a working proof of concept for Rust users and try to have 1-3 people toying with it even quickly.
• 4. figure out how to integrate this with cimgui

Point (3) will need involved Rust users.

Point (4) is interesting to discuss: @bitshifter old cimgui fork replaced all cimgui api with ImStr:
cimgui/cimgui@master...bitshifter:imstr
I do believe this would however be problematic for actual C user. We could have a compile-time option for cimgui to use const char* or ImStr but my gut feeling is that may be create hurdle with people pulling from package manager and not building themselves?

Otherwise cimgui could provide two api with a different prefix. One e.g. igButton() would take const char* + perform the strlen() itself, one e.g. igsButton() would take ImStr or simply two const char*. Since those functions are super thin wrapper I don't feel it would be problematic for main cimgui API. But this scheme would need to be adjusted for non ImGui:: functions a swell: ImDrawList_AddText, `

Either way this needs a solution. As a general feeling I believe that only direct C users would want to use const char* and every indirect users (LUA, C#, Rust, language bindings) would be better off with ImStr.

---

It's too late for 1.80 but tentatively if the right things are in place we could aim for it by 1.81 or 1.82.
The branch being in place it would be ideal to start now working on nailing 3/4 so we have some real world feedback and then by the time we are done with 1.80 we can look at that.

[bitshifter]

It has been a long time since I looked at this. It looks like cimgui is now autogenerated rather than hand written like it was when I forked it. So it will be interesting to see if that "just works" . On the Rust side of things, I think either a #[repr(C)] ImStr struct or a start and end const char* pointers could work, most likely it could be hidden in the imgui-rs FFI layer. I think the reason I went with ImStr was it was less changes than adding start and end pointer parameters everywhere and a bit easier to see where my changes were. At the time there were a few dear imgui methods that already took an end pointer which would have made it a bit harder to keep track of what I'd changed.

[sonoro1234]

As a general feeling I believe that only direct C users would want to use const char* and every indirect users (LUA, C#, Rust, language bindings) would be better off with ImStr.

In LuaJIT-ImGui I would prefer

ig.Text("some_text")

over

ig.Text(ImStr("some_text"))

[ocornut]

In LuaJIT-ImGui I would prefer

Yes of course. From pretty much any language you would do Text("hello") and the bindings would convert that to the ImStr function implicitly/automatically or explicitly (preferable as it can leverage knowing string length without a strlen call).

[ocornut]

FYI there are two things to do here:

• transition to stb_printf (code is done-ish)
• This VS issue is IHMO quite problematic https://developercommunity.visualstudio.com/t/allow-natstepfilter-and-natjmc-to-be-included-as-p/561718 as it add a debugger-stepping burden to every user. If .natstepfilter could be included in a project like .natvis it would solve this issue. Please upvote if you agree (I have genuinely no clue why my final message was downvoted ?)

[ocornut]

Reminder that this VS issue is IHMO the biggest remaining hurdle before merging this.
Upvotes would be appreciated:

https://developercommunity.visualstudio.com/t/allow-natstepfilter-and-natjmc-to-be-included-as-p/561718

As-is, the change seriously hinder debugger experience for C++ users.
If VS gets that fix, we can at least explain to people that they can drag 1 file into their project to alleviate the issue.

[BeastLe9enD]

Hey, I wanted to check the current status of this pull request. Is the VS issue the only thing that prevents merging and how is the status on that?
Also, I saw that there is the https://github.com/ocornut/imgui/tree/features/string_view branch, is this the branch that will be merged into master and can I use this branch until being merged?
Thank you for your effort!:)

[ocornut]

Hey, I wanted to check the current status of this pull request. Is the VS issue the only thing that prevents merging and how is the status on that?

The VS issue is the main friction point remaining (please ask friends to upvote https://developercommunity.visualstudio.com/t/allow-natstepfilter-and-natjmc-to-be-included-as-p/561718 ;).
We need to be able to point people to a solution to not make debug-stepping painful.

Also, I saw that there is the https://github.com/ocornut/imgui/tree/features/string_view branch, is this the branch that will be > merged into master and can I use this branch until being merged?

Yes, this is the branch expected to be merged into master.
This is considered a maintained branch and we regularly rebase it over master (at very minimum every release, but actually every few weeks). I am aware of at least 2-3 declared users.

We would actually appreciate if you used this branch and reported issues, if any.

[nicolasnoble]

We would actually appreciate if you used this branch and reported issues, if any.

I would love to use the branch, but unfortunately, I need the docking one more... I know it may be potentially a lot of work for you, as the two branches may not always merge cleanly (I know they don't at the moment), but is there any chance of a docking+string_view branch? I can offer my help for this if it's a direction you'd want to go.

[ocornut]

I've pushed some work now (on all three branches) to make it possible to merge docking+string_view.
Also a minor fix to imgui_test_suite to build with string_view. For what it is worth, the merge is passing our tests.

[ocornut]

The improvement for natstepfilter have been added on VS roadmap! 🍾

[BeastLe9enD]

is there a way to use this with the docking branch? If no, do you have an estimated time when this is going to be merged (I know eta's are annoying, but would love to use this from rust)

[PathogenDavid]

@BeastLe9enD

is there a way to use this with the docking branch?

features/string_view can be locally merged into docking without issue, and should work with Dear Bindings.

[ZimM-LostPolygon]

Are there plans to merge this in the foreseeable future? I'm writing a wrapper using dear_bindings and just wondering whether maybe I should focus on just the ImStr path instead of the "old and busted" zero-terminated approach.