Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

add SetNextColorsData to give per point colors #332

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

niavok
Copy link

@niavok niavok commented Feb 21, 2022

Hello,

I needed to have color per point for lines and markers so I developed the feature .

image

This request has already been discuted here :
#298
#321 (comment)

The patch add only one function allowing to override the color used for the data:

// Set color buffer for the next item only. The colors array must be keep alive until the next item plot.
IMPLOT_API void SetNextColorsData(ImPlotCol_ target, ImU32* const& colors, int stride = sizeof(ImU32));

I know you are not convinced by the need of implicit lifetime management of the color pointer but with 2 calls there is not lot of option without data copy.
As the SetNextColorsData call is often done just before the plot call this liftime management is not so dangerous for most developers. If you find it too dangerous, it could be moved in implot_internal.h

The internal implementation mainly replace some "ImU32 col" by some "const ImU32* col_ptr, int col_stride" parameter. This allow to use a stride of 0 to support easily the legacy behavior with the Style color in the same code path.

I tested and added demo for lines, scatter, VLines and HLines but it should work for other plots.

For the line plot, the color of the first point is used for the the whole line instead of a gradient between the both point color. This is because the AddLine of the ImGui API take only 1 color even if deep inside the draw API, in ImDrawList::AddPolyline, 2 different color could have been provided leading to a gradient in most renderers:

for (int i = 0; i < points_count; i++)
{
_VtxWritePtr[0].pos = temp_points[i * 2 + 0]; _VtxWritePtr[0].uv = tex_uv0; _VtxWritePtr[0].col = col; // Left-side outer edge
_VtxWritePtr[1].pos = temp_points[i * 2 + 1]; _VtxWritePtr[1].uv = tex_uv1; _VtxWritePtr[1].col = col; // Right-side outer edge
_VtxWritePtr += 2;
}

If you have better idea of API or implementation, i can rework the patch, or improve the demo if needed. As the patch modify the internal API of implot, it will be a lot easier to keep sync with the feature directly in the upstream.

Thanks !
Frédéric

@epezent
Copy link
Owner

epezent commented Feb 21, 2022

First, this is amazing work! Thank you for opening this PR

I know you are not convinced by the need of implicit lifetime management of the color pointer but with 2 calls there is not lot of option without data copy.

We should discuss this in the context of the proposal in #330. It's likely in the near future that we lump styling parameters in the PlotX functions and deprecate the decoupled SetNext and Push/Pop item styling API.

For the line plot, the color of the first point is used for the the whole line instead of a gradient between the both point color. This is because the AddLine of the ImGui API take only 1 color even if deep inside the draw API, in ImDrawList::AddPolyline, 2 different color could have been provided leading to a gradient in most renderers:

I plan to eleminate the ImGui fallback implementation in #319 -- it only exists to provide an AA option for line plot (at some point we will add back AA via RendererLineStripAA) So, I think it makes more sense to have individual line segments form a gradient between end points.

@epezent
Copy link
Owner

epezent commented Feb 21, 2022

Scanning the code, the first idea that jumps out to me is possibly putting color indexing/getting into the existing Getter structs. These currently return an ImPlotPoint (in most cases), but perhaps they should return a point-color tuple. Or provide separate GetData() and GetColor() functions. The point is to keep the indexing/striding implementation out of RendererLineStrip etc.

I'm curious how this should behave for PlotShaded and we defintely want to make it workf or PlotBars too.

Also, I think this would also need to honor offset as well as stride.

@niavok
Copy link
Author

niavok commented Mar 1, 2022

Thanks for the feedback.

I will wait with attention the new API.

@niavok
Copy link
Author

niavok commented Nov 14, 2022

Hello,
I updated my code for personal usage and with the new render primitive it was almost a full rewrite.
Now, it used internally a ColorGetter.
The new draw primitive allow one color per vertex so I added color interpolation.
I also added demo for shaded plots and stairstep.
image
image

Implementation and demo for lines, scatter, infinite lines, shaded plots and stairstep
@dnedry2
Copy link

dnedry2 commented Nov 30, 2022

I ran into this same issue with my own project. What I did was create additional PlotX functions that take a color array as well as data:

void PlotScatter(const char* label_id, const T* xs, const T* ys, ImU32* colormap, bool* mask, int count, int offset=0, int stride=sizeof(T));
void PlotLine(const char* label_id, const T* xs, const T* ys, ImU32* colormap, bool* mask, int count, int offset=0, int stride=sizeof(T));
void PlotShaded(const char* label_id, const T* xs, const T* ys, ImU32* colormap, bool* mask, int count, double y_ref=0, int offset=0, int stride=sizeof(T));

This way has the point color as part of ImPlotPoint and the existing getters fetch them. (and a bool mask, to filter out points)

Here's an example of scatter, lines, and shaded lines:
Screen Shot 2022-11-29 at 19 45 34
The two plots share a colormap, but the bottom is far more dense. I interpolate its colormaps myself before calling PlotLines.

Another consideration is the legend colors. I'm not sure how to decide that with per point color.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants