-
Notifications
You must be signed in to change notification settings - Fork 494
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 parallel coordinates chart #1082
base: master
Are you sure you want to change the base?
Add parallel coordinates chart #1082
Conversation
4b038b2
to
9fdf2f8
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I added a few nitpicks, but overall it looks good. I just miss a test (that could also serve as an example) of how to use the new chart type.
@@ -107,11 +107,11 @@ int ctkVTKScalarsToColorsViewTest2(int argc, char * argv [] ) | |||
ctkVTKScalarsToColorsView view(0); | |||
// add transfer function item | |||
vtkPlot* ctfPlot = view.addColorTransferFunction(ctf); | |||
view.chart()->SetPlotCorner(ctfPlot, 1); | |||
view.xyChart()->SetPlotCorner(ctfPlot, 1); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why is it called xyChart and not chartXY? If we follow VTK naming conventions then the method name should be chartXY.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was thinking that it is more natual to say, parallel coordinates chart, instead of chart parallel coordinates. Plus it matches the naming convention of abstractChart().
vtk in general follows the following naming pattern. vtk(Abstract)Base, vtkSpecialBase, e.g. vtkAlgorithm, vtkPolyDataAlgorithm. vtkCharts* are kinda the exception to the rule.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree that having xyChart
may feel more natural and is aligned with the convention adopted by Qt in the Qt Chart1 module.
That said, CTK provides a convenience wrapper around VTK, for sake of consistency with the VTK naming convention, I think we should then have the set/get functions and associated internal variables match the VTK class names.
ls -1 vtkCh*
vtkChartBox.cxx
vtkChartBox.h
vtkChart.cxx
vtkChart.h
vtkChartHistogram2D.cxx
vtkChartHistogram2D.h
vtkChartLegend.cxx
vtkChartLegend.h
vtkChartMatrix.cxx
vtkChartMatrix.h
vtkChartParallelCoordinates.cxx
vtkChartParallelCoordinates.h
vtkChartPie.cxx
vtkChartPie.h
vtkChartXY.cxx
vtkChartXY.h
vtkChartXYZ.cxx
vtkChartXYZ.h
Footnotes
switch (corner) | ||
{ | ||
// bottom left | ||
case 0: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Aren't there enums for 0, 1, 2, 3?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Parallel coordinates don't have plot corner function.
cb71d40
to
1cb4ffa
Compare
69fca16
to
5ea5ad3
Compare
|
||
// ---------------------------------------------------------------------------- | ||
vtkChartXY* ctkVTKChartView::chart()const | ||
{ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
{ | |
{ | |
qWarning() << "This function is deprecated. Use xyChart() or abstractChart() instead"; |
Q_ENUM(ChartTypes) | ||
|
||
/// Set/Get the type of the chart, XYChart is the default | ||
/// The string names as the same as the ChartTypes enum |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
/// The string names as the same as the ChartTypes enum |
Looks this description is not correct.
Once this last few comments have been addressed, this will be good to integrate. |
5ea5ad3
to
b2c412b
Compare
b2c412b
to
263d17f
Compare
Introduces abstractChart() and chartXY() functions. The chart() function has been converted to chartXY(). Therefore, chart() is deprecated. Co-authored-by: Andras Lasso <[email protected]> Co-authored-by: Jean-Christophe Fillion-Robin <[email protected]>
263d17f
to
0c80e70
Compare
This Pull request adds support for changing the chart type from vtkChartXY to vtkChartParallelCoordinates.
@lassoan @jcfr