epezent / implot

Immediate Mode Plotting
MIT License
4.66k stars 520 forks source link

Incorrect stride behaviour when sizeof(T) == sizeof(float) #303

Closed joshtyler closed 2 years ago

joshtyler commented 2 years ago

I think there is a bug when striding data that happens to have the same size as a float

To reproduce:

  1. Compile and run the demo, look at Custom->Custom Data Getters and Setters. The Vector2f line should be a line from (0,0), to (1,1)
  2. Change this line to int16_t x,y; https://github.com/epezent/implot/blob/6ee1559715fae9480fcaeb81f24d80a4d1e8c407/implot_demo.cpp#L48
  3. Compile and run the demo again. Look at the same plot. The Vector2f line now (incorrectly) goes from (0,0) to (1,0).

We would expect this change to have no effect, since where the structure is used, stride is passed in as sizeof(MyImPlot::Vector2f) https://github.com/epezent/implot/blob/6ee1559715fae9480fcaeb81f24d80a4d1e8c407/implot_demo.cpp#L1680

Root cause:

A few months ago IndexData() was introduced to speed up rendering. The bug seems to be on this line https://github.com/epezent/implot/blob/6ee1559715fae9480fcaeb81f24d80a4d1e8c407/implot_items.cpp#L290

On this line I think sizeof(float)should be sizeof(T). Where sizeof(T) == sizeof(float), but the data type contains multiple values (e.g. in this case T is {int16_t, int16_T}), The function will do the wrong thing

N.B. This fix should also speed up rendering for other data types, which are currently taking a pessimistic route through IndexData()!

epezent commented 2 years ago

Oof! What a blunder. That should defintely be sizeof(T). Thanks for the catch! I just pushed a fix.