Replace MIRTK: Surface creation

[JostMigenda]

This is step 1 of the larger project to remove the dependency on MIRTK; an alternate implementation of CemrgCommandLine::ExecuteSurf that uses CemrgCommonUtils::ExtractSurfaceFromSegmentation() instead of MIRTK’s extract-surface and smooth-surface binaries.

This is a draft PR right now, because there are a number of to-do items:

• For now, there’s a boolean to easily switch between the old and new implementation during development and testing; that (plus the old implementation and its helper functions) needs to be deleted eventually
• It still uses the MIRTK close-image binary at the start
• Need to check that the semantics of the threshold/blur/smoothing parameters are identical and there aren’t any subtle differences between numerical values
• Optional: Check whether it makes sense to input and return pointers to an mitk::Image/mitk::Surface directly, instead of writing them to disk and passing around the file path. Updating the calling code may be more work, but this could reduce overhead quite a bit.

[github-actions]

⚡ Code Analysis Results ⚡

🔴 Cppcheck found 22 issues! Click here to see details.

CemrgApp/CemrgApp/Modules/CemrgAppModule/src/CemrgCommonUtils.cpp

Lines 1813 to 1818 in 5b4fb6d

	scalars = vtkFloatArray::SafeDownCast(surface->GetVtkPolyData()->GetCellData()->GetScalars());
	numObjects = surface->GetVtkPolyData()->GetNumberOfCells();
	} else{
	// surface->GetVtkPolyData()->GetCellData()->SetActiveScalars(fieldName.toStdString().c_str());
	scalars = vtkFloatArray::SafeDownCast(surface->GetVtkPolyData()->GetCellData()->GetScalars());
	numObjects = surface->GetVtkPolyData()->GetNumberOfCells();

!Line: 1813 - style: Redundant initialization for 'scalars'. The initialized value is overwritten before it is read. [redundantInitialization]

CemrgApp/CemrgApp/Modules/CemrgAppModule/src/CemrgCommonUtils.cpp

Lines 1806 to 1811 in 5b4fb6d

	vtkFloatArray *scalars = vtkFloatArray::New();
	vtkIdType numObjects;
	std::cout << "fieldName: " << fieldName.toStdString() << '\n';
	if (isElem){

!Line: 1806 - note: scalars is initialized

CemrgApp/CemrgApp/Modules/CemrgAppModule/src/CemrgCommonUtils.cpp

Lines 1813 to 1818 in 5b4fb6d

	scalars = vtkFloatArray::SafeDownCast(surface->GetVtkPolyData()->GetCellData()->GetScalars());
	numObjects = surface->GetVtkPolyData()->GetNumberOfCells();
	} else{
	// surface->GetVtkPolyData()->GetCellData()->SetActiveScalars(fieldName.toStdString().c_str());
	scalars = vtkFloatArray::SafeDownCast(surface->GetVtkPolyData()->GetCellData()->GetScalars());
	numObjects = surface->GetVtkPolyData()->GetNumberOfCells();

!Line: 1813 - note: scalars is overwritten

CemrgApp/CemrgApp/Plugins/kcl.cemrgapp.atrialfibres/src/internal/AtrialFibresClipperView.h

Lines 55 to 60 in 5b4fb6d

	class AtrialFibresClipperView : public QmitkAbstractView {
	// this is needed for all Qt objects that should have a Qt meta-object
	// (everything that derives from QObject and wants to have signal/slots)
	Q_OBJECT

!Line: 55 - style: The class 'AtrialFibresClipperView' does not have a constructor although it has private member variables. [noConstructor]

CemrgApp/CemrgApp/Plugins/kcl.cemrgapp.atrialfibres/src/internal/AtrialFibresView.h

Lines 66 to 71 in 5b4fb6d

	class AtrialFibresView : public QmitkAbstractView {
	// this is needed for all Qt objects that should have a Qt meta-object
	// (everything that derives from QObject and wants to have signal/slots)
	Q_OBJECT

!Line: 66 - style: The class 'AtrialFibresView' does not have a constructor although it has private member variables. [noConstructor]

CemrgApp/CemrgApp/Plugins/kcl.cemrgapp.atrialfibres/src/internal/AtrialFibresClipperView.cpp

Lines 451 to 456 in 5b4fb6d

	radii = vtkDoubleArray::SafeDownCast(line->GetPointData()->GetArray("MaximumInscribedSphereRadius"));
	double adjust = ctrPlane->GetRadius() / radii->GetValue(position);
	//Adjust highlighted clipper
	for (unsigned int i=0; i<clipperActors.size(); i++)
	clipperActors.at(i)->GetProperty()->SetOpacity(i==(unsigned)index ? 1.0 : 0.1);

!Line: 451 - style: Redundant initialization for 'radii'. The initialized value is overwritten before it is read. [redundantInitialization]

CemrgApp/CemrgApp/Plugins/kcl.cemrgapp.atrialfibres/src/internal/AtrialFibresClipperView.cpp

Lines 450 to 455 in 5b4fb6d

	vtkSmartPointer<vtkDoubleArray> radii = vtkSmartPointer<vtkDoubleArray>::New();
	radii = vtkDoubleArray::SafeDownCast(line->GetPointData()->GetArray("MaximumInscribedSphereRadius"));
	double adjust = ctrPlane->GetRadius() / radii->GetValue(position);
	//Adjust highlighted clipper
	for (unsigned int i=0; i<clipperActors.size(); i++)

!Line: 450 - note: radii is initialized

CemrgApp/CemrgApp/Plugins/kcl.cemrgapp.atrialfibres/src/internal/AtrialFibresClipperView.cpp

Lines 451 to 456 in 5b4fb6d

	radii = vtkDoubleArray::SafeDownCast(line->GetPointData()->GetArray("MaximumInscribedSphereRadius"));
	double adjust = ctrPlane->GetRadius() / radii->GetValue(position);
	//Adjust highlighted clipper
	for (unsigned int i=0; i<clipperActors.size(); i++)
	clipperActors.at(i)->GetProperty()->SetOpacity(i==(unsigned)index ? 1.0 : 0.1);

!Line: 451 - note: radii is overwritten

CemrgApp/CemrgApp/Plugins/kcl.cemrgapp.atrialfibres/src/internal/AtrialFibresLandmarksView.h

Lines 53 to 58 in 5b4fb6d

	class AtrialFibresLandmarksView : public QmitkAbstractView {
	// this is needed for all Qt objects that should have a Qt meta-object
	// (everything that derives from QObject and wants to have signal/slots)
	Q_OBJECT

!Line: 53 - style: The class 'AtrialFibresLandmarksView' does not have a constructor although it has private member variables. [noConstructor]

CemrgApp/CemrgApp/Plugins/kcl.cemrgapp.atrialfibres/src/internal/AtrialFibresLandmarksView.cpp

Lines 392 to 397 in 5b4fb6d

	double distance;
	int pickedSeedId = -1;
	double minDistance = 1E10;
	for (int i=0; i<pickedCellPointIds->GetNumberOfIds(); i++) {
	distance = vtkMath::Distance2BetweenPoints(
	pickPosition, surface->GetVtkPolyData()->GetPoint(pickedCellPointIds->GetId(i)));

!Line: 392 - style: The scope of the variable 'distance' can be reduced. [variableScope]

CemrgApp/CemrgApp/Plugins/kcl.cemrgapp.atrialfibres/src/internal/AtrialFibresLandmarksView.cpp

Lines 248 to 253 in 5b4fb6d

	std::ofstream fileRefined, fileRough, fileRefinedLabels;
	MITK_INFO << "[SaveRefinedPoints] Saving TXT file.";
	fileRefined.open((prodPath + outname + ".txt").toStdString());
	fileRefinedLabels.open((prodPath + outname + "-Labels.txt").toStdString());

!Line: 248 - style: Unused variable: fileRough [unusedVariable]

CemrgApp/CemrgApp/Plugins/kcl.cemrgapp.atrialfibres/src/internal/AtrialFibresVisualiseView.h

Lines 50 to 55 in 5b4fb6d

	class AtrialFibresVisualiseView : public QmitkAbstractView {
	// this is needed for all Qt objects that should have a Qt meta-object
	// (everything that derives from QObject and wants to have signal/slots)
	Q_OBJECT

!Line: 50 - style: The class 'AtrialFibresVisualiseView' does not have a constructor although it has private member variables. [noConstructor]

CemrgApp/CemrgApp/Plugins/kcl.cemrgapp.atrialfibres/src/internal/AtrialFibresView.cpp

Lines 1794 to 1799 in 5b4fb6d

	if(!userInputAccepted){
	QDialog* inputs = new QDialog(0, Qt::WindowFlags());
	m_UIEditLabels.setupUi(inputs);
	connect(m_UIEditLabels.buttonBox, SIGNAL(accepted()), inputs, SLOT(accept()));
	connect(m_UIEditLabels.buttonBox, SIGNAL(rejected()), inputs, SLOT(reject()));
	std::cout << "WHICH ATRIUM: " << uac_whichAtrium.toStdString() << '\n';

!Line: 1794 - style: Condition '!userInputAccepted' is always true [knownConditionTrueFalse]

CemrgApp/CemrgApp/Plugins/kcl.cemrgapp.atrialfibres/src/internal/AtrialFibresView.cpp

Lines 1792 to 1797 in 5b4fb6d

	bool userInputAccepted=false;
	if(!userInputAccepted){
	QDialog* inputs = new QDialog(0, Qt::WindowFlags());
	m_UIEditLabels.setupUi(inputs);
	connect(m_UIEditLabels.buttonBox, SIGNAL(accepted()), inputs, SLOT(accept()));

!Line: 1792 - note: Assignment 'userInputAccepted=false', assigned value is 0

CemrgApp/CemrgApp/Plugins/kcl.cemrgapp.atrialfibres/src/internal/AtrialFibresView.cpp

Lines 1794 to 1799 in 5b4fb6d

	if(!userInputAccepted){
	QDialog* inputs = new QDialog(0, Qt::WindowFlags());
	m_UIEditLabels.setupUi(inputs);
	connect(m_UIEditLabels.buttonBox, SIGNAL(accepted()), inputs, SLOT(accept()));
	connect(m_UIEditLabels.buttonBox, SIGNAL(rejected()), inputs, SLOT(reject()));
	std::cout << "WHICH ATRIUM: " << uac_whichAtrium.toStdString() << '\n';

!Line: 1794 - note: Condition '!userInputAccepted' is always true

CemrgApp/CemrgApp/Plugins/kcl.cemrgapp.atrialfibres/src/internal/AtrialFibresView.cpp

Lines 2336 to 2341 in 5b4fb6d

	segImage = atrium->LoadImage(segRegPath);
	resultString = segRegPath;
	tagName += "-reg";
	SetLgeAnalysis(true);

!Line: 2336 - style: Variable 'segImage' is assigned a value that is never used. [unreadVariable]

CemrgApp/CemrgApp/Plugins/kcl.cemrgapp.atrialfibres/src/internal/AtrialFibresView.cpp

Lines 192 to 197 in 5b4fb6d

	reply1 = Ask("Question", "Use alternative DICOM reader?");
	#endif
	if (reply1 == QMessageBox::Yes) {
	QString dicomFolder = QFileDialog::getExistingDirectory(NULL, "Open folder with DICOMs.", mitk::IOUtil::GetProgramPath().c_str(), QFileDialog::ShowDirsOnly|QFileDialog::DontUseNativeDialog);

!Line: 192 - style: Redundant initialization for 'reply1'. The initialized value is overwritten before it is read. [redundantInitialization]

CemrgApp/CemrgApp/Plugins/kcl.cemrgapp.atrialfibres/src/internal/AtrialFibresView.cpp

Lines 189 to 194 in 5b4fb6d

	int reply1 = QMessageBox::No;
	#if defined(__APPLE__)
	MITK_INFO << "Ask user about alternative DICOM reader";
	reply1 = Ask("Question", "Use alternative DICOM reader?");
	#endif

!Line: 189 - note: reply1 is initialized

CemrgApp/CemrgApp/Plugins/kcl.cemrgapp.atrialfibres/src/internal/AtrialFibresView.cpp

Lines 192 to 197 in 5b4fb6d

	reply1 = Ask("Question", "Use alternative DICOM reader?");
	#endif
	if (reply1 == QMessageBox::Yes) {
	QString dicomFolder = QFileDialog::getExistingDirectory(NULL, "Open folder with DICOMs.", mitk::IOUtil::GetProgramPath().c_str(), QFileDialog::ShowDirsOnly|QFileDialog::DontUseNativeDialog);

!Line: 192 - note: reply1 is overwritten

CemrgApp/CemrgApp/Plugins/kcl.cemrgapp.mmcwplugin/src/internal/MmcwView.cpp

Lines 721 to 726 in 5b4fb6d

	if (dcm_path_fix) {
	MITK_INFO << "[ATTENTION] Saving imgTimes.lst file to project directory.";
	time = directory + "/imgTimes.lst";
	file.open(time.toStdString(), std::ofstream::binary);
	file << "dcm- .nii\n";
	}

!Line: 721 - style: Condition 'dcm_path_fix' is always true [knownConditionTrueFalse]

CemrgApp/CemrgApp/Plugins/kcl.cemrgapp.mmcwplugin/src/internal/MmcwView.cpp

Lines 720 to 725 in 5b4fb6d

	bool dcm_path_fix = true;
	if (dcm_path_fix) {
	MITK_INFO << "[ATTENTION] Saving imgTimes.lst file to project directory.";
	time = directory + "/imgTimes.lst";
	file.open(time.toStdString(), std::ofstream::binary);
	file << "dcm- .nii\n";

!Line: 720 - note: Assignment 'dcm_path_fix=true', assigned value is 1

CemrgApp/CemrgApp/Plugins/kcl.cemrgapp.mmcwplugin/src/internal/MmcwView.cpp

Lines 721 to 726 in 5b4fb6d

	if (dcm_path_fix) {
	MITK_INFO << "[ATTENTION] Saving imgTimes.lst file to project directory.";
	time = directory + "/imgTimes.lst";
	file.open(time.toStdString(), std::ofstream::binary);
	file << "dcm- .nii\n";
	}

!Line: 721 - note: Condition 'dcm_path_fix' is always true

[JostMigenda]

Reverted this commit after discussing with @OrodRazeghi on Slack. Writing the mitk::Surface to disk between steps is useful, e.g. if clinicians want to look at the mesh and check its quality before moving on to the next steps.

[LouiseABowler]

This PR isn't quite ready for a full review as the surface generation parameters still need to be checked, but I have done a preliminary review to get the process started.

Overall, this is looking good 👍 I was able to run the example with no problems on Linux, and the changes to the code are clear.

I did spot one potential issue which @JostMigenda is going to pick up on. If invalid values are supplied for the threshold, smoothing or blur parameters, CemrgApp will crash as there isn't currently a way for it to gracefully exit or return an error message to the user. We've had a couple of messages about this and have identified two options for where to make these checks.

Once that's taken care of and the default parameters are finalised, I'll take another look and finish the review off.

[JostMigenda]

It looks like there are some subtle differences between the surfaces generated by MIRTK (on the development branch) and by CemrgCommonUtils::ExtractSurfaceFromSegmentation (our designated replacement). Note in particular the top left image in the attached screenshots, where the white line generated by MIRTK seems like a better fit.

• development branch
• features/replaceMIRTK branch (this PR)

I’m not sure whether that’s caused (at least in part) by the default parameters used for MIRTK being less well suited for the new implementation; so as discussed during previous meeetings, let’s wait for @alonsoJASL to experiment with that.