ocornut/imgui

Fork 0

mirror of https://github.com/ocornut/imgui.git synced 2026-11-14 19:04:44 +00:00

Bug with VertexBuffer size > UINT16_MAX #514

New issue

Closed

opened 2016-01-30 12:53:44 +00:00 by itamago · 11 comments

itamago commented

2016-01-30 12:53:44 +00:00

(Migrated from github.com)

Hello,

With lot of widgets, I have an ImGUI generated vertex-buffer with 71180 vertices (so, more than UINT16_MAX) :

int countVertices = cmd_list->VtxBuffer.size();
ASSERT(countVertices < UINT16_MAX);

The IndexBuffers are defined using ImDrawIdx with its standard definition :

typedef unsigned short ImDrawIdx;

It would mean that some vertices won't be reachable. Not a big deal.
But, the result is more interesting : the App violently crashes the METAL api, and the OS remains completely frozen (thx for these drivers Mr. Apple). So I reboot between each test.

I know that I can customise the ImDrawIdx and use 32 bit indices, but that would be too easy :)

I think there may be a bug in the VertexBuffer generation, or at least there is missing an ASSERT checking that the size of VertexBuffers match the indice length.

EDIT: I just made an expensive change in my engine, just to be sure, by using 32bits indices everywhere (in ImGui too). It works. But, I continue to think that a good old ASSERT in ImGui to verify that vertex-buffers have a correct size (depending on the ImDrawIdx length) would spare some time for developers.

Hello, With lot of widgets, I have an ImGUI generated vertex-buffer with 71180 vertices (so, more than UINT16_MAX) : ``` cpp int countVertices = cmd_list->VtxBuffer.size(); ASSERT(countVertices < UINT16_MAX); ``` The IndexBuffers are defined using ImDrawIdx with its standard definition : ``` cpp typedef unsigned short ImDrawIdx; ``` It would mean that some vertices won't be reachable. Not a big deal. But, the result is more interesting : the App violently crashes the METAL api, and the OS remains completely frozen (thx for these drivers Mr. Apple). So I reboot between each test. I know that I can customise the ImDrawIdx and use 32 bit indices, but that would be too easy :) I think there may be a bug in the VertexBuffer generation, or at least there is missing an ASSERT checking that the size of VertexBuffers match the indice length. EDIT: I just made an expensive change in my engine, just to be sure, by using 32bits indices everywhere (in ImGui too). It works. But, I continue to think that a good old ASSERT in ImGui to verify that vertex-buffers have a correct size (depending on the ImDrawIdx length) would spare some time for developers.

ocornut commented

2016-01-30 13:51:30 +00:00

(Migrated from github.com)

There's already an assert in AddDrawListToRenderList()

// Check that draw_list doesn't use more vertices than indexable (default ImDrawIdx = 2 bytes = 64K vertices)
// If this assert triggers because you are drawing lots of stuff manually, A) workaround by calling BeginChild()/EndChild() to put your draw commands in multiple draw lists, B) #define ImDrawIdx to a 'unsigned int' in imconfig.h and render accordingly.
const unsigned long long int max_vtx_idx = (unsigned long long int)1L << (sizeof(ImDrawIdx)*8);
(void)max_vtx_idx;
IM_ASSERT((unsigned long long int)draw_list->_VtxCurrentIdx <= max_vtx_idx); // Too many vertices in same ImDrawList

It is a bug if it doesn't trigger, could you investigate?
You can also look at the ImGui metrics windows for some extra information.

However note that you could and should probably add a similar test in your backend especially if you know it is crashing your system...

71180 vertices in a single window is an unusual amount, are you doing custom drawing?
While the ImDrawList api is convenient with anti-aliasing and rounding enabled it currently uses a lot of vertices. If all you are doing is drawing axis-aligned rounded rectangle/outline I would like eventually to bake some of the rendering into the texture atlas to save on vertices. But realistically no time to do so.

You can also manually split draw commands to avoid getting past the 64k limits and save that bandwidth. See the thread
https://github.com/ocornut/imgui/issues/292

There's already an assert in AddDrawListToRenderList() ``` // Check that draw_list doesn't use more vertices than indexable (default ImDrawIdx = 2 bytes = 64K vertices) // If this assert triggers because you are drawing lots of stuff manually, A) workaround by calling BeginChild()/EndChild() to put your draw commands in multiple draw lists, B) #define ImDrawIdx to a 'unsigned int' in imconfig.h and render accordingly. const unsigned long long int max_vtx_idx = (unsigned long long int)1L << (sizeof(ImDrawIdx)*8); (void)max_vtx_idx; IM_ASSERT((unsigned long long int)draw_list->_VtxCurrentIdx <= max_vtx_idx); // Too many vertices in same ImDrawList ``` It is a bug if it doesn't trigger, could you investigate? You can also look at the ImGui metrics windows for some extra information. However note that you could and should probably add a similar test in your backend especially if you know it is crashing your system... 71180 vertices in a single window is an unusual amount, are you doing custom drawing? While the ImDrawList api is convenient with anti-aliasing and rounding enabled it currently uses a lot of vertices. If all you are doing is drawing axis-aligned rounded rectangle/outline I would like eventually to bake some of the rendering into the texture atlas to save on vertices. But realistically no time to do so. You can also manually split draw commands to avoid getting past the 64k limits and save that bandwidth. See the thread https://github.com/ocornut/imgui/issues/292

