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 parallel coordinates chart #1082

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

spyridon97
Copy link

@spyridon97 spyridon97 commented Apr 12, 2023

This Pull request adds support for changing the chart type from vtkChartXY to vtkChartParallelCoordinates.

@lassoan @jcfr

@spyridon97 spyridon97 force-pushed the add-parallel-coordinates-chart branch 3 times, most recently from 4b038b2 to 9fdf2f8 Compare April 12, 2023 20:07
Copy link
Member

@lassoan lassoan left a 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);
Copy link
Member

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.

Copy link
Author

@spyridon97 spyridon97 Apr 13, 2023

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.

Copy link
Member

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

  1. https://doc.qt.io/qt-5/qtcharts-module.html

Libs/Visualization/VTK/Widgets/ctkVTKChartView.cpp Outdated Show resolved Hide resolved
switch (corner)
{
// bottom left
case 0:
Copy link
Member

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?

Copy link
Author

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.

@spyridon97 spyridon97 force-pushed the add-parallel-coordinates-chart branch 5 times, most recently from cb71d40 to 1cb4ffa Compare April 17, 2023 21:28
@jcfr jcfr force-pushed the add-parallel-coordinates-chart branch 2 times, most recently from 69fca16 to 5ea5ad3 Compare July 12, 2023 04:40

// ----------------------------------------------------------------------------
vtkChartXY* ctkVTKChartView::chart()const
{
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
{
{
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
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
/// The string names as the same as the ChartTypes enum

Looks this description is not correct.

@jcfr
Copy link
Member

jcfr commented Jul 12, 2023

Once this last few comments have been addressed, this will be good to integrate.

@jcfr jcfr force-pushed the add-parallel-coordinates-chart branch from 5ea5ad3 to b2c412b Compare July 13, 2023 17:59
@jcfr jcfr force-pushed the add-parallel-coordinates-chart branch from b2c412b to 263d17f Compare October 27, 2023 13:08
spyridon97 and others added 3 commits October 27, 2023 09:12
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]>
@jcfr jcfr force-pushed the add-parallel-coordinates-chart branch from 263d17f to 0c80e70 Compare October 27, 2023 13:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

3 participants