VtxOffset offset related fix (fixes blown up triangles on the screen)

[sergeyn]

It is possible to get VtxCurrentOffset and VtxCurrentIdx out of sync with buffers state. This scenario triggers it: Do PrimReserve , which thinks you are reserving too many vertices and triggers new draw command and vtxoffset update. Then you do unreserve same amount (leaving emptry draw command). Next clip rect update this draw command gets poped but does not restore vtx/idx buffer offset, resulting all following draw commands reference incorrect vertex data

[ocornut]

Linking to #3163, #3129

[sergeyn]

Is there anything preventing this small fix to get merged ?

[ocornut]

Hello,

1. PR doesn't follow the coding style, which in my experience correlate strongly with buggy or incomplete PR so it automatically goes in the low-priority side of the pile, I'm sorry for that but juggling with lots of tasks I have to give weight to those long established heuristics.

2. I have linked to two issues with more complete discussion/solution for this, which previously existed, and would expect a reaction as to whether this is already covered by Fix for ImDrawListSplitter::Merge - corrupted data in buffer (64k+ vertices) with 16-bits indices #3129 #3163 or not as your PR didn't mention it.

Regardless, #3163 is rather complex and tricky, I have asked thedmd to write more formal automated/regression tests for it because it's a rarely exercised path. I expect this to be solved shortly!

[sergeyn]

It's just two line of code, what coding style issues are there ? few spaces missing ?
regarding #3163 - as you said, it's a complicated commit and I don't understand what exactly it is solving and I don't have much time to figure it out, so I can't say if it does already fix what mine commit does or not. I can poke author of #3163 to review mine.
Thanks for your answer anyway.

[rokups]

It's just two line of code, what coding style issues are there ?

In Dear ImGui we place curly braces either on all if/else branches, or none. Since you added them after if - add them after else as well, even if it has just one line.

[sergeyn]

Just for your information, this project suffers from a bug in ImGui this PR fixes.

[epezent]

To weigh in here, Omar, we are trying to ensure ImPlot works as expected when using 16-bit indices and plotting more than 64k points (not that uncommon for our users). We know supporting 16-bit indices would be most useful for you per #2591 and so that is a part of our motivation.

For performance reasons (you can see the results here), we bypass DrawList.AddLine and fill the idx/vtx buffers ourselves. Our render loop upfront PrimReserves for the total number of segments in a line strip, iterates the segments buffering only those visible on screen, and then PrimUnreserves for the number of segments that were culled and not added to the buffers.

As it is, we see artifacting like this. This is in-line with the issue I spoke of in #3245 (comment)

@sergeyn has suggested that this PR will solve our issues, but understandably there may be a more systemic issue at play given the other two PRs linked above. I just personally test #3163 and it did NOT resolve our issue. Artifacting is still happening just as above.

Let us know how we can help move this along. Cheers!

[ocornut]

@sergeyn Would you be able to submit a minimal repro code for this issue?

[ocornut]

1. I managed to build a repro for this based on another test made for 3163
2. Also managed to repro the issue in ImPlot by removing the ImGuiCond_Always cond in the SetNextPlotLimits() call under benchmark.
3. However @epezent if I merge Fix for ImDrawListSplitter::Merge - corrupted data in buffer (64k+ vertices) with 16-bits indices #3129 #3163 I cannot see the bug anymore, and looking at the code I would expect it was fixed, so I'm particularly confused by your statement that "I just personally test Fix for ImDrawListSplitter::Merge - corrupted data in buffer (64k+ vertices) with 16-bits indices #3129 #3163 and it did NOT resolve our issue". Would you mind double-checking that no git-mystery-confusion affected your testing?

Thanks all!

[epezent]

That's great news! It's very well possible that I mixed up git commits when testing it. I'll give it another shot.

[sergeyn]

I confirm 3163 does not fix this problem.

[sergeyn]

To reproduce - grab latest implot and use following code to do a plot. This is spiral shifted 200,200 up-right so that it is completely culled. You can also remove a 200 shift and shift it with a mouse to observe same behavior:

if (ImPlot::BeginPlot("my plot", nullptr, "plot"))
        {
           static const int num_plot_points = 20000;
            ImPlot::PlotLine("sin(50*x)",
               [](void*, int idx) {
                  float r = 0.9f; // outer radius
                  float a = 0; // inner radius
                  float b = 0.05f; // incerement per rev
                  float n = (r - a) / b; // number  of revolutions
                  double th = 2 * n * M_PI; // angle
                  float Th = float(th * idx / (num_plot_points - 1));
                  return ImPlotPoint(200+0.5f + (a + b * Th / (2.0f * (float)M_PI)) * cosf(Th), 200+0.5f + (a + b * Th / (2.0f * (float)M_PI)) * sinf(Th));
               }, nullptr, num_plot_points);
            ImPlot::EndPlot();
        }

I see following image with #3163:

[ocornut]

Thanks for the details, we will investigate again tomorrow.
Trying to build simple repros which we can use as regression tests, so will work on that tomorrow too.
(One issue I have with #3163 as-is is the addition of extra code in ImDrawListSplitter::SetCurrentChannel which can be noticeable for large table set, so either way it needs to be improved)

[epezent]

@ocornut, I just did another round of tests, and it would seem that #3163 does work for me. I'm not sure why it didn't work for me on my first attempt, or why it's not working for @sergeyn, but here are some points to consider:

1. My first attempt I tried this inside of your test framework using the default DX11 backend.
2. This attempt, l used the official GLFW and OpenGL3 example backends.

I suspect my issue was simply git incompetence, but I'm still not sure why it wouldn't be working for @sergeyn. Screenshots ...

Master

#3163

@ocornut, note the spiral being truncated even when ImGuiBackendFlags_RendererHasVtxOffset is enabled. Our fix for this is in epezent/implot#41, but it can't be merged until the ImGui issue is resolved.

[ocornut]

Likewise, I tried @sergeyn spiral code above above #3163 and cannot repro..... Without 3163 i get the issue..

[sergeyn]

Well, I can try again to eliminate any git screw-up

[ocornut]

Would you good if you tried... It would be really weird if both of you did a git screw-up. Although I would prefer if that was the case since it would mean we don't have this issue with 3163...

All of this is so confusing we'll just write more regression tests for it now.

[sergeyn]

It looks like it works.

It would be really weird if both of you did a git screw-up
First time I used "download zip" and it looks like it downloaded a different branch . This time I used proper checkout and it seem to have fixed the problem.

[epezent]

Great! @ocornut, sorry for the confusion.

[ocornut]

I merged a fix for this specific issue because it is simpler:
e22e3f3

Note that #3163 cover other issues which haven't been merged yet, related to the channel splitter Merge() function (I don't think ImPlot relies on this at all).

Thanks all for your help. This code is particularly tricky so I'm still untangling it and writing more/better regression tests.

[epezent]

Thank you Omar!