itamago commented

2016-01-30 15:03:01 +00:00

(Migrated from github.com)

You're right, there is an ASSERT which is not triggered, and I'm doing intensive tests of ImGUI with lot of things on the screen.
I rollbacked my modifs in order to debug with 16bits indices. Give me few minutes to see what's wrong.

(after thinking a little bit, I finally decided to keep 32bits indices which I should have done one day or another, so this isn't a blocking point for me anymore)

EDIT: the figures from the Metrics window are consistent with the number of vertices (>= 65536).
Need to understand why the im_assert is not triggered by mine is...

You're right, there is an ASSERT which is not triggered, and I'm doing intensive tests of ImGUI with **lot** of things on the screen. I rollbacked my modifs in order to debug with 16bits indices. Give me few minutes to see what's wrong. (after thinking a little bit, I finally decided to keep 32bits indices which I should have done one day or another, so this isn't a blocking point _for me_ anymore) EDIT: the figures from the Metrics window are consistent with the number of vertices (>= 65536). Need to understand why the im_assert is not triggered by mine is...

itamago commented

2016-01-30 15:14:34 +00:00

(Migrated from github.com)

Ok, there is a missing assert somewhere else.

Here is a short log of what is happening :

"V" and "I" for vertex-count and indices-count in the final lists provided by the API
"IM-V" is printed when there is a check in ImGui

IM_ASSERT((unsigned long long int)draw_list->_VtxCurrentIdx <= max_vtx_idx); // Too many vertices in same ImDrawList
printf("IM-V: %u\n", draw_list->_VtxCurrentIdx);

V: 256
I: 921
V: 1060
I: 1680
V: 118
I: 363
V: 96
I: 384
V: 96
I: 384
V: 96
I: 384
V: 244
I: 903
V: 472
I: 984
V: 65274
I: 240081
V: 2992
I: 6972

IM-V: 256
IM-V: 1152
IM-V: 118
IM-V: 96
IM-V: 96
IM-V: 96
IM-V: 244
IM-V: 472
IM-V: 5644
IM-V: 2992

As you can see, there is vertex-buffer which is close to explode (V: 65274), and it explodes when I create new fake guis.
But the verification in ImGui doesn't show anything wrong.

There should be another verification to add.

Ok, there is a missing assert somewhere else. Here is a short log of what is happening : - "V" and "I" for vertex-count and indices-count in the final lists provided by the API - "IM-V" is printed when there is a check in ImGui ``` cpp IM_ASSERT((unsigned long long int)draw_list->_VtxCurrentIdx <= max_vtx_idx); // Too many vertices in same ImDrawList printf("IM-V: %u\n", draw_list->_VtxCurrentIdx); ``` ``` V: 256 I: 921 V: 1060 I: 1680 V: 118 I: 363 V: 96 I: 384 V: 96 I: 384 V: 96 I: 384 V: 244 I: 903 V: 472 I: 984 V: 65274 I: 240081 V: 2992 I: 6972 IM-V: 256 IM-V: 1152 IM-V: 118 IM-V: 96 IM-V: 96 IM-V: 96 IM-V: 244 IM-V: 472 IM-V: 5644 IM-V: 2992 ``` As you can see, there is vertex-buffer which is close to explode (V: 65274), and it explodes when I create new fake guis. But the verification in ImGui doesn't show anything wrong. There should be another verification to add.

itamago commented

2016-01-30 15:19:00 +00:00

(Migrated from github.com)

Don't you think the ASSERT should be this one instead of the existing one ?

IM_ASSERT((unsigned long long int)draw_list->VtxBuffer.Size <= max_vtx_idx); // Too many vertices in same ImDrawList

This one is triggered.

Don't you think the ASSERT should be this one instead of the existing one ? ``` cpp IM_ASSERT((unsigned long long int)draw_list->VtxBuffer.Size <= max_vtx_idx); // Too many vertices in same ImDrawList ``` This one is triggered.

ocornut commented

2016-01-30 15:32:10 +00:00

(Migrated from github.com)

_VtxCurrentIdx should always be equal to _VtxBuffer.Size at the end of rendering, else it means that PrimReserve() haven't been used correctly, or there is a bug somewhere.

In ShowMetrics if I change
bool node_opened = ImGui::TreeNode(draw_list, "%s: '%s' %d vtx, %d indices, %d cmds", label, draw_list->_OwnerName ? draw_list->_OwnerName : "", draw_list->VtxBuffer.Size, draw_list->IdxBuffer.Size, draw_list->CmdBuffer.Size);
to
bool node_opened = ImGui::TreeNode(draw_list, "%s: '%s' %d/%d vtx, %d indices, %d cmds", label, draw_list->_OwnerName ? draw_list->_OwnerName : "", draw_list->VtxBuffer.Size, draw_list->_VtxCurrentIdx, draw_list->IdxBuffer.Size, draw_list->CmdBuffer.Size);

