Fix for ImDrawListSplitter::Merge - corrupted data in buffer (64k+ vertices) with 16-bits indices #3129

[thedmd]

This is fix for ImDrawListSplitter::Merge - corrupted data in buffer (64k+ vertices) with 16-bits indices #3129

Thanks for @ShironekoBen for finding underlying issue and making initial patch.

Basically issue was caused by draw commands cached while making channel split and when switching channels, they internal state (texture ID, clip rect and vertex offset) may no longer match on in ImDrawList.

Another partial fix will be to not create draw commands while preparing split since check now is always present in SetCurrentChannel. Fill will be partial because on heavy use of vertices we can still hit state when draw command is stale.

Pre-allocating single draw command on split may no longer be necessary since SetCurrentChannel always check for validity after switch. Also for sparse channel splits there are a lot of unused draw commands allocated, just to be discarded on merge.
This is a case for Node Editor where each node has few assigned channels, where most of them are empty (one for selection, one for highlights, one for background, etc) most of the time. So maybe this change can also be considered.

Introduction of channel merging based on bounding boxes may further reduce number of generated draw commands. They number can quickly explode when 16-bit indices are used with large meshes:

Code used for testing:
https://gist.github.com/thedmd/e384a83c0fe9e82ec0f69c869510ad44

Repro on non-patched ImGui copy:

• Copy&paste above gist
• Invoke TestSplitterBug()

[franciscod]

Is the backend checker something that should be added? Only on the win32/dx11 example?

[thedmd]

Index data generation in draw list was corrupted, so this is not related to backend. All supporting vtxOffset were affected.

[thedmd]

@ocornut , @ShironekoBen Can you please take a look at this fix?

[ocornut]

Looked a little bit at this and talked to @thedmd for help.

Two observations:

• Thinking about all this is confusing and we're fixing rarely taken code paths, so I think we should be methodically adding regression tests for all this before proceeding with applying a patch.
• I haven't done measurements so may be overthinking this, but I have concerns that extra code in SetCurrentChannel() might have performance impact. SetCurrentChannel() may be called very frequently and we generally want to optimize for it (hence the level of memcpy low-levelness in current version already :)

[ShironekoBen]

Cool - looks pretty good to me. Just a couple of thoughts from here:

1. Having the merge code in UpdateStaleDrawCmd() feels a bit weird to me... the majority of the time the last two commands in the list won't be mergeable - correct me if I'm wrong here, but I think the only case where they will be (outside of user error or not-very-likely corner cases) is specifically when writing out the final merged commands, so in that context I think it might make more sense to do that check there rather than bundling it up into the "common" code-path. (this would also reduce the load on SetCurrentChannel() a bit)

(edit: ignore this, I'm an idiot, see below)
2) I think I'd be inclined to remove the IM_ASSERT(cmd_size >= window->DrawList->CmdBuffer.Size) from PushColumnsBackground() entirely - if we can't guarantee that the clip region change doesn't generate a draw command (which seems likely) then testing that the draw command count didn't decrease doesn't really achieve anything. In fact, right now the only case I can think of where that might fire is a benign one of the corner cases above, where the last two draw commands are both identical and share the correct clip rect - in which case they would get merged, reducing the count, but that's fine and so shouldn't trigger an assert anyway.

Edit: Oops, sorry - read that assert backwards - it's checking for the opposite of what I thought it was. In that case the assert makes sense, although I do wonder if we can be sure the clip region is the same at that point?

[sergeyn]

Guys, would also take a look at my PR #3232
It fixes a case when you do PrimReserve(something) followed by PrimUnreserve(same something) . I'm not sure if your PR fixes this case or not.
Regards.

[thedmd]

Fixed by 84862ec

[ocornut]

We spent a long while working on this (thanks @thedmd for the precious help), it's been quite complicated to get it right without altering performance (the initial PR for #3163 had too much overhead in column switching). Because the matter was so confusing we split the work leading to this only many commits:

78d5ccf
e22e3f3
003153b
0320e72
41f47c8
f6120f8
a6bb047
117d57d
e35a813
b1f2eac
3bef743
84862ec

We simplified much of the ImDrawList command handling code, and in turn some code could get further optimizations (columns switch are now faster).