SetColormap causes seg-fault

[ArekSredzki]

First, thanks for the great plotting library!

Unfortunately, when updating from 6a5e940 to da5b4ab last night, I hit a nasty seg fault in code that used to work.
It appears that the addition of PushColormap may have broken some use cases of SetColormap.

The following code used to work but now crashes:

  if (!ImPlot::BeginPlot("##VelocityPlot", "Time (s)", "Velocity (m/s))
  {
    return;
  }

  constexpr size_t num_colors = 5UL;
  const static ImVec4 color_map[num_colors]{
    ... colors ...
  };
  ImPlot::SetColormap(color_map, num_colors);

  ...PlotLines...

  ImPlot::SetColormap(ImPlotColormap_Default);
  ImPlot::EndPlot();

Changing SetColormap to PushColormap and PopColormap as so fixes it:

  if (!ImPlot::BeginPlot("##VelocityPlot", "Time (s)", "Velocity (m/s)")
  {
    return;
  }

  constexpr size_t num_colors = 5UL;
  const static ImVec4 color_map[num_colors]{
    ... colors ...
  };
  ImPlot::PushColormap(color_map, num_colors);

  ...PlotLines...

  ImPlot::PopColormap();
  ImPlot::EndPlot();

What made this especially difficult to pinpoint was that the seg-fault occurred in GetLegendLabel called by EndPlot. Note that moving SetColormap to come after EndPlot did not fix the issue.

It'd be great to find out what this caused a crash (I didn't have the time to dig into this) but also to ensure that changes such as this are documented under the API BREAKING CHANGES section in the future.

It's worth noting that I see that PushColormap is now preferable, but a seg-fault wasn't the best way to discover that :)

Thanks!

[epezent]

@ArekSredzki, thanks for the bug report. I'll take a look and see if anything seems strange.

[epezent]

Sorry, I'm wrong. Your code is fine!! I'll look into this

[bear24rw]

ImPlot::SetColormap(ImPlotColormap_Default); uses the default samples value of zero which calls BustItemCache() which clears all the items from the plot. Then when End() is called and it goes to plot everything it goes out of bound on gp.CurrentPlot->Items.GetByIndex in the GetLegendLabel function.

Idk what the fix is but i think that's what's happening.

[epezent]

@bear24rw , I think you are absolutely right. I may be able to resolve this by removing BustItemCache here (I'm trying to remember exactly why I have that code there, to be honest). I guess it didn't occur to me that folks might want to set a new colormap mid-plot.

@ArekSredzki, may I ask why you choose to call SetColormap inside of Begin/EndPlot and not outside? If you don't have a particular reason, the simplest solution may be for you to move that code outside, and then for us to add some asserts to prevent misuse.

[epezent]

After a little investigation, I think it is a design flaw on my part to call BustItemCache inside of SetColormap. What this effectively does is clears the cached color for every plot item so that previous plots will be resampled from the new colormap the next time through. If that happens between BeginPlot and EndPlot, then the cached item state which is used for legend rendering breaks, as demonstrated above. I have removed BustItemCache from SetColormap.

The reason that BustItemCache was even there was so that changing the colormap in the demo would reset the colors of every plot item. I can explicitly make a call to BustItemCache after SetColormap in those scenarios.

@ArekSredzki , can you test your code with this commit: 8a3ccf0

[epezent]

@ArekSredzki , any luck? Let me know if it's working so I can close the issue. Thanks!

[ArekSredzki]

Thanks @epezent ! I just checked and it now works properly with both methods :)