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

MITK upgrade #79

Merged
merged 31 commits into from
Jun 10, 2024
Merged

MITK upgrade #79

merged 31 commits into from
Jun 10, 2024

Conversation

JostMigenda
Copy link
Collaborator

This combines work from the previous MITK upgrade effort and @alonsoJASL’s fork … and then rebased all of it onto of the current development branch. This was a bit of a painful rebase—going forward, we really should avoid having branches diverge for multiple years!—but José and I completed it this afternoon.

Next up, I’ll try to build it on my machine and see what further changes are needed. Therefore, I’ll leave this as a draft PR for now.

Copy link

github-actions bot commented May 8, 2024

⚡ Code Analysis Results ⚡

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

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]

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

!Line: 1806 - note: scalars is initialized

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

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]

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]

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]

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

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

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]

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]

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]

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]

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]

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

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

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

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

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]

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

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

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: 723 - style: Condition 'dcm_path_fix' is always true [knownConditionTrueFalse]

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: 722 - note: Assignment 'dcm_path_fix=true', assigned value is 1

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: 723 - note: Condition 'dcm_path_fix' is always true


@alonsoJASL alonsoJASL marked this pull request as ready for review May 30, 2024 10:59
alonsoJASL
alonsoJASL previously approved these changes May 30, 2024
Copy link
Collaborator

@alonsoJASL alonsoJASL left a comment

Choose a reason for hiding this comment

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

Changes look OK.

LouiseABowler
LouiseABowler previously approved these changes Jun 7, 2024
Copy link
Collaborator

@LouiseABowler LouiseABowler left a comment

Choose a reason for hiding this comment

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

This is looking really good @JostMigenda! 🚀

I've left a couple of comments but go ahead and merge this when you're ready, those comments won't need any further review.

I have also given this PR a go on my Linux machine today. Mixed results there - it builds fine, but the applcation won't open. I suspect any fixes will inolve the setup instructions rather than the code, do don't let that block this PR 👍

.github/workflows/build.yml Outdated Show resolved Hide resolved
.github/workflows/test.yml Outdated Show resolved Hide resolved
CemrgApp/CMakeExternals/VMTK.cmake Outdated Show resolved Hide resolved
CemrgApp/CMakeExternals/VMTK.cmake Outdated Show resolved Hide resolved
Co-authored-by: Louise Bowler <LouiseABowler@users.noreply.github.com>
Copy link
Collaborator

@LouiseABowler LouiseABowler left a comment

Choose a reason for hiding this comment

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

Thanks for the revisions @JostMigenda, this looks good to me 🙌

@JostMigenda
Copy link
Collaborator Author

Test’s are failing during the “Download precompiled Build folder” step; no logs for that step is available, but the error message for the overall workflow run says:
System.IO.IOException: No space left on device
Is it possible that the downloaded build folder (plus the CemrgApp repo itself) exceeds the 14 GB disk space limit on GitHub’s runners?

Anyway, since the tests already fail on the head of the development branch, this shouldn’t stop us from merging this PR.

@JostMigenda JostMigenda merged commit 75afd96 into development Jun 10, 2024
1 of 3 checks passed
@JostMigenda JostMigenda deleted the feature/mitk-upgrade branch June 14, 2024 13:18
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.

5 participants