I only seem to get matching values.
So there is something fishy. Do you have a repro?

V: 256
V: 1060  // why is it != 1152?
V: 118
V: 96
V: 96
V: 96
V: 244
V: 472
V: 65274 // *edit* hmm why !=5644
V: 2992

IM-V: 256
IM-V: 1152
IM-V: 118
IM-V: 96
IM-V: 96
IM-V: 96
IM-V: 244
IM-V: 472
IM-V: 5644
IM-V: 2992

_VtxCurrentIdx should always be equal to _VtxBuffer.Size at the end of rendering, else it means that PrimReserve() haven't been used correctly, or there is a bug somewhere. In ShowMetrics if I change `bool node_opened = ImGui::TreeNode(draw_list, "%s: '%s' %d vtx, %d indices, %d cmds", label, draw_list->_OwnerName ? draw_list->_OwnerName : "", draw_list->VtxBuffer.Size, draw_list->IdxBuffer.Size, draw_list->CmdBuffer.Size);` to `bool node_opened = ImGui::TreeNode(draw_list, "%s: '%s' %d/%d vtx, %d indices, %d cmds", label, draw_list->_OwnerName ? draw_list->_OwnerName : "", draw_list->VtxBuffer.Size, draw_list->_VtxCurrentIdx, draw_list->IdxBuffer.Size, draw_list->CmdBuffer.Size);` I only seem to get matching values. So there is something fishy. Do you have a repro? ``` V: 256 V: 1060 // why is it != 1152? V: 118 V: 96 V: 96 V: 96 V: 244 V: 472 V: 65274 // *edit* hmm why !=5644 V: 2992 IM-V: 256 IM-V: 1152 IM-V: 118 IM-V: 96 IM-V: 96 IM-V: 96 IM-V: 244 IM-V: 472 IM-V: 5644 IM-V: 2992 ```

itamago commented

2016-01-30 15:38:08 +00:00

(Migrated from github.com)

Difficult to share my complete source code to reproduce it... or in private maybe.
I sent you an email

Difficult to share my complete source code to reproduce it... or in private maybe. I sent you an email

ocornut commented

2016-01-30 15:52:01 +00:00

(Migrated from github.com)

OK I think I've found the bug in ImDrawList::AddText()

_VtxCurrentIdx = (ImDrawIdx)VtxBuffer.Size;

should be

_VtxCurrentIdx = (unsigned int)VtxBuffer.Size;

Could you confirm?

OK I think I've found the bug in ImDrawList::AddText() `_VtxCurrentIdx = (ImDrawIdx)VtxBuffer.Size;` should be `_VtxCurrentIdx = (unsigned int)VtxBuffer.Size;` Could you confirm?

itamago commented

2016-01-30 15:55:16 +00:00

(Migrated from github.com)

Great, thanks !

EDIT : I confirm the bug is fixed => the ASSERT is correctly triggered.

Great, thanks ! EDIT : I confirm the bug is fixed => the ASSERT is correctly triggered.

ocornut commented

2016-01-30 16:03:11 +00:00

(Migrated from github.com)

Committed fix and added an extra assert:
IM_ASSERT((int)draw_list->_VtxCurrentIdx == draw_list->VtxBuffer.Size); // Sanity check. Bug or mismatch between PrimReserve() calls and incrementing _VtxCurrentIdx, _VtxWritePtr etc.

I think there is something fishy because I can't explain the 1060/1152 difference, and 5644 isn't 65274 mod 64K. So this second assert may also trigger even as you switch to 32-bits and I'm interested in hearing about it. Thanks!

Committed fix and added an extra assert: `IM_ASSERT((int)draw_list->_VtxCurrentIdx == draw_list->VtxBuffer.Size); // Sanity check. Bug or mismatch between PrimReserve() calls and incrementing _VtxCurrentIdx, _VtxWritePtr etc.` I think there is something fishy because I can't explain the 1060/1152 difference, and 5644 isn't 65274 mod 64K. So this second assert may also trigger even as you switch to 32-bits and I'm interested in hearing about it. Thanks!

itamago commented

2016-01-30 17:12:28 +00:00

(Migrated from github.com)

Thanks for the fix !
I will bump ImGui as soon as I finish the migration to 32 bits (I disabled tri-strips to be quicker... and I'm moving from NvTriStrip (16bits) to TriStripper (32bits)). It should take me one hour. I keep you informed when I have some results. Thx again !

Thanks for the fix ! I will bump ImGui as soon as I finish the migration to 32 bits (I disabled tri-strips to be quicker... and I'm moving from NvTriStrip (16bits) to TriStripper (32bits)). It should take me one hour. I keep you informed when I have some results. Thx again !

itamago commented

2016-01-30 19:48:55 +00:00

(Migrated from github.com)

I bumped ImGui to tip.
No assert is triggered, so the values should be correct.

But in release, I noticed that some columns are missing in my test-case. I will dig into it tomorrow..

I bumped ImGui to tip. No assert is triggered, so the values should be correct. But in release, I noticed that some columns are missing in my test-case. I will dig into it tomorrow..