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 DentureRegistration extension #1922

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

Conversation

gongmingjun
Copy link

@gongmingjun gongmingjun commented Mar 16, 2023

New extension

  • Extension has a reasonable name (not too general, not too narrow, suggests what the extension is for)
  • Repository name is Slicer+ExtensionName
  • Repository is associated with 3d-slicer-extension GitHub topic so that it is listed here. To edit topics, click the settings icon in the right side of "About" section header and enter 3d-slicer-extension in "Topics" and click "Save changes". To learn more about topics, read https://help.github.com/en/articles/about-topics
  • Extension description summarizes in 1-2 sentences what the extension is usable (should be understandable for non-experts)
  • Any known related patents must be mentioned in the extension description.
  • LICENSE.txt is present in the repository root. MIT (https://choosealicense.com/licenses/mit/) or Apache (https://choosealicense.com/licenses/apache-2.0/) license is recommended. If source code license is more restrictive for users than MIT, BSD, Apache, or 3D Slicer license then the name of the used license must be mentioned in the extension description.
  • Extension URL and revision (scmurl, scmrevision) is correct, consider using a branch name (main, release, ...) instead of a specific git has to avoid re-submitting pull request whenever the extension is updated
  • Extension icon URL is correct (do not use the icon's webpage but the raw data download URL that you get from the download button - it should look something like this: https://raw.githubusercontent.com/user/repo/main/SomeIcon.png)
  • 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.
    • Content of submitted s4ext file is consistent with the top-level CMakeLists.txt file in the repository (description, URLs, dependencies, etc. are the same)
  • Hide unused features in the repository to reduce noise/irrelevant information:
    • Click Settings and in repository settings uncheck Wiki, Projects, and Discussions (if they are currently not used)
    • Click the settings icon next to About in the top-right corner of the repository main page and uncheck Releases and Packages (if they are currently not used)

@pieper
Copy link
Member

pieper commented Mar 16, 2023

Thanks for sharing this. Please add a license file.

@lassoan
Copy link
Contributor

lassoan commented Mar 16, 2023

  • I could not figure out what exactly the extension does because there was no sample data set provided. You can create a release (from any git version) and upload the sample file (phantom or anonymized patient) as a release asset.
  • Do not duplicate code from other Slicer extensions (e.g., you copied over code from SlicerIGT extension FiducialRegistrationWizard\Logic\vtkPointMatcher.cxx). You can call any method of any logic classes of any other module in any other Slicer extension. See for example how SlicerIGT extension uses SlicerIGSIO extension modules.
  • Do not duplicate code from VTK (e.g., myMultiObjectMassProperties). If you need modifications in the class then you can change the class in VTK itself. Maybe start with a question on the VTK forum where you describe how you want to use the class and why the current VTK implementation seems to fall short. If there is an agreement that the VTK class needs improvement then you can make the change directly in VTK and we can get those changes in Slicer from there.
  • There are non-ASCII (character code >128) characters in the source code (e.g., in yunsuan.cxx) This may cause errors in sourc code management (the file might be interpreted as binary) and build errors (even when only used in comments)
  • FooBar is just a placeholder name. When you implement a real class then remove it (rename qSlicerRepairFooBarWidget.h to qSlicerRepairWidget.h)

It would have been trivial to implement this module in Python, while it was considerable effort to implement it in C++ (and will continue to remain much more work to maintain and develop it further in C++). I would recommend to consider switching to C++. You can put custom computation (e.g., yunsuan) into a C++ CLI module.

@gongmingjun
Copy link
Author

Here are some changes we made:

  1. We added a license.
  2. We provided a sample data (including the CT images of the patient and the denture).
  3. We have deleted the code duplicated from other Slicer extensions.
  4. We also deleted the code duplicated from VTK. However, you are suggested to build our extension with VTK 9.2, because we used a class in VTK named vtkMultiObjectMassProperties which might cause error in VTK 8.2.
  5. We deleted the non-ASCII characters in the source code.
  6. We renamed the class from qSlicerRepairFooBarWidget to qSlicerRepairWidget.
  7. We have implemented our module all in C++.
    We hope it will be satisfactory.

@pieper
Copy link
Member

pieper commented Mar 28, 2023

I think @lassoan probably meant to say that migrating the module to python would make the code more concise and maintainable. I know you have it working now in C++, but it is an interesting question of how much effort it would be to translate to python now vs how much effort it will save over the life of the extension.

@jcfr jcfr changed the title Add files via upload Add DentureRegistration extension Aug 15, 2023
SlicerDentureRegistration.s4ext Outdated Show resolved Hide resolved
SlicerDentureRegistration.s4ext Outdated Show resolved Hide resolved
@jcfr
Copy link
Member

jcfr commented Aug 16, 2023

We added a license.

Thanks for taking care of this, I posted some additional suggestion here:

@jcfr jcfr added the Status: Awaiting Response ⏳ Waiting for a response/more information label Aug 16, 2023
jcfr added 2 commits May 2, 2024 03:22
Update scmrevision, contributors, category and homepage

Add missing "description" key
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Status: Awaiting Response ⏳ Waiting for a response/more information
Development

Successfully merging this pull request may close these issues.

4 participants