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 new extension SlicerMeshing #1621

Closed
wants to merge 6 commits into from
Closed

Add new extension SlicerMeshing #1621

wants to merge 6 commits into from

Conversation

idhamari
Copy link
Contributor

@idhamari idhamari commented Mar 4, 2019

Thank you for contributing to 3D Slicer!

  • Update an existing extension with this pull request: Please delete all text in this template and just describe which extension is updated and optionally tell us in a sentence what has been changed. To make extension updates easier in the future (so that you don't have to submit a new pull request after each change in your extension), you may consider replacing specific git hash in your s4ext file by a branch name (for example: master or release for nightly version; (majorVersion).(minorVersion) such as 4.10 for stable Slicer version).
  • Add a new extension with this pull request: please only keep "Submitting a new extension" section and delete all other text of this template, then follow the instructions in the preserved section.

Submitting a new extension

Thank you very much for considering sharing your extension with the Slicer community.

To make sure users can find your extension, understand what it is intended for and how to use it, please complete the checklist below. You do not need to complete all the item by the time you submit the pull request, but most likely the changes will only be merged if all the tasks are done.

  • Extension has a reasonable name (not too general, not too narrow, suggests what the extension is for)
  • Repository name is Slicer+ExtensionName
  • Extension description summarizes in 1-2 sentences what the extension is usable (should be understandable for non-experts)
  • Extension URL and revision (scmurl, scmrevision) is correct, consider using a branch name (master, release, ...) instead of a specific git has to avoid re-submitting pull request whenever the extension is updated
  • Extension icon URL is correct
  • Screenshot URLs (screenshoturls) are correct, contains at least one
  • Homepage URL points to valid webpage containing the following:
    • Extension name
    • Short description: 1-2 sentences, which summarizes what the extension is usable for
    • At least one nice, informative image, that illustrates what the extension can do. It may be a screenshot.
    • Description of contained modules: at one sentence for each module
    • Tutorial: step-by-step description of at least the most typical use case, include a few screenshots, provide download links to sample input data set
    • Publication: link to publication and/or to PubMed reference (if available)
    • License: We suggest you use a permissive license that includes patent and contribution clauses. This will help protect developers and ensure the code remains freely available. We suggest you use the Slicer License or the Apache 2.0. Always mention in your README file the license you have chosen. If you choose a different license, explain why to the extension maintainers. Depending on the license we may not be able to host your work.
      Read here to learn more about licenses.

See more information about the submission process on the Slicer wiki: https://www.slicer.org/wiki/Documentation/Nightly/Developers/Tutorials/BuildTestPackageDistributeExtensions

If you have any questions or comments then please describe them here.

@jcfr
Copy link
Member

jcfr commented Mar 4, 2019

Instead of deleting the file, I suggest you run the command git rebase --interactive 68cd2e7^ and comment out the two commits update cervical spine and Delete SlicerCervicalSpine.s4ext and then force push.

@idhamari
Copy link
Contributor Author

idhamari commented Mar 4, 2019

done!

@fedorov
Copy link
Member

fedorov commented Mar 4, 2019

The proposed name of the extension is confusing, since it seems to imply the "ultimate" solution to meshing. There are already some other meshing tools in Slicer. From a quick look at the documentation web site (https://medicalimageanalysistutorials.github.io/SlicerMeshing), it is not clear what are the actual capabilities of the extension ("The Slicer Meshing extension is a GUI for two state of the arts meshing tools Instant Meshes and Robust Quad/Hex-dominant Meshes.")

Can you explain how the functionality of this module is different from other meshing extensions already in Slicer? Is "SlicerMeshing" really a good name (i.e., does this module provide meshing for a specific application, or for any domain/geometries? does it implement a specific meshing algorithm, or is it designed to be extensible to incorporate other algorithms? does it implement meshing using different elements (hex, tetrahedra)? is it 3d only or can accept geometries in 2d or dimensions higher than 3d?)

@idhamari
Copy link
Contributor Author

idhamari commented Mar 4, 2019

Thanks Andrey for the feedback.

The proposed name of the extension is confusing

It is a unique and a simple name but I don't mind change it, please suggest a name.

There are already some other meshing tools in Slicer,

I am aware of that. I think it is nice to add new algorithms this give users more options to use, also it is helpful for researchers to compare different methods.

Can you explain how the functionality of this module i

Please refer to the provided links, you can find more details in addition to the related papers.

@fedorov
Copy link
Member

fedorov commented Mar 4, 2019

It is a unique and a simple name but I don't mind change it, please suggest a name.

Indeed, it is a very nice name. I don't have a good idea for a more specific name, since I don't know the exact capabilities of the module.

I think it is nice to add new algorithms this give users more options to use, also it is helpful for researchers to compare different methods.

Completely agree.

Can you explain how the functionality of this module is different from other meshing extensions already in Slicer?
Please refer to the provided links, you can find more details in addition to the related papers.

Which links do you refer to? I looked at the documentation page here and didn't find answers: https://medicalimageanalysistutorials.github.io/SlicerMeshing/. I think it is fair to expect to understand the range of capabilities of the extension without having to read papers.

I am just trying to advocate for the user here. I don't have any other agenda, or much time to investigate this module in detail.

Please feel free to ignore this feedback.

@jcfr
Copy link
Member

jcfr commented Mar 4, 2019

@fedorov Thanks for your time reviewing the extension 🙏

@idhamari You be ideal to update the documentation to describe "Instant Meshes" and "Robust Quad/Hex-dominant Meshes" tools.

@idhamari
Copy link
Contributor Author

idhamari commented Mar 4, 2019

Which links do you refer to?

The links are embedded in the text

image

Here are the links again:

[1] https://github.com/wjakob/instant-meshes

[2] https://github.com/gaoxifeng/robust_hex_dominant_meshing

I am just trying to advocate for the user here.

Sorry about that. I am not an expert in meshes myself. One of my colleagues tried different tools and was not satisfied until he used these tools, hence the implementation. I think instant-meshes provides equally triangulation with many control options e.g. controlling number of triangles. The hexa-dominant-mesh provides volumetric meshings. Please feel free to try them and provide some feedback.

Please feel free to ignore this feedback.

All feedback and comments are very welcome!

@idhamari You be ideal to update the documentation to describe "Instant Meshes" and "Robust Quad/Hex-dominant Meshes" tools.

I just added a README.md file, hope this help. As I am providing only an interface. Interested people are welcome to contact the original authors of the tools for more deep technical questions.

@jcfr if you guys think this contribution is not important, please feel free to ignore it.

@lassoan
Copy link
Contributor

lassoan commented Mar 5, 2019

Thanks a lot for your contribution. SegmentMesher extension can create volumetric meshes from segmentations using tetgen and cleaver2 engines. It does not make sense to duplicate all the user interface and general module infrastructure for "Instant Meshes" and "Robust Quad/Hex-dominant Meshes" engines, but we should just add these two engines to the existing extension, since it would significantly reduce maintenance and further development efforts (that are typically much higher than the initial implementation effort). It would also make it harder for users to find and learn various volumetric meshing extensions if we had multiple extensions for the exact same task.

I can help you merging the two extensions. If you take on the commitment of maintaining the extension then I don't mind transferring the ownership of the merged SegmentMesher extension to you/your organization. Anything you prefer, really, I just want to avoid running parallel efforts and make things simple for users.

@idhamari
Copy link
Contributor Author

idhamari commented Mar 5, 2019

It does not make sense to duplicate all the user interface
but we should just add these two engines to the existing extension

@lassoan thanks for your comment. You are right. It was easier for me to do it this way as I am not familiar with CMake.

I can help you merging the two extensions.

This would be nice. Please feel free to add these engines t(and use all the related resources if needed) to SegmentMesher then cancel this pull request.

@fedorov
Copy link
Member

fedorov commented Mar 5, 2019

@idhamari I also noticed that you created forks of the original repositories under your github space, which you use in the superbuild of this extension:

Can you explain why you did that instead of proposing the CMake fixes to the upstream repositories? By forking off the repositories, it will not be possible to take advantage of the maintenance by the original creators of the code. Do you have good reasons for doing what you did?

@lassoan
Copy link
Contributor

lassoan commented Mar 5, 2019

I'll give it a try to add the two methods to the extension (I'm rebuilding Slicer now, but I should be able to work on this tomorrow).

@idhamari
Copy link
Contributor Author

idhamari commented Mar 5, 2019

I'll give it a try to add the two methods to the extension (I'm rebuilding Slicer now, but I should be able to work on this tomorrow).

Thanks. Take your time, it is not an urgent issue.

Can you explain why you did that instead of proposing the CMake fixes to the upstream repositories? By forking off the repositories, it will not be possible to take advantage of the maintenance by the original creators of the code. Do you have good reasons for doing what you did?

I made a few modifications in the CMake files which I thought the original author may not be interested e.g. there are some spaces in the binary name that produce errors. As I mentioned before, this was the easiest way to do it without bothering the original authors or waste time waiting for a reply.

@idhamari idhamari closed this Jun 15, 2019
@lassoan
Copy link
Contributor

lassoan commented Sep 17, 2020

Sorry, this has somehow fallen through the cracks. Let us know if you are still willing to contribute this to the extension manager. Thank you.

@idhamari
Copy link
Contributor Author

idhamari commented Sep 17, 2020

@lassoan

Sorry, this has somehow fallen through the cracks.

it OK.

Let us know if you are still willing to contribute this to the extension manager. Thank you.

Yes, please. I will reopen the PR.

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.

None yet

4 participants