-
Notifications
You must be signed in to change notification settings - Fork 37
Added addTextExtended and addTextExtendedUnformatted for additional AddText Functionalities #55
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
Conversation
I am about to test these functions, so I am going to commit those for a backup.
The PlotLines() and PlotHistogram() are now tested and they can produce the exact same result from the web imgui demo.
The code is done, and testing is in sight.
Wait, is it normal that my commits in the previous pull request also appeared in this pull request? |
It's because I squash-merged your previous PR, so your branch has diverged from upstream main. You can simply rebase your branch onto the upstream main branch to get it synced up properly (and then force push), but it's not a big deal in this case. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR adds extended text drawing functionality to the zgui DrawList API, enabling custom font and sizing options. The changes introduce two new functions: addTextExtended
(with formatting) and addTextExtendedUnformatted
(without formatting) that support additional text rendering parameters.
- Added
AddTextArgs
struct to configure font, font size, wrap width, and clipping rectangle - Implemented
addTextExtended
andaddTextExtendedUnformatted
functions in Zig - Added corresponding C++ binding function
zguiDrawList_AddTextExtended
Reviewed Changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
File | Description |
---|---|
src/zgui.cpp | Added C++ binding function for extended text drawing with additional parameters |
src/gui.zig | Added Zig wrapper functions and struct for extended text drawing functionality |
Comments suppressed due to low confidence (1)
src/gui.zig:4554
- [nitpick] The struct name 'AddTextArgs' should follow consistent naming conventions. Consider renaming to 'TextDrawArgs' or 'DrawTextArgs' to better align with the context of drawing operations.
const AddTextArgs = struct {
src/zgui.cpp
Outdated
float wrap_width, | ||
const float cpu_fine_clip_rect[][4]) | ||
{ | ||
draw_list->AddText(font, font_size, {pos[0], pos[1]}, col, text_begin, text_end, wrap_width, (const ImVec4 *)&cpu_fine_clip_rect[0][0]); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Potential null pointer dereference. When 'cpu_fine_clip_rect' is null, accessing '&cpu_fine_clip_rect[0][0]' will cause undefined behavior. The cast should handle the null case properly.
draw_list->AddText(font, font_size, {pos[0], pos[1]}, col, text_begin, text_end, wrap_width, (const ImVec4 *)&cpu_fine_clip_rect[0][0]); | |
const ImVec4* clip_rect = (cpu_fine_clip_rect != nullptr) ? (const ImVec4 *)&cpu_fine_clip_rect[0][0] : nullptr; | |
draw_list->AddText(font, font_size, {pos[0], pos[1]}, col, text_begin, text_end, wrap_width, clip_rect); |
Copilot uses AI. Check for mistakes.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice catch bot
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Alright, will change the name, and try to apply that suggestion for clip_rect to see if that works.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the naming is good as is?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh, I thought the copilot suggests to rename the struct. If the name change isn't needed, then I am going to commit the suggested fix to the null pointer dereference only.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Here you go; the suggestion is applied.
As suggested by the code review, I have replaced my direct array access with a null pointer check.
All Good? After fixed the potential null pointer reference, anything else I need to do? |
As proposed in my issues of missing add text functions variant, I have enabled the function with the name addTextExtendedUnformatted and addTextExtended to handle text drawing with custom font and sizes:
As you can see, this update could load and render different fonts, along with changing the clipped region and wrap width defined in an additional struct AddTextArgs.
Let's see what would you think about this change.