[Impeller] Change imgui shader color attribute from int to vec4 & convert on host #32981
Conversation
|
It looks like this pull request may not have tests. Please make sure to add tests before merging. If you need an exemption to this rule, contact Hixie on the #hackers channel in Chat (don't just cc him here, he won't see it! He's on Discord!). If you are not sure if you need tests, consider this rule of thumb: the purpose of a test is to make sure someone doesn't accidentally revert the fix. Ask yourself, is there anything in your PR that you feel it is important we not accidentally revert back to how it was before your fix? Reviewers: Read the Tree Hygiene page and make sure this patch meets those guidelines before LGTMing. |
| // ...as opposed to: | ||
| // glVertexAttribPointer(2, 4, GL_FLOAT, false, 16, nullptr); | ||
| // | ||
| // impellerc could e.g. detect some kind of special naming convention hint |
There was a problem hiding this comment.
I'd rather this bit be in an issue instead of a comment.
| vtx_data.reserve(cmd_list->VtxBuffer.size()); | ||
| for (const auto& v : cmd_list->VtxBuffer) { | ||
| const impeller::Scalar kScale = 1.0f / 255.0f; | ||
| impeller::Vector4 color(((v.col >> 0) & 0xFF) * kScale, // |
There was a problem hiding this comment.
Nit: ColorConvertU32ToFloat4 instead. I bet it does exactly the same thing but I see defines in there to check to see if the format is BGRA instead. Good to not have this be tied to RGBA here.
|
|
||
| auto draw_list_vtx_bytes = | ||
| static_cast<size_t>(cmd_list->VtxBuffer.size_in_bytes()); | ||
| static_cast<size_t>(vtx_data.size() * sizeof(VS::PerVertexData)); |
There was a problem hiding this comment.
Probably (definitely) missing the forest for the trees here but IMGUI_OVERRIDE_DRAWVERT_STRUCT_LAYOUT will let you specify a custom layout (you can make it PerVertexData). That way, you can re-order the elements in place without an additional allocation.
Your call on if you want to do this. Probably easier TBH and saves a copy. But this way works too.
There was a problem hiding this comment.
Cool! I think for this we would add some padding in the struct and then overwrite col + the extra space in place:
#define IMGUI_OVERRIDE_DRAWVERT_STRUCT_LAYOUT \
struct ImDrawVert { \
ImVec2 pos; \
ImVec2 uv; \
ImU32 col; \
float[3] col_gba; \
};
But I'm not certain it's safe to modify the existing col data in-place to prevent the extra copy?
I'm also slightly biased towards not requiring overrides in imconfig for this backend. If people use Impeller in the future for anything else, it'd be nice if they can just drop it in and it'll magically work.
There was a problem hiding this comment.
The comment for the macro seems to indicate that it is. But your approach is safer and the perf differences minimal.
|
test-exempt: code refactor with no semantic change |
|
(and also, "is a test") |
…c4 & convert on host (flutter/engine#32981)
4 participants
Only affects playgrounds.
Changes the color attribute to
vec4in the shader. On the host, convertImDrawVerts toImguiRasterVertexShader::PerVertexDatabefore copying to the device buffer since they no longer map 1-1.This works around the lack of support for int attributes and bitwise operations in GLSL ES 1.
I added a comment inline describing a possible impellerc-based solution to enable support for color ints that maybe we can try later.