raysan5 / raygui

A simple and easy-to-use immediate-mode gui library
zlib License
3.47k stars 298 forks source link

Memory leaks in style functions #295

Closed Tinchetsu closed 1 year ago

Tinchetsu commented 1 year ago

There are memory leaks in the style functions. This could be a problem if we make a dynamic theme picker or something like that.

The functions have code like this (this is for the dark style, didn't check them all):

font.recs = (Rectangle *)malloc(font.glyphCount*sizeof(Rectangle));
memcpy(font.recs, darkFontRecs, font.glyphCount*sizeof(Rectangle));

font.glyphs = (GlyphInfo *)malloc(font.glyphCount*sizeof(GlyphInfo));
memcpy(font.glyphs, darkFontChars, font.glyphCount*sizeof(GlyphInfo));

The problem with that, is that the allocated memory can never be freed. Possible solution could be it just assign the values, since they are not used outside of this function(?)

This is the change that I did to the dark style:

font.recs = darkFontRecs; 
font.glyphs = darkFontChars;
Tinchetsu commented 1 year ago

As I was playing with this, I found out that I could get the font with GuiGetFont(), and manually free all these pointers, a bit ugly, but with that I can avoid the leaks. Maybe we can have a GuiUnloadStyleXXX for this?

raysan5 commented 1 year ago

@Tinchetsu Wow! Thanks for reporting! I don't know how I missed it!

Direct assignation could be a solution in the case styles are provided from a code file (.h) but in then that data can not be loaded because it belongs to const data memory segment.

Probably providing a GuiUnloadStyle() function is the best approach... actually, only the Font requires that unloading because the rest of the style data directly uses the global state of raygui... actually, Font should be something like GuiFont, to be independent of raylib...

Anyway, I'll look for a solution to this issue as soon as possible.

raysan5 commented 1 year ago

@Tinchetsu One possible temporal solution with current implementation is just calling UnloadFont(GuiGetFont()); before loading any new style. Far from perfect but considering Font should/could be managed externally by the user, it makes sense.

Probably the best approach is providing some abstraction layer for GuiFont over raylib Font but it will increase the complexity of the code...

Tinchetsu commented 1 year ago

Thanks, I can go with that, not a problem. Thank you @raysan5 for your great work with raylib and all the derivative stuff. Love it.

funlw65 commented 1 year ago

... but considering Font should/could be managed externally by the user, it makes sense.

I end up adding custom elements like buttons with gradients (for now, hardcoded) and two fonts per button, regular at displaying and hovering, and bold at clicking (some speedy buttons have even blinking LEDs)... For a final app, I can offer maximum two styles, dark and light or only dark... but I prefer having the colors internally defined, not allowing the final user to play with them as the look of the GUI becomes technical/industrial (or imagine something like a panel of an audio mixer) . Yes, raygui functionality underneath remains untouched but look changes... So, expect that the programmer might manage all the fonts and more...

Here, at the end of the article are two screenshots of the GUI (WIP): https://stm32vpc.blogspot.com/2023/02/the-system-has-been-decided.html

Imagine that the user starts playing with the style resource file, how much harm can do to such a graphical interface...

But this depends highly on the type of the application... what I want to say is that we end up managing everything regarding resources...

raysan5 commented 1 year ago

@funlw65 Wow! That application UI looks really beautiful! Congratulations!

By the way, I'm reading you are using raygui 3.2 mod, did you considered updating to raygui 4.0-dev? Functions are more consistent and it adds room for many improvements.

funlw65 commented 1 year ago

Thank you, I will take a look at 4.0-dev!

raysan5 commented 1 year ago

Reviewed with a previous solution that I don't know why I removed.

Now, before loading any new style, GuiLoadStyleDefault() should be called, it frees any previous font texture, recs and glyphs.

So, the style.h generated should keep allocating memory for recs and glyphs instead of assigning directly the global data.