Feat: Simplify PlotSurface API #135
Conversation
- The minor and the major parameters for the rest of the plot needs to be specified. These parameters include stride, offset and the boundary to use
- By default using `ImPlot3D::PlotSurface("Plot", z_values, num_x, num_y)` should work perfectly to produce a z-plot with just z_values
- Most of the parameters can be left as default but does allow for more control if desired
| @@ -37,6 +37,8 @@ | |||
| #include "imgui.h" | |||
| #ifndef IMGUI_DISABLE | |||
|
|
|||
| #include <limits.h> | |||
There was a problem hiding this comment.
Need this for INT_MAX used in IMPLOT3D_DEFAULT_MAJOR_STRIDE. Cannot use sizeof(T), -1(i/uint8) or 0 which are all potentially valid values.
I cannot think of a good value to make IMPLOT3D_DEFAULT_MAJOR_STRIDE other than INT_MAX. Maybe somebody has a better suggestion?
| @@ -453,6 +456,10 @@ IMPLOT3D_TMP void PlotQuad(const char* label_id, const T* xs, const T* ys, const | |||
| // to a predefined range | |||
| IMPLOT3D_TMP void PlotSurface(const char* label_id, const T* xs, const T* ys, const T* zs, int x_count, int y_count, double scale_min = 0.0, | |||
| double scale_max = 0.0, ImPlot3DSurfaceFlags flags = 0, int offset = 0, int stride = sizeof(T)); | |||
| IMPLOT3D_TMP void PlotSurface(const char* label_id, const T* values, int minor_count, int major_count, double scale_min = 0.0, double scale_max = 0.0, | |||
There was a problem hiding this comment.
Could maybe simplify this. Having all these parameters allows for a lot of control.
implot3d_demo.cpp
Outdated
| if (ImGui::Combo("Values Axis", &values_axis, "X-Axis\0Y-Axis\0Z-Axis\0")) { | ||
| // The major and value axis cannot be the same | ||
| if (major_axis == values_axis) { | ||
| major_axis = (major_axis + 1) % ImAxis3D_COUNT; |
There was a problem hiding this comment.
The Values-Axis and the Major-Axis cannot have the same value. Change them if they are the same.
implot3d_items.cpp
Outdated
| case 3: return ImPlot3DPoint(major_value, value, minor_value); // Y-Values + X-Major | ||
| case 2: return ImPlot3DPoint(value, minor_value, major_value); // X-Values + Z-Major | ||
| case 1: return ImPlot3DPoint(value, major_value, minor_value); // X-Values + Y-Major | ||
| case 8: // Z-Values + Z-Major. Not valid. Maybe assert here? |
There was a problem hiding this comment.
Should not support the value axis and major axis being the same type. There is a IM_ASSERT_USER_ERROR(values_axis != major_axis, "The values axis and major axis needs to be two different values"); in PlotSurface for this.
implot3d_items.cpp
Outdated
| @@ -917,6 +917,50 @@ template <typename T> struct IndexerIdx { | |||
| int Stride; | |||
| }; | |||
|
|
|||
| template <typename T> struct IndexerIdxMajorMinor { | |||
There was a problem hiding this comment.
Not sure what to call the indexer+getter. Ended up with this name but open to better suggestions?
Could maybe document how they work a bit better.
Added GetSurfaceValue function to help with determining the values for the surface to use and depending on what the data axis is set to will return different values.
implot3d_demo.cpp
Outdated
| const int updated_num_major = major_stride == 0 ? N : (N / abs(major_stride) + (N % major_stride == 0 ? 0 : 1)); | ||
|
|
||
| // Add an offset to the array if either the major or the minor stride is negative | ||
| const int array_offset = (major_stride < 0 ? (M * (N - 1)) : 0) + (minor_stride < 0 ? M : 1) - 1; |
There was a problem hiding this comment.
Need to add an offset to the array since we are now striding backwards. I added support for negative strides if needed. I don't know if it necessary?
implot3d_demo.cpp
Outdated
| @@ -873,6 +962,7 @@ void ShowAllDemos() { | |||
| DemoHeader("Triangle Plots", DemoTrianglePlots); | |||
| DemoHeader("Quad Plots", DemoQuadPlots); | |||
| DemoHeader("Surface Plots", DemoSurfacePlots); | |||
| DemoHeader("Simplified Surface Plots with Offset and Stride", DemoSimplifiedSurfacePlotsOffsetStride); | |||
There was a problem hiding this comment.
Not sure what to call this or where to put this. There is also nothing really simple about it.
Could maybe split it into two functions to show new API and then another for more complicated usage?
There was a problem hiding this comment.
I split this into two separate demos:
- To show the simplified surface plot API which just takes in Z-Values and plots the N*M surface plot
- Added a section similar to
ImPlot->Tools, where the stride/offset/axis flag combinations can be tested and added a demo that tests different combinations of values for these values
Did not know where to put the axis, offset and stride demo. I followed the example of ImPlot. Let me know if we should put it somewhere else?
|
|
||
| // TODO: I am pretty sure that the way that PlotSurfaceEx works is that it takes in the minor and the major count and thus it first iterates over | ||
| // the major then minor values. I need to confirm this first though | ||
| return PlotSurfaceEx(label_id, getter, minor_count, major_count, scale_min, scale_max, flags); |
There was a problem hiding this comment.
@brenocq: As far as I can tell PlotSurfaceEx does work in this way with major/minor being specified in this order since it just uses the getter and iterates over the major and then minor indexes.
I am hoping that we don't have to change anything in PlotSurfaceEx but let me know if you think I missed something?
There was a problem hiding this comment.
I did some further investigation and other than the Point.z in RendererSurfaceFill (added function Getter.GetSurfaceValue) I cannot see any specific things that depends on x, y or z values being in a certain order.
The function PlotSurfaceEx by default takes in x_count + y_count but this has always been minor and major since x has always been minor and y has been major. If the getter and indexer works correctly we can pass in the minor_count and major_count to this function. Could maybe rename the variables to minor_count and major_count instead of x_count and y_count since the new API can use different axis for different things?
It is very possible that I might have missed something.
Please @brenocq let me know if I missed anything?
…ing on what the data axis was set to - Don't know if we are keeping this. Might remove it depending on feeback
… surface plot and allows for even more control Default to `ImAxis3D_COUNT` which the plot will then use with `values_axis` but can change the value if desired
1. A simplified example where the x and y values do not have to be specified but only the z_values. 2. Simillar to ImPlot add a section `Tools` where we can show an example where the axis, stride and offset of the surface plot is changed
- The plot works with plots that use x-axis or y-axis information - As far as I can tell PlotSurfaceEx does not care what the values axis and minor/major values are as long as they are provided.
- If the value axis and the major axis is the same then do not have a case for it. - There are fewer case statements in the switch
…jor_count` since that matches how they are used in the new `PlotSurface` function. - Up to this point they have been used as x and y count but with the new function the major axis can be set
…inorRef` to apply to the major and minor when creating the `Getter` instead of determining it in the for loop each time
… minor values if they are already available. - Call and pass in values to the operator in the `Getter` function if the minor and major values are already known. - Default back to using the operator which does not take in the minor and major stride if it is not available.
There was a problem hiding this comment.
@brenocq I am quite happy with the implementation as it is. It does need some comments but I'll add them once I know whether I have gone in the correct direction.
When you get a chance please provide some feedback and let me know what you think
|
|
||
| // TODO: I am pretty sure that the way that PlotSurfaceEx works is that it takes in the minor and the major count and thus it first iterates over | ||
| // the major then minor values. I need to confirm this first though | ||
| return PlotSurfaceEx(label_id, getter, minor_count, major_count, scale_min, scale_max, flags); |
There was a problem hiding this comment.
I did some further investigation and other than the Point.z in RendererSurfaceFill (added function Getter.GetSurfaceValue) I cannot see any specific things that depends on x, y or z values being in a certain order.
The function PlotSurfaceEx by default takes in x_count + y_count but this has always been minor and major since x has always been minor and y has been major. If the getter and indexer works correctly we can pass in the minor_count and major_count to this function. Could maybe rename the variables to minor_count and major_count instead of x_count and y_count since the new API can use different axis for different things?
It is very possible that I might have missed something.
Please @brenocq let me know if I missed anything?
| p_plot[1] = Getter(x + 1 + y * XCount); | ||
| p_plot[2] = Getter(x + 1 + (y + 1) * XCount); | ||
| p_plot[3] = Getter(x + (y + 1) * XCount); | ||
| p_plot[0] = Getter(minor + major * MinorCount, minor, major); |
There was a problem hiding this comment.
Added the operator()(I idx, int minor, int major) function in the Getters for the GetterMinorMajor so that we don't have to determine the values again in the Getter with the operator()(I idx) function. Not sure if this is required but I thought it might be a nice little optimisation.
In the GetterXYZ the operator()(I idx, int minor, int major) just calls into the operator()(I idx) so it does not do anything.
implot3d_items.cpp
Outdated
| RendererSurfaceFill(const _Getter& getter, int x_count, int y_count, ImU32 col, double scale_min, double scale_max) | ||
| : RendererBase((x_count - 1) * (y_count - 1), 6, 4), Getter(getter), XCount(x_count), YCount(y_count), Col(col), ScaleMin(scale_min), | ||
| ScaleMax(scale_max) {} | ||
| RendererSurfaceFill(const _Getter& getter, int minor_count, int major_count, ImU32 col, double scale_min, double scale_max) |
There was a problem hiding this comment.
Change to using minor_count and major_count instead of x_count and y_count since it makes more sense in the context of the new function. Up to this point it has always been the case that x and y axis has been the minor and major axis but with the new function the user can specify the major axis and the minor axis(indirectly).
…et` to `MajorValueRef`, `MajorValueOffset`, `MinorValueRef` and `MinorValueOffset` - This is mainly distinguish it from the major and minor offset we add to the data pointer
brenocq
added
type:feat
|
It would be nice to have a simplified API with as few arguments as possible. Some people may get "scared" just by the number of arguments. IMPLOT3D_TMP void PlotSurface(const char* label_id, const T* values, int minor_count, int major_count, double scale_min = 0.0, double scale_max = 0.0, ImPlot3DSurfaceFlags flags = 0, int offset = 0, int stride = sizeof(T)))Note that the offset/stride is for the I like the idea of choosing custom major/minor/data axes! We could potentially add new flags to the
The flags above are a bit confusing, so we could also do the below:
Let me know what you think! These are just suggestions, nothing set in stone 😄 |
|
Hi @brenocq . Thank you for the constructive feedback. Just a couple of thoughts that I have based on your feedback:
I do agree that a simplified API would be better but I do think more control is sometimes better. Maybe we can rearrange the arguments so that the the arguments commonly used are at the front while arguments less used are at the back. void PlotSurface(const char* label_id, const T* values, int minor_count, int major_count, double scale_min = 0.0, double scale_max = 0.0, ImPlot3DSurfaceFlags flags = 0, const ImVec2& minor_bounds = ImVec2(-1, 1), const ImVec2& major_bounds = ImVec2(-1, 1), int offset = 0, int stride = sizeof(T), int major_offset = 0, int major_stride = IMPLOT3D_DEFAULT_MAJOR_STRIDE);Let me know what you think? I am open to better suggestions. |
- Rearange parameters for the `PlotSurface` function call and get rid of some of the parameters not needed. - Added flags to indicate what the plane axis is and use them accordingly - Fix demo to take into account some of the changes - Use data `stride` and `offset` instead of `minor stride` and `minor offset`. For the most part the `minor stride` is the same as `stride` but there is quite a big difference between `minor offset` and `offset`. - Added helper object `SurfacePlotPlaneGetter` which get the plane value from the 3D point based on which plane was set
|
@brenocq . I updated the API a bit keeping in mind some of the changes I previously mentioned. Let me know what you think? |
…comments - Depending to what the `ImPlot3DSurfaceFlags_Plane(XY/YZ/XZ)` flag is set to, the minor/major count values changes based on the flag set. Rename so that there is no potential confusion - Added some much needed comments for the new flags
- If neither the `ImPlot3DSurfaceFlags_PlaneXZ` and the `ImPlot3DSurfaceFlags_PlaneYZ` flag is specified then use `ImPlot3DSurfaceFlags_PlaneXY`.
Closes #110
Add what is supposed to be a "simplified" :o version of the
PlotSurfacewhich can take in the a N*M array and plot the data. The major and the minor parameters need to be specified instead of the x/y values so that we have a bit more control. Can also specify which axis is the data axis and the major axis. By default the data axis is Z-Axis and major axis is Y-Axis.There are a couple of extra minor and major parameters that can be used with the plot:
For simplicity the client can do
ImPlot3D::PlotSurface("Plot", z_values, num_x, num_y)which will plot thez_valueswith dimensionsnum_x*num_y.@brenocq : Let me know what you think. I hope you don't mind me looking at this issue. I saw this issue and was interested at taking a whack at it.
The function might be a bit overcomplicated but does allow for a lot of control. Could maybe simplify it?
Some things that still need to be fixed.
RendererSurfaceFill(Maybe otherRendererxx) is still very dependent onZ-Valuesand does not necessarily understand the concept of axis values that are not z-axis values. I might take a look at this so that the getter maybe potentially reports this to theRenderer?