Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

AddFontFromMemoryTTF invalidate font->ConfigData pointer of previous fonts #6825

Closed
ElectroidDes opened this issue Sep 15, 2023 · 16 comments
Closed

Comments

@ElectroidDes
Copy link

I can’t figure it out, but after calling AddFontFromMemoryTTF(), the pointer to the previously loaded ImFont font is reset to zero and setting ImFontConfig_.FontDataOwnedByAtlas = false - has no effect on this.

@CoolFox29281819
Copy link

Can you provide a code sample?

You can also just call the method directly instead of storing it in a pointer unless you have separate fonts. I would also make sure that your character array is in a separate header file and that you are only including it once.

If you are using a pointer make sure you follow the syntax:

ImFont* fontPointer = io.Fonts->AddFontFrommemoryTTF(arr, sizeof(arr), 3.f);
(I think these are the correct parameters im not sure)

@ElectroidDes
Copy link
Author

This happens when I load two fonts into a combo font at once and then load those two fonts into another combo font again - at which point Imgui simply deletes the loaded fonts.
This is just some kind of cruelty.

@ocornut
Copy link
Owner

ocornut commented Sep 15, 2023

You are making a mistake and the only way to get help is to share an actual verifiable example showing the issue.
Please stop opening issues that are ignoring our Issue Opening Guidelines:
https://github.com/ocornut/imgui/blob/master/docs/CONTRIBUTING.md

@CoolFox29281819
Copy link

This happens when I load two fonts into a combo font at once and then load those two fonts into another combo font again - at which point Imgui simply deletes the loaded fonts. This is just some kind of cruelty.

Please show an example with a video and some source code.

Thanks :)

@ElectroidDes
Copy link
Author

ElectroidDes commented Sep 15, 2023

You are making a mistake and the only way to get help is to share an actual verifiable example showing the issue. Please stop opening issues that are ignoring our Issue Opening Guidelines: https://github.com/ocornut/imgui/blob/master/docs/CONTRIBUTING.md

My apologies.

This is pure Imgui code (no backend) that is guaranteed to cause an error, or more precisely AddFontFromMemoryTTF() just takes and removes the previously loaded font:
Which exact call to the AddFontFromMemoryTTF function deletes the font was indicated in the comment to the code against the function.:


