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

Fix ImDrawList::AddImageRounded #1994

Merged
merged 1 commit into from
Aug 3, 2024

Conversation

Soreepeong
Copy link
Contributor

The function makes an assumption that there exists 1 font atlas texture, so that ImDrawList._Data->TexIdCommon ==
ImDrawList._CmdHeader.TextureId. Since we support multiple font atlas textures, that assumption is no longer true and
ImDrawList::AddConvexPolyFilled will create a new draw command as needed, giving ImGui::ShadeVertsLinearUV a clean draw command to work with.

This workaround forcefully sets the font atlas texture to be the texture the user is trying to draw for the duration of drawing polygons and shading those vertices again, so that no draw command change happens. Once the operation is done, font atlas texture is reverted back to what it was.

This fix is done without thread safety concerns, but an ImDrawList should not be touched from multiple threads at a single time, so this is fine.

The function makes an assumption that there exists 1 font atlas texture,
so that `ImDrawList._Data->TexIdCommon` ==
`ImDrawList._CmdHeader.TextureId`. Since we support multiple font atlas
textures, that assumption is no longer true and
`ImDrawList::AddConvexPolyFilled` will create a new draw command as
needed, giving `ImGui::ShadeVertsLinearUV` a clean draw command to work
with.

This workaround forcefully sets *the* font atlas texture to be the
texture the user is trying to draw for the duration of drawing polygons
and shading those vertices again, so that no draw command change
happens. Once the operation is done, font atlas texture is reverted back
to what it was.

This fix is done without thread safety concerns, but an `ImDrawList`
should not be touched from multiple threads at a single time, so this is
fine.
@Soreepeong Soreepeong requested a review from a team as a code owner August 3, 2024 09:24
@Soreepeong
Copy link
Contributor Author

https://github.com/goatcorp/gc-imgui/blob/e4920dc10cbf32c0cd9b4a48a4a71c8814242460/imgui_draw.cpp#L1669-L1693

ImDrawList::PathFillConvex in turn calls ImDrawList::AddConvexPolyFilled, which will push and pop the common texture ID as needed during its scope, which will often end up adding an empty draw command after it's done.
https://github.com/goatcorp/gc-imgui/blob/e4920dc10cbf32c0cd9b4a48a4a71c8814242460/imgui_draw.cpp#L993-L1001

ImGui::ShadeVertsLinearUV works on the most recent draw command.
https://github.com/goatcorp/gc-imgui/blob/e4920dc10cbf32c0cd9b4a48a4a71c8814242460/imgui_draw.cpp#L1909-L1931

@KazWolfe
Copy link
Member

KazWolfe commented Aug 3, 2024

At what point are we just going to hardfork ImGui?

@KazWolfe KazWolfe merged commit 23a2bd6 into goatcorp:master Aug 3, 2024
3 checks passed
@Soreepeong
Copy link
Contributor Author

We already did; we're just trying to not rebuild cimgui.dll for the time being. Proper fix should go in with ImGui version update

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants