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

Alternate more centerline derivatives #105

Merged
merged 4 commits into from
Jan 15, 2024

Conversation

chir-set
Copy link
Collaborator

This PR proposes one new module dealing with centerline models, and extends the functionalities of the 'BranchClipper' module.

CenterlineDisassembly: it breaks down a bifurcated centerline model into parts,
BranchClipper: highlight bifurcation splitting lines.

Incidentally,

  • the indentation of the list of modules is improved in the main README file,
  • BranchClipper is improved to prevent a crash when setting array names.

Please consider for inclusion in SlicerVMTK, as an alternative to #104 .

Regards.

@chir-set
Copy link
Collaborator Author

@lassoan @pieper @jcfr

Hi,

I'm considering merging this PR in preference to #104, in 'Rebase and merge' mode. I would prefer an approval from a Slicer main developer before committing, to avoid an unneeded reversal. Please advise.

Thanks and regards.

Copy link
Contributor

@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.

It looks good to me, thank you. I've added a few trivial comments inline.

# in batch mode, without a graphical user interface.
self.logic = CenterlineDisassemblyLogic()

self.ui.componentComboBox.addItem("Bifurcations", 1)
Copy link
Contributor

Choose a reason for hiding this comment

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

use constant instead of magic number (1, 2, 3)

self._parameterNode.disconnectGui(self._parameterNodeGuiTag)
self._parameterNodeGuiTag = None

def onSceneStartClose(self, caller, event) -> None:
Copy link
Contributor

Choose a reason for hiding this comment

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

Have you tried to use the new module template (in Slicer-5.6.1)?
The parameter node wrapper is a bit more opaque, but it would reduce the amount of boilerplate code you need to add and maintain.

# in batch mode, without a graphical user interface.
self.logic = CenterlineDisassemblyLogic()

self.ui.componentComboBox.addItem("Bifurcations", 1)
Copy link
Contributor

Choose a reason for hiding this comment

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

Make it translatable - _("Bifurcation")


### Disclaimer

Use at your own risks.
Copy link
Contributor

Choose a reason for hiding this comment

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

This is true for the entire 3D Slicer application and all modules in all extensions. If you really want then you can remind people in the extension's documentation. So, I would only add a warning for a specific module if there is something unusual about this module or you have somet more specific information/advice than just "use at your own risks".

@chir-set
Copy link
Collaborator Author

I'll handle all your suggestions in the coming days.

As for the new module template wizard, I suppose you are hinting at the older modules, like Cross-section analysis. I'll update them in the coming weeks as it will take more time. The new parameter node wrapper is indeed more friendly.

Thank your for your comments.

This module breaks down a bifurcated centerline model into parts.

Incidentally, improve the indentation of the main README file.
A crash would occur on a simple call while using vtkSetStringMacro.

Ex:
 l = slicer.modules.branchclipper.logic()
 l.SetCenterlineRadiusArrayName("r")

Using vtkSetMacro with std::string instead.
Branch profiles are created as models.

The module may create branch surfaces as segments and/or branch profiles.
@chir-set chir-set merged commit 16372f3 into vmtk:master Jan 15, 2024
@chir-set chir-set deleted the MoreCenterlineDerivatives_2 branch January 15, 2024 19:42
@chir-set
Copy link
Collaborator Author

Hi @lassoan,

#105 is merged and SlicerVMTK builds on cdash in all instances except Mac stable. The latter seems to be an already existing problem.

Thank you for your review.

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.

None yet

2 participants