int main()
{
    //--------------------------------------------------------
    ImGuiContext* ImGuiContext_p = ImGui::CreateContext();  
    ImGui::SetCurrentContext(ImGuiContext_p);              
    //--------------------------------------------------------

    //--------------------------------------------------------
    ImGuiIO& ImGuiIO_ref = ImGui::GetIO();                      
    //--------------------------------------------------------



    ImFontConfig ImFontConfig_defolt;
    ImFontConfig_defolt.SizePixels = 21;
    ImFont* ImFont_defolt_p = (ImGuiIO_ref).Fonts->AddFontDefault(&ImFontConfig_defolt);     



    //--------------------------------------------------------------------------------------------------
    ImFontConfig ImFontConfig_;
    ImFontConfig_.FontDataOwnedByAtlas = false;

    std::vector<ImWchar> vec_diap_1D = { 20000, 20500, 0 };

    ImFont* ImFont_p_1 = (ImGuiIO_ref).Fonts->AddFontFromFileTTF(u8"C:\\china东.TTF", 21, &ImFontConfig_, &vec_diap_1D[0]);   
    bool res = (ImGuiIO_ref).Fonts->Build();
    //--------------------------------------------------------------------------------------------------


    //--------------------------------------------------------------------------------------------------
    ImFontConfig ImFontConfig_2;
    ImFontConfig_2.FontDataOwnedByAtlas = false;
    std::vector<ImWchar> vec_diap_1D_2 = { 800, 1000, 0 };
    ImFont* ImFont_p_2 = (ImGuiIO_ref).Fonts->AddFontFromFileTTF(u8"C:\\Greek.TTF", 21, &ImFontConfig_2, &vec_diap_1D_2[0]);   //Можно использовать или прямую функцию Imgui или С++17 для чтения бинарного TTF из файла, как ниже, а потом функцию Imgui для чтения из памяти.
    res = (ImGuiIO_ref).Fonts->Build();


    ImFontConfig ImFontConfig_3;
    ImFontConfig_3.FontDataOwnedByAtlas = false;
    std::vector<ImWchar> vec_diap_1D_3 = { 1, 201, 0 };
    ImFont* ImFont_p_3 = (ImGuiIO_ref).Fonts->AddFontFromFileTTF(u8"C:\\beer.TTF", 21, &ImFontConfig_3, &vec_diap_1D_3[0]);   //Можно использовать или прямую функцию Imgui или С++17 для чтения бинарного TTF из файла, как ниже, а потом функцию Imgui для чтения из памяти.
    res = (ImGuiIO_ref).Fonts->Build();




    ImFontConfig ImFontConfig_4;
    ImFontConfig_4.FontDataOwnedByAtlas = false;
    ImFontConfig_4.MergeMode = false;
    std::vector<ImWchar> vec_diap_1D_4 = { 800, 1000, 0 };
    ImFont* ImFont_p_comb = (ImGuiIO_ref).Fonts->AddFontFromMemoryTTF(ImFont_p_2->ConfigData->FontData, ImFont_p_2->ConfigData->FontDataSize, 21, &ImFontConfig_4, &vec_diap_1D_4[0]);
    
    ImFontConfig ImFontConfig_5;
    ImFontConfig_5.MergeMode = true;
    ImFontConfig_5.MergeMode = false;
    std::vector<ImWchar> vec_diap_1D_5 = { 1, 201, 0 };
    ImFont_p_comb = (ImGuiIO_ref).Fonts->AddFontFromMemoryTTF(ImFont_p_3->ConfigData->FontData, ImFont_p_3->ConfigData->FontDataSize, 21, &ImFontConfig_5, &vec_diap_1D_5[0]);
    res = (ImGuiIO_ref).Fonts->Build();
   //--------------------------------------------------------------------------------------------------



    //-----------------------------------------------------------------------------
    ImFontConfig ImFontConfig_22;
    ImFontConfig_22.FontDataOwnedByAtlas = false;
    std::vector<ImWchar> vec_diap_1D_22 = { 800, 1000, 0 };
    ImFont* ImFont_p_22 = (ImGuiIO_ref).Fonts->AddFontFromFileTTF(u8"C:\\Greek.TTF", 21, &ImFontConfig_22, &vec_diap_1D_22[0]);   


    ImFontConfig ImFontConfig_33;
    ImFontConfig_3.FontDataOwnedByAtlas = false;
    std::vector<ImWchar> vec_diap_1D_33 = { 1, 201, 0 };
    ImFont* ImFont_p_33 = (ImGuiIO_ref).Fonts->AddFontFromFileTTF(u8"C:\\beer.TTF", 21, &ImFontConfig_33, &vec_diap_1D_33[0]);  
    res = (ImGuiIO_ref).Fonts->Build();




    ImFontConfig ImFontConfig_44;
    ImFontConfig_44.FontDataOwnedByAtlas = false;
    ImFontConfig_44.MergeMode = false;
    std::vector<ImWchar> vec_diap_1D_44 = { 800, 1000, 0 };
    ImFont* ImFont_p_comb4 = (ImGuiIO_ref).Fonts->AddFontFromMemoryTTF(ImFont_p_22->ConfigData->FontData, ImFont_p_22->ConfigData->FontDataSize, 21, &ImFontConfig_44, &vec_diap_1D_44[0]);  //HERE!!!! AddFontFromMemoryTTF resets the pointer ImFont_p_22->ConfigData->FontData.


    ImFontConfig ImFontConfig_55;
    ImFontConfig_55.MergeMode = true;
    ImFontConfig_55.MergeMode = false;
    std::vector<ImWchar> vec_diap_1D_55 = { 1, 201, 0 };
    ImFont_p_comb4 = (ImGuiIO_ref).Fonts->AddFontFromMemoryTTF(ImFont_p_33->ConfigData->FontData, ImFont_p_33->ConfigData->FontDataSize, 21, &ImFontConfig_55, &vec_diap_1D_55[0]);
    res = (ImGuiIO_ref).Fonts->Build();
    //-----------------------------------------------------------------------------


}

This Imgui code causes an error:

Assertion failed: font_cfg->FontData != 0 && font_cfg->FontDataSize > 0, file imgui_draw.cpp, line 2128
Comment in the code.

@ElectroidDes
Copy link
Author

Moreover, if at the beginning of the code I delete the line for loading the default Imgui Font, then the code completes successfully. I can't figure out what's wrong :(

@ocornut
Copy link
Owner

ocornut commented Sep 15, 2023

You are passing a value of 0 for font_data_size and it is asserting because of that.

You shouldn’t need to call Build() more than once at the end.

@ElectroidDes
Copy link
Author

You are passing a value of 0 for font_data_size and it is asserting because of that.

You shouldn’t need to call Build() more than once at the end.

But I don’t have a variable in the code called font_data_size to which I pass 0.

I call Build() exactly once after adding the font.
Adding fonts in code looks like I'm adding them all at once, but loading fonts can happen at different times while the application is running, so I still have to call Build() every time - right?

@ElectroidDes
Copy link
Author

You are passing a value of 0 for font_data_size and it is asserting because of that.

I'm talking about the Imgui function AddFontFromMemoryTTF - deletes the downloaded font. It just takes it and deletes it and because of this the application crashes.

@ElectroidDes
Copy link
Author

This happens when I load two fonts into a combo font at once and then load those two fonts into another combo font again - at which point Imgui simply deletes the loaded fonts. This is just some kind of cruelty.

Please show an example with a video and some source code.

Thanks :)

