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 QuickModelAlign extension #1941

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

Conversation

seanchoi0519
Copy link

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)

@jamesobutler
Copy link
Contributor

Does it make sense to include this Slicer module as part of SlicerMorph since it is based on the ALPACA module in SlicerMorph and therefore could benefit from the same maintenance activities? I worry about future maintenance of the code. For example there was some discussion about open3d usage being problematic at times and Murat indicating a switch to an ITK based ALPACA instead (see https://discourse.slicer.org/t/alpaca-open3d-error/29483/2). Additionally could this QuickAlign module directly use the ALPACA module in the SlicerMorph extension and apply customizations as needed? cc: @muratmaga, @smrolfe

@seanchoi0519
Copy link
Author

Does it make sense to include this Slicer module as part of SlicerMorph since it is based on the ALPACA module in SlicerMorph and therefore could benefit from the same maintenance activities? I worry about future maintenance of the code. For example there was some discussion about open3d usage being problematic at times and Murat indicating a switch to an ITK based ALPACA instead (see https://discourse.slicer.org/t/alpaca-open3d-error/29483/2). Additionally could this QuickAlign module directly use the ALPACA module in the SlicerMorph extension and apply customizations as needed? cc: @muratmaga, @smrolfe

Thanks for your reply. I am curious as to what you guys think.
That being said a lot of modifications have been made to make QuickAlign, I am not sure if we'll be able to achieve compatibility. For example, it does not use the '.ui' widget rather the UI is cutomized via code. The UI is also very simplistic and specific (does not allow for much customization) compared to ALPACA.
I did consider switching to ITK based ALPACA however considering the time I spent on modifying the code (many months) I decided that it may not be worth it at this time.

@smrolfe
Copy link
Contributor

smrolfe commented Jun 1, 2023

I would recommend at least calling the logic functions directly from ALPACA so that they don't need to be maintained separately. We are only supporting the open3D version temporarily due to the issues mentioned by @jamesobutler.

cc: @agporto

@agporto
Copy link

agporto commented Jun 1, 2023

I think there are two ways to go about it.

  • Option 1- If there is enough interest from SlicerMorph folks @smrolfe @muratmaga, it wouldn't be a bad idea to incorporate it into SlicerMorph. Sean's extension does something SlicerMorph users frequently ask for (the ability to register two 3D models and compare them using different types of visualization). However, some work would have to be done to integrate the tools to minimize redundancy. I am happy to help, but I would need helps from others as well. The main downside is that Sean might not want that integration, since his focus is on dentistry applications and he might want the UI to be specific to the task.
  • Option 2 - The other way to look at it is that it is a completely independent module that will grow into its own ecosystem. As I understand, Sean is regularly using the module in teaching activities and will need to maintain the code to be compatible at least with Slicer stables.

@muratmaga
Copy link

muratmaga commented Jun 1, 2023

I did consider switching to ITK based ALPACA however considering the time I spent on modifying the code (many months) I decided that it may not be worth it at this time.

In that case you will have to revise your code, particularly this section where you are using the wheels from our box server to install the open3d. https://github.com/seanchoi0519/SlicerQuickAlign/blob/main/QuickAlign.py#L76

I will remove these once the current ALPACA is deprecated and ITK-ALPACA becomes the default module in the next stable release. This will break your extension.

I highly encourage you to work with @agporto and @smrolfe to develop this into a full-fledged 3D model registration module. This can be a nice project week activity and I believe there is still time to list new ones.

@seanchoi0519
Copy link
Author

Thanks all for your responses.
I'm very happy with the idea of developing an independent full-fledged 3D model registration module. I think this could have potential for applications in many fields & opportunities for publications. This has been my goal for quite a while but I would require help.

@agporto @smrolfe would you guys be on board for this project? I can take charge of the UI and the visual display modes, however I would need help with the ITK backend integration. Let me know!

@seanchoi0519
Copy link
Author

I think there are two ways to go about it.
Option 1- If there is enough interest from SlicerMorph folks @smrolfe @muratmaga, it wouldn't be a bad idea to incorporate it into SlicerMorph. Sean's extension does something SlicerMorph users frequently ask for (the ability to register two 3D models and compare them using different types of visualization). However, some work would have to be done to integrate the tools to minimize redundancy. I am happy to help, but I would need helps from others as well. The main downside is that Sean might not want that integration, since his focus is on dentistry applications and he might want the UI to be specific to the task.
Option 2 - The other way to look at it is that it is a completely independent module that will grow into its own ecosystem. As I understand, Sean is regularly using the module in teaching activities and will need to maintain the code to be compatible at least with Slicer stables.

Thank you Arthur, that's correct. The end users in my mind are regular students & professors who may not have technical expertise. Hence I'd be looking to keep the user interface quite specific and simple. Yes, I do plan on regularly maintaining the code.

@muratmaga
Copy link

Hence I'd be looking to keep the user interface quite specific and simple

I think the goal would be to develeop a general purpose point cloud based registration tool, on which you can simply do a lightweight UI specific to your use case(s).

@seanchoi0519
Copy link
Author

Hi Murat
That's correct, and there are UI pictures I've uploaded on README file available to see what I meant by the UI.

@seanchoi0519
Copy link
Author

Hello @jamesobutler ,
A suggestion - I am happy to update the module with the SlicerMorph rigid registration pipeline (once it is done), however for now is it possible to have the extension available on the Slicer extension repository - compared to waiting for possibly many weeks for the update (which I'm very excited for) as I do have a network of dental institutions that could benefit from using this extension immediately. Let me know!

@jamesobutler jamesobutler requested a review from lassoan June 1, 2023 23:27
@muratmaga
Copy link

now is it possible to have the extension available on the Slicer extension repository

Are you aware of the Extension Wizard install method? That's you can provide a zip file of the repo to your users, which they can unzip and user the built in extension wizard to add to their slicer. It is essentially one more action (unzipping) compared to the extension catalog install. It might be good interim solution.

@seanchoi0519
Copy link
Author

now is it possible to have the extension available on the Slicer extension repository

Are you aware of the Extension Wizard install method? That's you can provide a zip file of the repo to your users, which they can unzip and user the built in extension wizard to add to their slicer. It is essentially one more action (unzipping) compared to the extension catalog install. It might be good interim solution.

Thanks Murat, I guess I can do that, but again I keep in mind my target users may lack technical expertise and my goal would be to keep the installation process as simple as possible.

Would it be a hassle to make it available on extension repository for now? Meanwhile I could work to update the code, perhaps something like quickalign_preview, similar to the ALPACA. Let me know your thoughts,

Sean

@muratmaga
Copy link

Would it be a hassle to make it available on extension

That's really not for me to say as i dont review these or make the decisions. All i can say @agporto and @chz31 are looking into building the general purpose mesh registration tool and probably join this PW (remotely)

@lassoan
Copy link
Contributor

lassoan commented Jun 5, 2023

Adding/renaming/removing extensions have significant long-term impact. If an extension appears in the extension catalog for a few months and it shows up in some youtube videos or forum posts then and then the extension is renamed or removed then for years we need to keep explaining users where that extension went, why, what to use instead.

Therefore, I would avoid adding a separate extension for mesh registration if it later will be folded into SlicerMorph. However, if the plan is to keep it as a separate extension then it is OK to create a separate extension for it now (and dependency with SlicerMorph can be sorted out later - extension dependencies are mostly internal implementation details).

One important change is needed in the extension name. In Slicer you can align many kind of data sets in many parameter spaces, so if the scope of the extension is spatial alignment of models then that must be included in the name. Something like QuickModelAlign, QuickAlignSurface, ModelQuickAlign, etc. would be sufficient.

@seanchoi0519
Copy link
Author

Thank you @lassoan
I will implement the extension name change, this is not a problem.
Whether this module will be integrated to 3DSlicer as a seperate module or folded into Slicermorph, I'm happy for @agporto to decide!

@agporto
Copy link

agporto commented Jun 19, 2023

Hi @seanchoi0519, you should do what you think is best. Given the focus of your module, my suggestion would be to keep it as a separate extension that will eventually use SlicerMorph's model registration module (for ease of maintenance).

@seanchoi0519
Copy link
Author

Thank you @agporto, I'm happy to move forward with that
What would be the next steps? Is everyone OK with this module being added as a seperate extension in the preview release? Then it will be eventually merged with SlicerMorph's model registration module?

@seanchoi0519
Copy link
Author

seanchoi0519 commented Jun 23, 2023

Hello everyone,

Following Arthur's advice, I do feel happy to publish the module as is. I will update the module once the rigid registration approach from SlicerMorph is published.

I've also amended the name to "QuickModelAlign." @lassoan
Please let me know if there's anything further to do on my end.

Many thanks to everyone!
@jamesobutler

@muratmaga
Copy link

Hello everyone,

Following Arthur's advice, I do feel happy to publish the module as is. I will update the module once the rigid registration approach from SlicerMorph is published.

I've also amended the name to "QuickModelAlign." @lassoan Please let me know if there's anything further to do on my end.

Many thanks to everyone! @jamesobutler

Sean, @chz31 is going to a demo of the separate registration module tomorrow to our monthly SlicerMorph user group. Feel free to join. It is on 11 (PT), here is the webex link. I think the module will be pushed to SlicerMorph by the end of the week.

https://seattlechildrens.webex.com/seattlechildrens/j.php?MTID=me662604e1ffd9c40f25888d048fcc0de

@seanchoi0519
Copy link
Author

Hello everyone,
Following Arthur's advice, I do feel happy to publish the module as is. I will update the module once the rigid registration approach from SlicerMorph is published.
I've also amended the name to "QuickModelAlign." @lassoan Please let me know if there's anything further to do on my end.
Many thanks to everyone! @jamesobutler

Sean, @chz31 is going to a demo of the separate registration module tomorrow to our monthly SlicerMorph user group. Feel free to join. It is on 11 (PT), here is the webex link. I think the module will be pushed to SlicerMorph by the end of the week.

https://seattlechildrens.webex.com/seattlechildrens/j.php?MTID=me662604e1ffd9c40f25888d048fcc0de

Sounds good, will be there!

@seanchoi0519
Copy link
Author

Hi @muratmaga @jamesobutler

Got to check out the "FastModelAlign" today (lol nice name), code looks great
I think it'd be great if we add the display & animation capabilities that my "QuickModelAlign" offers.
Imo it is one capability to register 2 models, but another to actually allow visual comparison of the output

I would either be looking to:

  1. collaborate with @chz31 to add this capability
  2. As mentioned before, I think the visualization aspect has big use cases in education field (I am partnering up with multiple universities at the moment in dental education), which is why I want to publish it as a seperate module.

Please advise

@muratmaga
Copy link

Imo it is one capability to register 2 models, but another to actually allow visual comparison of the output

I think we will keep the module strictly for registration. I agree visualizations are important but it will be different for everyone. So my suggestion, continue maintaining your extension in the way that will help you customize the visualizations whichever way you want, simply add SlicerMorph as a dependency for it.

This way you don't have to worry about maintaining the registration code, libraries, etc. The only requirement for you would be making sure that user did in fact run ITK-ALPACA (which brings the automatically necessary ITK python module). There is an example doing that programmatically that we can provide you.

@seanchoi0519
Copy link
Author

Imo it is one capability to register 2 models, but another to actually allow visual comparison of the output

I think we will keep the module strictly for registration. I agree visualizations are important but it will be different for everyone. So my suggestion, continue maintaining your extension in the way that will help you customize the visualizations whichever way you want, simply add SlicerMorph as a dependency for it.

This way you don't have to worry about maintaining the registration code, libraries, etc. The only requirement for you would be making sure that user did in fact run ITK-ALPACA (which brings the automatically necessary ITK python module). There is an example doing that programmatically that we can provide you.

Hello murat, thanks and I'd be happy to check out the example code.

@smrolfe
Copy link
Contributor

smrolfe commented Jun 28, 2023

You can use

slicer.util.selectModule(slicer.modules.alpaca)

to switch to the ALPACA module, which will initiate install of the dependencies. Then switch back to your module on completion.

@muratmaga
Copy link

@seanchoi0519 FastModelAlign module is now incorporated to SlicerMorph (as a testing module) and if all goes well, will be available with tomorrow's build.

@jcfr jcfr changed the title QuickAlign Pull Request Add QuickAlign extension Aug 15, 2023
@jcfr jcfr changed the title Add QuickAlign extension Add QuickModelAlign extension Aug 16, 2023
@jcfr
Copy link
Member

jcfr commented Aug 16, 2023

FastModelAlign module is now available in SlicerMorph ✔️

@seanchoi0519 Did you have a chance to work on the QuickModelAlign extension ?

@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:10
Update scmrevision from "master" to "main"

Update homepage specifying readme section

Update description to reference updated extension name

Update screenshoturls with list of URLs referenced in the readme file
@lassoan
Copy link
Contributor

lassoan commented Dec 3, 2024

We are cleaning out our backlog of open pull requests. We have introduced tiers for extensions, making it easier to add experimental extensions to the index. @seanchoi0519 if you want your extension to be added to the index then let us know and we can add it as a tier 1. If we don't hear from you then we will close the pull request (but you can reopen it anytime).

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.

7 participants