ImStr / string span support

[ocornut]

Followup to ocornut/imgui#3038 (comment)

We are planing (maybe around 1.82) to add support for spring span everywhere in Dear ImGui.
This mean that strings won't need to be zero-terminated.
This will have multiple benefits:

• Easier use from Rust (which doesn't zero-terminate string), currently we are losing on Rust users partly before of that.
• From C++, the length of literals can be optimized out by the compiler, so ImGui::Text("Hello") will compile as ImStr("Hello", 5) as the compiler can optimize the strlen() down to 5. This has been proven to give some benefits. For non-literal, the call to strlen() is done once at call site.
• From any language/tech where the string length is stored, which are most high-level languages, but even in C/C++ custom string types or e.g. std::string, we can technically remove the strlen() call since we already know the length of strings. Right now people are doing:

std::string some_std_string = "Hello";
ImGui::Text(some_std_string.c_str());  // this will internally do another strlen() in Text

Instead they could do:

std::string some_std_string = "Hello";
ImGui::Text(some_std_string);  // Use ImStr(const std::string& s) : ImStr(s.c_str(), c.length()) {} constructor, faster to write, no strlen

However this will need support from cimgui.

using cimgui from C could be even more difficult.

I believe the best solution is that cimgui provide TWO api, with both always available.

• (1) One new API that takes ImStr or simply two const char* (maybe ImStr is cleaner)
• (2) One old and C-friendly API that takes const char* and inline call the first API: igText(const char* str) { igsText(str, str + strlen(str); }

User who actually write code in C will want to use API (2).
Old bindings can use API (2). But we want improving bindings to use API (1).

So from any languages, such as Lua or also .Net/C#, we want user to never see anything about ImStr() (or double const char*) and use their normal string, and the language back-end should automatically do this conversion and use the native string type length to avoid strlen call as a bonus.

How would that work with Lua-JIT and e.g. C# ? (cc @mellinoe)

[ice1000]

spring span

Did you mean string span?

[sonoro1234]

Work just begun in https://github.com/cimgui/cimgui/tree/string_view

[sonoro1234]

Work finished and examples working!!

[sonoro1234]

While working on LuaJIT wrapping I realized that ImStr to const char* susbtitution would clash with already present substitutions on imgui. So when the signature resulting from the substitution is already present in imgui the const char* version is not added now.

The place where this substitution is made has changed: now the new signature is added before computation of overloading names so that naming conventions have changed.

[ocornut]

I find it strange to name the const char* function with a Chpt suffix.
I think the existing C functions should exist with the same name as always existed.
And new ImStr functions can use a different PREFIX. e.g. igs or is intead of ig.
Different prefix will avoid redundant function displayed twice in text completion.

[sonoro1234]

And new ImStr functions can use a different PREFIX. e.g. igs or is intead of ig.

This is difficult to achieve but the postfixes can be changed: const char* -> Str and ImStr -> STR for example

Different prefix will avoid redundant function displayed twice in text completion.

Not sure to understand

[sonoro1234]

found a problem in

IMGUI_API void          Columns(int count = 1, ImStr id = NULL, bool border = true);

with C versions

CIMGUI_API void igColumnsStr(int count,ImStr id,bool border);
CIMGUI_API void igColumnsChpt(int count,const char* id,bool border);

Not being a pointer ImStr id does not accept nil (NULL) as argument so that NULL cant be a default argument from C in this case.

[ocornut]

This is difficult to achieve

Well it seems like the only reasonable answer to this problem ihmo.

Not sure to understand

Using suffixes is very polluting for anyone using completion in their IDE (everything will now appear twice).

found a problem in
IMGUI_API void Columns(int count = 1, ImStr id = NULL, bool border = true);

Interesting issue, thanks.

It'll work properly in C++ but be misleading.. ImStr id = NULL is same as ImStr id = ImStr(NULL) and so the if (id) in the columns code will work. I can fix the prototype (search for ImStr[^,]*NULL and replace with =ImStr() but if I fix all ImStr xxx = NULL prototype into ImStr xxx = ImStr() (more correct) then parsing may need to adjusted on your side?

[sonoro1234]

Well it seems like the only reasonable answer to this problem ihmo.

So you want to change the postfixes as explained?

It'll work properly in C++ but be misleading.. ImStr id = NULL is same as ImStr id = ImStr(NULL) and so the if (id) in the columns code will work. I can fix the prototype (search for ImStr[^,]*NULL and replace with =ImStr() but if I fix all ImStr xxx = NULL prototype into ImStr xxx = ImStr() (more correct) then parsing may need to adjusted on your side?

Yes, that would need taking care of it in LuaJIT-ImGui wrapping (not in cimgui where defaults and overloadings doesnt matter)
But the main problem is in the overloadings management: If the second parameter is nil there is no way to tell if we want the ImStr or the char* version.

[ocornut]

So you want to change the postfixes as explained?

No I think we should change the prefixes only.

But the main problem is in the overloadings management: If the second parameter is nil there is no way to tell if we want the ImStr or the char* version.

In that case it doesn't matter which one is being called.

[sonoro1234]

No I think we should change the prefixes only.

The main undefinition is that not all cimgui functions have an ig prefix

CIMGUI_API ImGuiID ImGuiWindow_GetIDStr(ImGuiWindow* self,ImStr str);
CIMGUI_API ImGuiID ImGuiWindow_GetIDChpt(ImGuiWindow* self,const char* str);
CIMGUI_API ImGuiID ImGuiWindow_GetIDChptChpt(ImGuiWindow* self,const char* str,const char* str_end);
CIMGUI_API ImGuiID ImGuiWindow_GetIDPtr(ImGuiWindow* self,const void* ptr);
CIMGUI_API ImGuiID ImGuiWindow_GetIDInt(ImGuiWindow* self,int n);

When functions are methods from a struct they take the struct prefix.
Also not taking ig prefix are the third party widgets (implot and so on)

One solution would be to use S as a prefix:

CIMGUI_API void igColumns(int count,ImStr id,bool border);
CIMGUI_API void SigColumns(int count,const char* id,bool border);

[sonoro1234]

It'll work properly in C++ but be misleading.. ImStr id = NULL is same as ImStr id = ImStr(NULL) and so the if (id) in the columns code will work. I can fix the prototype (search for ImStr[^,]*NULL and replace with =ImStr() but if I fix all ImStr xxx = NULL prototype into ImStr xxx = ImStr() (more correct) then parsing may need to adjusted on your side?

As in any case I will have to adapt the code, it is not worthwhile that you make this change, I will take care of it on the LuaJIT-ImGui side.

[sonoro1234]

Just pushed to string_view with the prefix naming strategy and char* -> Str, ImStr -> STR (for backward compability)

[sonoro1234]

@ocornut gentle ping for last three messages.

[ocornut]

Just pushed to string_view with the prefix naming strategy and char* -> Str, ImStr -> STR (for backward compability)

Unless I'm misunderstanding something, renaming those functions means there is NO backward compatibility?

(PS: I am considering renaming ImStr to ImStrv on C++ side. This would clarify that it is a non-owning string view and not a full string holder)

Let's put everything on the table.
Here are the three cases the current commit output:

bool igIsPopupOpenSTR(ImStr str_id,ImGuiPopupFlags flags);
bool SigIsPopupOpenSTR(const char* str_id,ImGuiPopupFlags flags);

const char* ImFont_CalcWordWrapPositionASTR(ImFont* self,float scale,ImStr text,float wrap_width);
const char* SImFont_CalcWordWrapPositionASTR(ImFont* self,float scale,const char* text,float wrap_width);
const char* ImFont_CalcWordWrapPositionAStr(ImFont* self,float scale,const char* text,const char* text_end,float wrap_width);

int igImStrcmpStr(const char* str1,const char* str2);
int igImStrcmpSTR(ImStr str1,ImStr str2);

• Prefix are inconsistent and suffixes are duplicates.
• Str vs STR is not a sensible diffenciating naming scheme.
• I don't understand why we have ImFont_XXXStr functions vs igImStrcmpXXXX functions. Shouldn't the ImXXXX function not have a prefix in the existing cimgui version? Is it because of a mangling/naming conflicts with C++ version?
• I never understood why the initial prefix was ig and not ImGui_ to be entirely consistent with other class/systems available.
• The simpler naming should be for const char* functions since those are the ones which can be called by 99% users using hand-written C source code. For the functions generally called by backends it doesn't matter what the C name is.

I tried different things and changed my mind.
Suffix may be better. The reason is not all functions need strings. Many API can exist without the duplicate functions.

Right now here's what I am considering:

bool ImGui_IsPopupOpen(const char* str_id,ImGuiPopupFlags flags);
bool ImGui_IsPopupOpen_Strv(ImStrv str_id,ImGuiPopupFlags flags);

const char* ImFont_CalcWordWrapPositionA(ImFont* self,float scale,const char* text,float wrap_width);
const char* ImFont_CalcWordWrapPositionA_Strv(ImFont* self,float scale,ImStrv text,float wrap_width);

int ImStrcmp(const char* str1,const char* str2);
int ImStrcmp_Strv(ImStr str1,ImStrv str2);

Note:

• The Strv functions are used 99% of the time by language backends (LUA, C# etc..), not called directly by much hand-written code.
• The non-Strv functions are used 99% of the time by C users.
• But C users can call the Strv function if they need to provide a range.
• Because not all functions in the API use strings, not all functions will have a _Strv variant. We need suitable metadata for backends to decide that can map e.g. IsItemHovered [any language] > IsItemHovered [c side] but need to map Button [any language] -> Button_Strv [c side]
• CalcWordWrapPositionA doesn't have a double char* version since that's duplicate with the Strv version.
• The non-Strv functions should be static inline calling the Strv functions

About ig>ImGui_ prefix and backward compatibility.

• I think doing this work would be a good opportunity to fix the naming ig to ImGui_ but this is an optional idea.
• For backward compatibility for C users, we can probably add optional header e.g. cimgui_backward_compat.h (always built by cimgui) which automatically list #define igIsPopupOpen ImGui_IsPopupOpen and maybe other things.
• If we omit this step then we can keep the old prefix for old API:

bool igIsPopupOpen(const char* str_id,ImGuiPopupFlags flags); // classic api
bool ImGui_IsPopupOpen_Strv(ImStrv str_id,ImGuiPopupFlags flags); // string view api

[sonoro1234]

Perhaps would be useful to explain how naming is managed in cimgui:

1 - All the functions present in imgui take a prefix from a function getCname that can be given to the parser that takes structname (if aplicable) or "", namespace and funcname and that provides a cimguiname. (Actual configuration: In the case there is a struct name that will be the prefix, if there is not stuct name: if namespace is ImGui set ig prefix, if namespace is not ImGui use it as the prefix)

2 - After that when an imgui function has overloadings a postfix is added by functions name_overloadsAlgo and typetoStr (which just translates types to more convenient strings) according to its arguments to guaranty that there are no name clashes. This will be ov_cimguiname.

In order to add const char* versions of ImStr there are two possible paths:

A - Add this function before name_overloadsAlgo so that its name will be made the same way as other functions
B - Do it after name_overloadsAlgo and make its name by adding some string (prefix or postfix) to the ov_cimguiname of the function it is derived from. (could be made that string is added to original function instead of the derived one)

Actually it is done by the B path so we have for ImGui::IsPopupOpen
first two ones provided by overload naming and third one as a ov_cimguiname modification

CIMGUI_API bool igIsPopupOpenID(ImGuiID id,ImGuiPopupFlags popup_flags); //from imgui_internal.h
CIMGUI_API bool igIsPopupOpenSTR(ImStr str_id,ImGuiPopupFlags flags); //from imgui.h
CIMGUI_API bool SigIsPopupOpenSTR(const char* str_id,ImGuiPopupFlags flags); //derived const char* version

if name change is made to the original and not to the derived it would be

CIMGUI_API bool igIsPopupOpenID(ImGuiID id,ImGuiPopupFlags popup_flags); //from imgui_internal.h
CIMGUI_API bool igIsPopupOpenSTR_Strv(ImStr str_id,ImGuiPopupFlags flags); //from imgui.h
CIMGUI_API bool igIsPopupOpenSTR(const char* str_id,ImGuiPopupFlags flags); //derived const char* version

@ocornut Is this the desired one?

with path A it would be

CIMGUI_API bool igIsPopupOpenID(ImGuiID id,ImGuiPopupFlags popup_flags);
CIMGUI_API bool igIsPopupOpenSTR(ImStr str_id,ImGuiPopupFlags flags);
CIMGUI_API bool igIsPopupOpenStr(const char* str_id,ImGuiPopupFlags flags);

And for ImGui::ImStrcmp
Both names provided by overload naming and const char* version generation skipped because there was a signature already present that would clash.

CIMGUI_API int igImStrcmpStr(const char* str1,const char* str2);
CIMGUI_API int igImStrcmpSTR(ImStr str1,ImStr str2);

There are quite things that can be changed (ImGui prefix and others) but not everything is possible with this scheme. (steps 1 and 2 and options A or B)

[sonoro1234]

Now I am answering some particular questions you made before, but the main issue should be a naming proposal according to the scheme explained above

Unless I'm misunderstanding something, renaming those functions means there is NO backward compatibility?

By backward compability I meant using the same translation const char* -> Str as was used before so that if generation is done for imgui 1.80 or older, names will be the same they were with old generator.

Str vs STR is not a sensible diffenciating naming scheme.

Can be changed. Which translation do you prefer instead of const char* -> Str, ImStr -> STR?

CalcWordWrapPositionA doesn't have a double char* version since that's duplicate with the Strv version.

In imgui.h there is inline const char* CalcWordWrapPositionA(float scale, const char* text, const char* text_end, float wrap_width) So it appears in cimgui

The non-Strv functions should be static inline calling the Strv functions

What is the problem in how it is done now? They just call the ImStr version expecting implicit conversion, if they called the Strv versions there would be two levels of indirection. (C -> C -> C++)

I think doing this work would be a good opportunity to fix the naming ig to ImGui_ but this is an optional idea.

It can be changed if you will. All the binding makers are making string substitution from ig to something.
In LuaJIT-ImGui I just drop it so that this can be done right now:

local ImGui = require"imgui.glfw"
...
ImGui.Begin("name")

[ocornut]

@sonoro1234 So that you know - when you make post and edit the answers afterwards we don't see the edits in e-mail notification. I generally only read message in e-mails. There's an enormous gap between initial post and after edit so good to be mindful of this :)

[ocornut]

Thanks for the details.
I did indeed forgot that we need to add suffixes to handle the overloads....

There are quite things that can be changed (ImGui prefix and others) but not everything is possible with this scheme. (steps 1 and 2 and options A or B)

To solve the naming conflict with loose functions (not part of ImGui:: or some struct) If it was up to me I would use a c prefix everywhere:

int cImStrcmp(const char* str1,const char* str2);
int cImStrcmp_Strv(ImStrv str1,ImStrv str2);

and then maximum consistency

void cImGui_Button(...)

When the prefix is added by the generator because of overloads then there's always a type suffix:

bool cImGui_IsPopupOpen_ID(ImGuiID id,ImGuiPopupFlags popup_flags);
bool cImGui_IsPopupOpen_Str(const char* str_id,ImGuiPopupFlags flags);
bool cImGui_IsPopupOpen_Strv(ImStrv str_id,ImGuiPopupFlags flags);

But if a function doesn't have overload it's only needed for Strv?

bool cImGui_SomeFunction(const char* ...);
bool cImGui_SomeFunction_Strv(ImStrv .....);

It can be changed if you will. All the binding makers are making string substitution from ig to something.
In LuaJIT-ImGui I just drop it so that this can be done right now:

I am talking about the C-side function/symbols name. I don't think it should be changed at will.
Further substitution can be done by the non-C language wrapper.

[sonoro1234]

@sonoro1234 So that you know - when you make post and edit the answers afterwards we don't see the edits in e-mail notification. I generally only read message in e-mails. There's an enormous gap between initial post and after edit so good to be mindful of this :)

It is almost imposible for me not to edit messages, specially when they are large. The only solution would be to make a new post and delete the old one. I only use github and notifications alert in order to read them.

To solve the naming conflict with loose functions (not part of ImGui:: or some struct) If it was up to me I would use a c prefix everywhere:

It seems right to me. But in the case of ImStrcmp being both present in imgui and generated by name_ overloadsAlgo the result would be

int cImStrcmpStr(const char* str1,const char* str2);
int cImStrcmpStrv(ImStrv str1,ImStrv str2);

Where I understand that for typetoStr you choose: char* -> Str and ImStr -> Strv (I have dropped _ which is mainly used for separing namespaces and strucnames as well as for adding _Strv to added const char* function versions not present in imgui )

But if a function doesn't have overload it's only needed for Strv?

In the case that there are not overloads for a function, no postfixes are added.
So, in case we have only:

ImGui::SomeFunction(ImStrv .....);

The result of cimgui would be (after adding const char* version and appending _Strv to it)

cImGui_SomeFunction(ImStrv .....);
cImGui_SomeFunction_Strv(const char*.....);

I am talking about the C-side function/symbols name.

I have the impression that very few people are using cimgui just for writing C programs.
Main usage are bindings to other languages.
But for the prefixes c and ImGui perhaps a quest issue could be opened. (Just in case)

For the ImStrv -> Strv translation I would better wait until ImStr is converted to ImStrv in features/string_view, meanwhile it would remain ImStr -> STR

[sonoro1234]

Pushed changes just for _Strv postfix in const char* versions of ImStr

[sonoro1234]

To anyone that wants to try ImStr @repi @Jasper-Bekkers @simonvanbernem ?
Naming issues shouldnt be a stopper to try cimgui/string_view branch.

[ocornut]

It is almost imposible for me not to edit messages, specially when they are large. The only solution would be to make a new post and delete the old one.

Fine to make edits/improvement to an existing post, but if you create a innocent 2 lines post and then edit it into a 2 pages post with different ideas and new questions, then people are more likely going to miss on it.

In the case that there are not overloads for a function, no postfixes are added.
So, in case we have only:

I think we are on the same page.

I am talking about the C-side function/symbols name.
I have the impression that very few people are using cimgui just for writing C programs.
Main usage are bindings to other languages.

It may be true but we still need to serve that population. To be honest I don't have metrics of how many C users vs C# vs LUA users so we are only guessing, but I see C users on twitter.

For the ImStrv -> Strv translation I would better wait until ImStr is converted to ImStrv in features/string_view, meanwhile it would remain ImStr -> STR

While push the rename soon.

[ocornut]

Now renamed ImStr to ImStrV in the branch full history, sorry for the delay.

[sonoro1234]

While trying to generate an error was found:

typedef void* (ImGuiMemAllocFunc)(size_t sz, void* user_data);

Shouldn't it be typedef void* (*ImGuiMemAllocFunc)(size_t sz, void* user_data);?

[sonoro1234]

It is a function typedef instead of a function pointer typedef, it is perfectly legal but generates more ambiguities for the parser.
typedef void* ImGuiMemAllocFunc(size_t sz, void* user_data);
would also be legal but even worse for the parser.

Now working on the parser for that!!

[ocornut]

Will fix it to be consistent with other function pointer typedefs, sorry.

[sonoro1234]

Perhaps not necessary, I already did the changes for allowing function typedefs (not function pointer typedefs) and it seems not to break anything

For ImStrv the branch with all changes is now https://github.com/cimgui/cimgui/tree/string_view2

[ocornut]

I think it should be 1 task doing both things - or whatever solves the problem - you seem to insist that the current program structure cannot be reworked, so I'm not sure how to move forward.

[sonoro1234]

1 task doing both things

one task doing two things is actually two tasks

[sonoro1234]

After those two tasks are done: if there are just two overloads and one isSTRV_S drop postfix on the other.
This could solve the igButton issue.

[sonoro1234]

But this would break what I am doing in LuaJIT: when there are overloads a generic function is generated without postfixes that calls the right overload according to argument types. There would be a name clash.

[sonoro1234]

Unless the opposite is done in LuaJIT-ImGui and all other cimgui bindings...

[ocornut]

Two questions for short term:

(1) Imagine if we released new version of dear imgui today using ImStrv: without even supporting the full Strv function could today's branch of cimgui easily provide 100% of the same functionality as today with const char* in C prototypes? (so in theory cimgui.h output would be 100% the same as today, and even cimgui.cpp can be 100% the same since as today in C++ side the const char* can be used to create a ImStrv) ? When you parse the header file it is equivalent as if you replaced ImStrv with const char* everywhere. In theory if I run cimgui generator on imgui from master, and then build/run cimgui with imgui from string_view branch it should work already. We can just ensure that generator can do this replacement.
That can ensure we can release new dear imgui today or next week/month and as a base everything can work. Anything else can be future improvement (dual api etc. discussed in messages above)

(2) Could you provide a replacement to allow supporting ImStrv label = NULL in C++ code?

Not being a pointer ImStr iddoes not accept nil (NULL) as argument so that NULL cant be a default argument from C in this case.

Potential replacement could be replacing ImStrv .* = NULL with ImStrv .= ImStrv() in the loader before parsing?

Right now I don't think the function names discussed in message above are good-enough for user, so if we focus on (1) then cimgui can ship today to support an updated dear imgui (and then we can release dear imgui with ImStrv) and we can give it more time to solve those issues. We will investigate writing a "v3" of cimgui to see if we can solve those problems too.

Thank you.

[sonoro1234]

Not completely sure but I think that the only difference between master or docking_inter branches of cimgui and string_view2 is the naming strategy of functions using ImStrv. Both branches provide a usable C wrapping of imgui functions and in the case they use ImStrv provide a const char* counterpart.

ImStrv label = NULL is not a cimgui issue becuse cimgui does not provide default values, they are provided in other languaje bindings using cimgui as LuaJIT-ImGui or whatever.

[ocornut]

Not completely sure but I think that the only difference between master or docking_inter branches of cimgui and string_view2 is the naming strategy of functions using ImStrv. Both branches provide a usable C wrapping of imgui functions and in the case they use ImStrv provide a const char* counterpart.

I just want to make sure if we switch to ImStrv tomorrow people using other languages will be able to update after minor cimgui changes.

ImStrv label = NULL is not a cimgui issue becuse cimgui does not provide default values, they are provided in other languaje bindings using cimgui as LuaJIT-ImGui or whatever.

Does it means that it's only stored in the metadata field? In this case could the metadata automatically replace = NULL with = ImStrv() so C++ version of imgui.h can use = NULL ?

[sonoro1234]

I just want to make sure if we switch to ImStrv tomorrow people using other languages will be able to update after minor cimgui changes.

I can only be sure about LuaJIT-ImGui:

All modules will run without troubles.
As for cimguizmo_quat

As for other languaje bindings I guess they will be able to succeed with small changes (but really dont know)

Does it means that it's only stored in the metadata field? In this case could the metadata automatically replace = NULL with = ImStrv() so C++ version of imgui.h can use = NULL ?

I could be useful for LuaJIT-ImGui but then it would need to be ig.ImStrv(). I dont know for others. Perhaps it is easier to handle it in the binding generation side: When an argument of type ImStrv has a default value of NULL do whatever is equivalent in your language to label = label or ig.ImStrv()

[sonoro1234]

@mellinoe @oprypin @lmariscal @Gekkio @Dominaezzz @ThisDrunkDane @Gnimuc @thomcc @ctreffs @vaiorabbit
and any other interested:

cimgui branch string_view2 just regenerated.
Any thoughts?
Will you be using Strv or const char * version?

[ctreffs]

Swift provides String initializers for C-Strings that are null-terminated code sequences. const char * will be translated to UnsafePointer<CChar> for the Swift wrapper implementation to be used. As long as const char * continue to provide zero-terminated Strings I favour this over the Strv implementation.

[thomcc]

For Rust, creating a NUL-terminated string from a literal is inconvenient, and either requires a macro (what we have now) or allocation, so from that branch we badly want the Strv option, or something equivalent that can express non-nul-terminated strings.

Beyond that, I have no strong opinions on the shape of the API, since we'll have to wrap it anyway, as calling into C is awkward and unsafe (which is disliked).

I have no opinion on anything like naming conventions as I'm not going to use these from C.

I'd be okay needing to pass a flags into cimgui's generator or whatever in order to generate the files with string views rather than const char*.

I just want to make sure if we switch to ImStrv tomorrow people using other languages will be able to update after minor cimgui changes.

This would need a lot of internal changes in imgui-rs, but nothing we wouldn't find desirable. As it is we have to do a lot of work to work around the need to use nul-terminated strings.

For users of imgui-rs, it should be fine and not break much, although we'd probably deprecate some things that would be redundant (and only exist in the Rust API).

---

P.S. If the outcome of this ends up being that only the C++ API can use string views (and cimgui just exposes const char*), the Rust bindings could probably make do, as calling C++ from Rust is doable, but takes a lot of sweat, elbow grease, and a moderate amount of pain. (Somewhat obviously, I'd find this to be undesirable, but I'd understand)

[ocornut]

Thank you @thomcc for your feedback, much appreciated!

[lmariscal]

Since Nim was designed with C interop in mind it already contains a compatible type CString. Nim bindings would continue to use the const char *, since Strv would only add an extra step to generate a string.

I agree with @thomcc in that it would be nice to be able to select between Strv and const char * via a generation or a compilation time flag (maybe enabled by default for the public files), so that languages that require this extra step can have it but also don't force it into every build.

[ThisDevDane]

We do have a cstring type in Odin as well, but if we do the Strv I shouldn't have to do copies to add null termination so that's good. I would actually prefer the double char * option over having to assemble the struct every time but that's fine.

I'm also in the camp that a build option is fine.

[Dominaezzz]

Kotlin bindings require making a copy of the string to pass to C anyway, so either way is fine.
This won't break users of kotlin-imgui.

[sonoro1234]

it would be nice to be able to select between Strv and const char * via a generation or a compilation time flag

I would not do it on a first stage. Now both versions are generated (ImStrv and const char *) and it should do no harm. It will be a major transition, when it is finished we could add the flag based in the bind-makers experiencies up till then.

[thomcc]

I agree with @thomcc in that it would be nice to be able to select between Strv and const char * via a generation or a compilation time flag

Ah, to be clear, I would only use the Strv. I only meant if there are too many problems with switching to it in cimgui by default, I don't mind passing it as a flag.

I don't mind having both versions either.

I would actually prefer the double char * option over having to assemble the struct every time but that's fine.

Yes, same here actually, but it isn't a big deal.

[oprypin]

oprypin/crystal-imgui@strv
Crystal is happily migrated to ImStrv with no functionality loss or breakage*.

Crystal has a text String type, a Bytestring type, and a Nil type, all of which are easily convertible to the struct on the fly. I just accept any of these three -- pretty much the same as before, but indeed saving a strlen call.
oprypin/crystal-imgui@strv#diff-da70292aabc6a4bb66a56612d6d1fca6d303792cb7df4a758c012e69769ae20c

The naming scheme is fine, keeping both seems fine to me, but I'll switch to ImStrv fully.

Something I noticed is that there are never any ImStrv return values, only inputs as args. Probably intentional, but just pointing that out. (I checked with Ctrl+F IMGUI_API ImStrv).

---

Also note that currently cimgui on that branch 2688f45 seems to be failing to compile on whatever compilers these were on Mac and Windows - call to 'ImHashStr' is ambiguous
https://github.com/oprypin/crystal-imgui/actions/runs/724235772

---

And I am actually running into a crash:

cimgui/imgui/imgui_widgets.cpp:2318: bool ImGui::DragScalar(ImStrv, ImGuiDataType, void*, float, const void*, const void*, ImStrv, ImGuiSliderFlags): Assertion `format_p.End - format_p.Begin < ((int)(sizeof(format_0) / sizeof(*(format_0))))' failed.

When doing this call (i.e. when opening Widgets → Basic):

https://github.com/oprypin/crystal-imgui/blob/7381050dec5949e5e7bc4eccab82f8af5b31699c/src/demo.cr#L496
(C++ equivalent https://github.com/ocornut/imgui/blob/c283a1da5bab8f8492e0c5b4ac9f3bd71adc05c2/imgui_demo.cpp#L658)
→
https://github.com/oprypin/crystal-imgui/blob/6088cd15a3ceb1943fc20aa054b3e3605946b5b6/src/obj.cr#L1099-L1100
→
https://github.com/cimgui/cimgui/blob/2688f45ff99b22532a932b17fee414f45c4bbad8/cimgui.cpp#L835-L837
→
https://github.com/ocornut/imgui/blob/b69e65f1cdd6d4bc3e9f36c47ce9a6de34f2c621/imgui_widgets.cpp#L2318

The passed args are:

label # => ImGui::ImStrv(@begin=Pointer(UInt8)@0x55ce4af77a1c, @end=Pointer(UInt8)@0x55ce4af77a24)
v # => Pointer(Int32)@0x7fff276cb9c4
v_speed # => 1.0
v_min # => 0
v_max # => 0
format # => ImGui::ImStrv(@begin=Pointer(UInt8)@0x55ce4af71724, @end=Pointer(UInt8)@0x55ce4af71726)
flags # => 0

And I was going to attribute this crash purely to the C++ ImGui code
but compiling the C++ demo works fine, so I guess I'll have to just keep looking for a mistake on my side

Command that works fine:

c++ -I. -Ibackends examples/example_glfw_opengl3/main.cpp backends/imgui_impl_glfw.cpp backends/imgui_impl_opengl3.cpp imgui_demo.cpp imgui_widgets.cpp imgui.cpp imgui_draw.cpp imgui_tables.cpp -lglfw -lGLEW -lGL

[sonoro1234]

Also note that currently cimgui on that branch 2688f45 seems to be failing to compile

@oprypin
It is not failing to compile for me on mingw-w64 and nmake.
Could you try building from cimgui/CMakeLists.txt?

[sonoro1234]

Just pushed correction for compiling test on Debug mode and correct default values for const char* version on definitions.json

[ocornut]

For general "dear imgui side" feedback (not specific to cimgui) on the ImStrv best to post in ocornut/imgui#3038

Something I noticed is that there are never any ImStrv return values, only inputs as args. Probably intentional, but just pointing that out.

If I am not mistaken there are only 6 functions in the public API, they all return imgui-owned-or-interned strings which are zero-terminated. If we returned ImStrv it would require users to assume strings may not be zero-terminated, which is a non-trivial burden for users for some languages/librairies:

ImGui

const char*   GetVersion();                               // get the compiled version string e.g. "1.80 WIP" (essentially the value for IMGUI_VERSION from the compiled version of imgui.cpp)
const char*   TableGetColumnName(int column_n = -1);      // return "" if column didn't have a name declared by TableSetupColumn(). Pass -1 to use current column.
const char*   GetStyleColorName(ImGuiCol idx);                                    // get a string corresponding to the enum value (for display, saving, etc.).
const char*   GetClipboardText();
const char*   SaveIniSettingsToMemory(size_t* out_ini_size = NULL);               // return a zero-terminated string with the .ini data which you can save by your own mean. call when io.WantSaveIniSettings is set, then save data by your own mean and clear io.WantSaveIniSettings.

And this one specifically return 1 pointer:

const char*       ImFont::CalcWordWrapPositionA(float scale, ImStrv text, float wrap_width) const;

And I am actually running into a crash:
cimgui/imgui/imgui_widgets.cpp:2318: bool ImGui::DragScalar(ImStrv, ImGuiDataType, void*, float, const void*, const void*, ImStrv, ImGuiSliderFlags): Assertion `format_p.End - format_p.Begin < ((int)(sizeof(format_0) / sizeof(*(format_0))))' failed.

Can't seem to understand how that could happen considering the difference is 2 here: (as default value is "%d")

format # => ImGui::ImStrv(@begin=Pointer(UInt8)@0x55ce4af71724, @EnD=Pointer(UInt8)@0x55ce4af71726)

[oprypin]

Also note that currently cimgui on that branch 2688f45 seems to be failing to compile on whatever compilers these were on Mac and Windows - call to 'ImHashStr' is ambiguous
https://github.com/oprypin/crystal-imgui/actions/runs/724235772

@sonoro1234

It is not failing to compile for me on mingw-w64 and nmake.
Could you try building from cimgui/CMakeLists.txt?

I sent a link to GitHub Actions, you can see everything how I'm building there.
https://github.com/oprypin/crystal-imgui/runs/2282958426

cmake -DCMAKE_CXX_FLAGS='-DIMGUI_USE_WCHAR32' .
cmake --build . -j4

But it doesn't matter how I build;
On Linux locally, without any compilation flags, actually I'm getting the same complaint, just as a warning

$ g++ --version        
g++ (GCC) 10.2.0
$ g++ -I. -c cimgui.cpp  
cimgui.cpp: In function ‘ImGuiID igImHashStr_StrU32(const char*, ImU32)’:
cimgui.cpp:3016:30: warning: ISO C++ says that these are ambiguous, even though the worst conversion for the first is better than the worst conversion for the second:
 3016 |     return ImHashStr(str,seed);
      |                              ^
In file included from cimgui.cpp:9:
./imgui/imgui_internal.h:268:25: note: candidate 1: ‘ImGuiID ImHashStr(const char*, size_t, ImU32)’
  268 | static inline ImGuiID   ImHashStr(const char* data, size_t data_size = 0, ImU32 seed = 0) { return ImHashStr(ImStrv(data, data_size ? data + data_size : NULL), seed); }
      |                         ^~~~~~~~~
./imgui/imgui_internal.h:267:25: note: candidate 2: ‘ImGuiID ImHashStr(ImStrv, ImU32)’
  267 | IMGUI_API ImGuiID       ImHashStr(ImStrv str, ImU32 seed = 0);
      |                         ^~~~~~~~~

---

Can't seem to understand how that could happen considering the difference is 2 here: (as default value is "%d")

Welp, I found it. Crystal just has a bug in passing multiple structs to a C function.
So nothing relevant to this issue, I guess. Though that will impede my enthusiasm about this migration :D But I can still use the char* versions fine.

I will point out, the way that arguments need to be passed (the binary interface on the stack) quickly becomes complicated, and wildly different across platforms, when structs are involved. Maybe some other language will have trouble with it too.
So this is a super tiny argument towards unwrapping this struct into two direct args in cimgui. But don't mind me.

Details about where the data got garbled by Crystal

So those values I had mentioned were printed in Crystal before passing them to the call.

ImStrv.new(label)  # => ImGui::ImStrv(@begin=Pointer(UInt8)@0x55940f341aac, @end=Pointer(UInt8)@0x55940f341ab4)
v                  # => Pointer(Int32)@0x7ffd378b0a54
v_speed            # => 1.0
v_min              # => 0
v_max              # => 0
ImStrv.new(format) # => ImGui::ImStrv(@begin=Pointer(UInt8)@0x55940f33b7b4, @end=Pointer(UInt8)@0x55940f33b7b6)
flags              # => None

Then directly inside the call on the C side I already get garbled data.

CIMGUI_API bool igDragInt_Strv(ImStrv label,int* v,float v_speed,int v_min,int v_max,ImStrv format,ImGuiSliderFlags flags) {
    printf("%p %p, %p, %f, %d, %d, %p %p, %d\n", label.Begin, label.End, v, v_speed, v_min, v_max, format.Begin, format.End, flags);

0x55940f341aac 0x55940f341ab4, 0x7ffd378b0a54, 1.000000, 0, 0, 0x55940f33b7b6 0x78d0990700000000, 255047604

[lmariscal]

@oprypin @ocornut @sonoro1234 It seems to be an issue that only happens with the clang compiler. I'm having the same issues using:

clang version 11.1.0
Target: x86_64-pc-linux-gnu
Thread model: posix

The error that is shown using cmake without any aditional flags or definitions:

[2/7] Building CXX object CMakeFiles/cimgui.dir/cimgui.cpp.o
FAILED: CMakeFiles/cimgui.dir/cimgui.cpp.o 
/usr/bin/clang++ -DIMGUI_DISABLE_OBSOLETE_FUNCTIONS=1 -DIMGUI_IMPL_API="extern	\"C\"	" -Dcimgui_EXPORTS -I../ -I../imgui -fPIC -MD -MT CMakeFiles/cimgui.dir/cimgui.cpp.o -MF CMakeFiles/cimgui.dir/cimgui.cpp.o.d -o CMakeFiles/cimgui.dir/cimgui.cpp.o -c ../cimgui.cpp
../cimgui.cpp:3016:12: error: call to 'ImHashStr' is ambiguous
    return ImHashStr(str,seed);
           ^~~~~~~~~
.././imgui/imgui_internal.h:267:25: note: candidate function
IMGUI_API ImGuiID       ImHashStr(ImStrv str, ImU32 seed = 0);
                        ^
.././imgui/imgui_internal.h:268:25: note: candidate function
static inline ImGuiID   ImHashStr(const char* data, size_t data_size = 0, ImU32 seed = 0) { return ImHashStr(ImStrv(data, data_size ? data + data_size : NULL), seed); }
                        ^
1 error generated.
[6/7] Building CXX object CMakeFiles/cimgui.dir/imgui/imgui.cpp.o
../imgui/imgui.cpp:5691:116: warning: implicit conversion of NULL constant to 'bool' [-Wnull-conversion]
    const float marker_size_x = (flags & ImGuiWindowFlags_UnsavedDocument) ? CalcTextSize(UNSAVED_DOCUMENT_MARKER, NULL, false).x : 0.0f;
                                                                             ~~~~~~~~~~~~                          ^~~~
                                                                                                                   false

Using GCC 10.2.0 the same line is shown as a warning instead of an error:

[4/7] Building CXX object CMakeFiles/cimgui.dir/cimgui.cpp.o
../cimgui.cpp: In function ‘ImGuiID igImHashStr_StrU32(const char*, ImU32)’:
../cimgui.cpp:3016:30: warning: ISO C++ says that these are ambiguous, even though the worst conversion for the first is better than the worst conversion for the second:
 3016 |     return ImHashStr(str,seed);
      |                              ^
In file included from ../cimgui.cpp:9:
.././imgui/imgui_internal.h:268:25: note: candidate 1: ‘ImGuiID ImHashStr(const char*, size_t, ImU32)’
  268 | static inline ImGuiID   ImHashStr(const char* data, size_t data_size = 0, ImU32 seed = 0) { return ImHashStr(ImStrv(data, data_size ? data + data_size : NULL), seed); }
      |                         ^~~~~~~~~
.././imgui/imgui_internal.h:267:25: note: candidate 2: ‘ImGuiID ImHashStr(ImStrv, ImU32)’
  267 | IMGUI_API ImGuiID       ImHashStr(ImStrv str, ImU32 seed = 0);
      |                         ^~~~~~~~~
[7/7] Linking CXX shared library cimgui.so

I am able to compile the same files with:

clang++ -c imgui.cpp imgui_demo.cpp imgui_draw.cpp imgui_tables.cpp imgui_widgets.cpp

And even:

clang++ -c -DIMGUI_DISABLE_OBSOLETE_FUNCTIONS=1 -DIMGUI_IMPL_API="extern \"C\" " -Dcimgui_EXPORTS -I../ -I../imgui -fPIC -MD -MT imgui.cpp imgui_demo.cpp imgui_draw.cpp imgui_tables.cpp imgui_widgets.cpp

[sonoro1234]

ImHashStr issue is happening only with 64bit compilation: mingw-w64 warning and nmake error.

The only solution I can think is wrapping argument in ImStrv as

CIMGUI_API ImGuiID igImHashStr_StrU32(const char* str,ImU32 seed)
{
    return ImHashStr(ImStrv(str),seed);
}

I will wrap into ImStrv all const char* versions.

[sonoro1234]

Just pushed change. Is that ok?

[sonoro1234]

Welp, I found it. Crystal just has a bug in passing multiple structs to a C function.

@oprypin
So, are you getting errors on igSetNextWindowPos which passes two ImVec2 also?

[oprypin]

@sonoro1234 Please don't let that bug affect decisions here :)
If you're curious: I believe that the condition for the error to happen is that the ImStrv parameter is in 5th position or later. So SetNextWindowPos in particular is not affected.
From this heuristic I was able to correctly guess that PlotLines also crashes.

---

And thanks for the change to resolve that compiler warning/error. It works.

[sonoro1234]

This issue is too long so I open another issue to continue: #186