Showed it. And ? :)

@ocornut
Copy link
Owner

ocornut commented Sep 16, 2023

But I don’t have a variable in the code called font_data_size to which I pass 0.

You are regularly passing values that you have never set, eg when you are passing ImFont_p_22->ConfigData->FontDataSize to the function. This is likely to be 0 you never set that function.

Adding fonts in code looks like I'm adding them all at once, but loading fonts can happen at different times while the application is running, so I still have to call Build() every time - right?

This becomes a whole different problem if you load during application run. It needs to always be done before NewFrame(). But there’s no need to call Build() more than once in the code you posted which is the code we are discussing now.

I'm talking about the Imgui function AddFontFromMemoryTTF - deletes the downloaded font.

Are you using a debugger to confirm this or are you just making wild guesses? You should always be using a debugger to confirm your data and code behavior.

@ElectroidDes
Copy link
Author

ElectroidDes commented Sep 16, 2023

You are regularly passing values that you have never set, eg when you are passing ImFont_p_22->ConfigData->FontDataSize to the function. This is likely to be 0 you never set that function.

For some reason you don't want to hear me.

Firstly, I wrote that if I remove the loading of the default Imga font at the very beginning, then ALL the rest of the code works, that is, ImFont_p_22->ConfigData->FontDataSize is NOT equal to zero.
Secondly: WHY IN YOUR opinion ImFont_p_22->ConfigData->FontDataSize MUST be zero??
If I loaded the Greek.TTF font into ImFont_p_22?? This is, to put it mildly, a very strange comment on your part.

Are you using a debugger to confirm this or are you just making wild guesses? You should always be using a debugger to confirm your data and code behavior.

I’m not a complete fool, of course I used debugging, I wrote from the BEGINNING and even in the title of the topic that the Imgui AddFontFromMemoryTTF function ITSELF deletes previously loaded fonts. I even indicated in the code with a comment after which particular call to the AddFontFromMemoryTTF function - this deletion occurs.

@ocornut
Copy link
Owner

ocornut commented Sep 16, 2023

For some reason you don't want to hear me.

You would be surprised how many well-intended people are accidentally making false claims, then we have to work and dig into their mistake for them. I'm not saying you are making a false claim, but in order to get the free help you are receiving here you need to be putting every effort to prove your claim.

If you said "I used a debugger and noticed that between point XX and point XX this value changes in a way that seems incorrect" that would immediately convey what you tried and understood better. Otherwise I have to guess.

Moreoever, I know for a fact that we don't "delete" fonts randomly.

And, right now we cannot easy test your code as it depends on files/ranges we don't have, so you are not making it easier for us.

Secondly: WHY IN YOUR opinion ImFont_p_22->ConfigData->FontDataSize MUST be zero??

That was my honest intuition reading the code because you are never setting this value, and well, this is what one of the two possible things the assert is triggering for.
it looks like you are accessing some internal members on the assumption that internal loading code will set them for you.


So I looked into it and here's the issue:

  • When building we set the font->ConfigData pointer to point to somewhere in our internal buffer in ImFontAtlas.
  • If you add fonts after building, those pointers can become dangling (point to old memory location) until building again.

Technically none of the members in ImFont are for public consumption but I do realize this isn't explicitly written down.
You are accessing through those pointers after calling AddFontXXX and before BuildXXX so this is faulty and causing your issue.

I'm going to make a change now to prevent or detect that better.

@ocornut ocornut changed the title AddFontFromMemoryTTF - delete Font AddFontFromMemoryTTF invalidate font->ConfigData pointer of previous fonts Sep 16, 2023
ocornut added a commit that referenced this issue Sep 16, 2023
…s ConfigData pointers prior to building again. (#6825)
@ocornut
Copy link
Owner

ocornut commented Sep 16, 2023

I have pushed a change 6addf28 to make this undefined behavior (access ImFont::ConfigData between Add and Build) better defined.

Note that two issues remains in your code:

  • ImFontConfig_5 and ImFontConfig_55 don't have FontDataOwnedByAtlas = false;
  • The glyph ranges lifetime don't extend past the function, which currently requires that you NEED to call Build() once in the function but you cannot then call Build() once you have left the function even once.

I do realize that the lifetime and ownership issues are confusing in the font loading API.
I've been hesitant to redesign/refactor them yet because I don't have all elements I need for a full "V2" font api yet.

@ElectroidDes
Copy link
Author

Now I understand, thank you very much.

@ocornut
Copy link
Owner

ocornut commented Sep 18, 2023

Closing as answered + some changes to facilitate this.

Hope to get back to the ownership mess soon.

@ocornut ocornut closed this as completed Sep 18, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants