SlicerPhotogrammetry Experimental

[oothomas]

New extension

Tier 1

Any extension that is listed in the Extensions Catalog must fulfill these requirements.

• Repository name is Slicer+ExtensionName (except if the repository that hosts the extension can be also used without Slicer)
• 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 and the name of the license is mentioned in extension homepage. 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. MIT (https://choosealicense.com/licenses/mit/) or Apache (https://choosealicense.com/licenses/apache-2.0/) license is recommended. Read here to learn more about licenses. If source code license is more restrictive for users than MIT, BSD, Apache, or 3D Slicer license then describe the reason for the license choice and include the name of the used license in the extension description.
• Extension URL and revision (scmurl, scmrevision) is correct, consider using a branch name (main, release, ...) instead of a specific git hash 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
• Content of submitted json file is consistent with the top-level CMakeLists.txt file in the repository (dependencies, etc. are the same)
• 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
  • Publication: link to publication and/or to PubMed reference (if available)
• Hide unused github features (such as Wiki, Projects, and Discussions, Releases, Packages) 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)
• The extension is safe:
  • Does not include or download binaries from unreliable sources
  • Does not send any information anywhere without user consent (explicit opt-in is required)

[muratmaga]

can this be merged? @jcfr @lassoan @pieper

[jamesobutler]

• The installation of segment-anything is requiring a local git dependency that regular users might not have. This should be switched to install from the zip format of the source code pulled down like TotalSegmentator does here. https://github.com/SlicerMorph/SlicerPhotogrammetry/blob/8bbb4e7d847e0904e9d8c5955801f8bd5ff0e7de/Photogrammetry/Photogrammetry.py#L855
• There is further a dependency on git to clone a repo? https://github.com/SlicerMorph/SlicerPhotogrammetry/blob/8bbb4e7d847e0904e9d8c5955801f8bd5ff0e7de/Photogrammetry/Photogrammetry.py#L2320-L2327
• Is there a reason to force a specific older computation backend? If users have a newer Nvidia driver they could have compatibility with the CUDA 12.x versions of PyTorch. https://github.com/SlicerMorph/SlicerPhotogrammetry/blob/8bbb4e7d847e0904e9d8c5955801f8bd5ff0e7de/Photogrammetry/Photogrammetry.py#L823

https://github.com/SlicerMorph/SlicerPhotogrammetry?tab=readme-ov-file#running-locally:

Currently only mode of operation supported is to launch the OpenDroneMap server locally. This requires having docker (and optionally Nvidia Container toolkit if you are planning to use a Nvidia GPU) installed and that the user running Slicer is authorized to launch docker images.

Is this extension generally useful to the Slicer community to be installed by regular Slicer users on their systems? Or is this specific to MorphoCloud usage? If so, it would seem MorphoCloud should install the extension directly rather than going through the Slicer Extensions Index. Especially since this appears to contain only scripted loadable modules without a real need to actually compile anything as the factory build machines might otherwise provide. The extension could be loaded through additional module paths which can also be done by dropping the extension zip into the main window area to load.

[muratmaga]

• The installation of segment-anything is requiring a local git dependency that regular users might not have. This should be switched to install from the zip format of the source code pulled down like TotalSegmentator does here. https://github.com/SlicerMorph/SlicerPhotogrammetry/blob/8bbb4e7d847e0904e9d8c5955801f8bd5ff0e7de/Photogrammetry/Photogrammetry.py#L855

  • There is further a dependency on git to clone a repo? https://github.com/SlicerMorph/SlicerPhotogrammetry/blob/8bbb4e7d847e0904e9d8c5955801f8bd5ff0e7de/Photogrammetry/Photogrammetry.py#L2320-L2327

For both of these we can switch to unpacking the zip files. Thanks for the example. @oothomas can you please make the changes?

* Is there a reason to force a specific older computation backend? If users have a newer Nvidia driver they could have compatibility with the CUDA 12.x versions of PyTorch. https://github.com/SlicerMorph/SlicerPhotogrammetry/blob/8bbb4e7d847e0904e9d8c5955801f8bd5ff0e7de/Photogrammetry/Photogrammetry.py#L823

Yes, because with 12.X versions light the torch bring a very old version of the torchvision library that breaks SAM (we have reported this separately as an issue). In our experiments forcing version 11.8 did provide a recent version of the torchvision

https://github.com/SlicerMorph/SlicerPhotogrammetry?tab=readme-ov-file#running-locally:

Currently only mode of operation supported is to launch the OpenDroneMap server locally. This requires having docker (and optionally Nvidia Container toolkit if you are planning to use a Nvidia GPU) installed and that the user running Slicer is authorized to launch docker images.

Is this extension generally useful to the Slicer community to be installed by regular Slicer users on their systems? Or is this specific to MorphoCloud usage? If so, it would seem MorphoCloud should install the extension directly rather than going through the Slicer Extensions Index. Especially since this appears to contain only scripted loadable modules without a real need to actually compile anything as the factory build machines might otherwise provide. The extension could be loaded through additional module paths which can also be done by dropping the extension zip into the main window area to load.

Initially we want to deploy this on the Linux (and yes on the MorphoCloud), and then on Windows and Mac) I can't speak if the extension is generally useful for all the Slicer community, but it is quite useful and requested for SlicerMorph users, hence we want to have it in the extension catalogue.

[jamesobutler]

Initially we want to deploy this on the Linux (and yes on the MorphoCloud), and then on Windows and Mac)

The Slicer Extensions Index doesn't currently provide a way to specify that an extension be available for only one of the platforms. Since this is a scripted loadable module there won't be a build error on Windows or macOS that would otherwise prevent it on those platforms. You could potentially throw a error if it is building on Windows or macOS in the CMakeLists.txt of the extension https://github.com/SlicerMorph/SlicerPhotogrammetry/blob/master/CMakeLists.txt. This would help prevent the extension from showing up for regular Slicer users on these platforms. The extension isn't really "ready" for these users. This extension appears to be set up for easy use strictly through MorphoCloud where all the dependencies are already available.

[oothomas]

just checking to see if I understand the problem:

@muratmaga and @jamesobutler would adding the .zip files to the resource folder and doing the following solve the problem?

[jamesobutler]

No don’t upload the zipped source code to your repo. Install using the download zip link to latest code or a specific commit like TotalSegmentator does here.

- slicer.util.pip_install("git+https://github.com/facebookresearch/segment-anything.git")
+ slicer.util.pip_install("https://github.com/facebookresearch/segment-anything/archive/refs/heads/main.zip")

[oothomas]

I've made the requested changes:

[muratmaga]

@oothomas I confirm it works well for a fresh install. Can you add a logic to the CMakeList.txt so that the build will fail for non-Linux OS? That way extension will not show up on the Mac/Windows extension list until we are ready to deploy it on those platforms?

[oothomas]

I've added this to the CMAKE file:

However, I'm not sure if this is the right approach. I also don't know how to fix the current pre-commit issue:

[oothomas]

nevermind, I figured it out.

[muratmaga]

@oothomas please do not post screen captures, but copy/paste the lines or link the actual line number. Also if this is the change, I am not sure if the build names are correct. I don't think there is a win32 build of Slicer.

SlicerMorph/SlicerPhotogrammetry@5aa0531#diff-1e7de1ae2d059d21e1dd75d5812d5a34b0222cef273b7c3a2af62eb747f9d20a

And the way it is develop, wouldn't this simply display with the message and proceed with the rest of the cmake file?

[jamesobutler]

Win32 CMake variable is the correct one to use to determine if Windows. See existing Slicer usage here. I would suggest the CMake message usage use FATAL_ERROR (like this) to indicate the error instead of the STATUS usage used in the screenshot.

[oothomas]

Thanks @jamesobutler I made that change. Is the use of return() here okay? I assume this will end the make process and only coninue if UNIX.

if(APPLE OR WIN32)
  message(FATAL_ERROR "SlicerPhotogrammetry is only supported on Linux. Skipping build on this platform.")
  return()
endif()

[jamesobutler]

The FATAL_ERROR message will cause the configure to exit with a failure and not proceed to building so the return() is unnecessary.

[muratmaga]

@jamesobutler can this be merged?

[jcfr]

Thanks @jamesobutler for the review 🙏

[jcfr]

@oothomas When you have chance, I suggest you also address:

• Revisit use of GlobalWarningDisplayOff() SlicerMorph/SlicerPhotogrammetry#66

[jcfr]

@jamesobutler Since you have been reviewing this extension, I will let you move forward with its integration 🚀

[jamesobutler]

• Regarding the CMakeLists.txt logic to throw the error on configure, I would probably recommend putting that after the CMake minimum statement and project name and possibly after extension metadata fields.

• Have you thought about an extension icon? This is what users will see when they go to install it from the Extensions Manager. Nearly every existing extension has a unique icon and does not use the template provided extension icon.
  https://raw.githubusercontent.com/SlicerMorph/SlicerPhotogrammetry/master/SlicerPhotogrammetry.png

• A usual review provided by @lassoan is that the extension should be named without the "Slicer" prefix so that not all extensions start with that when browsing the extensions manager in the application. The GitHub repo should have the "Slicer" prefix though.
  https://github.com/SlicerMorph/SlicerPhotogrammetry/blob/6a8a51c688b0ad2cbfac0c07869e8adba655bc68/CMakeLists.txt#L6 should be updated to just "Photogrammetry" as the extension name and other logging/dialog messages to the user should indicate to install the "Photogrammetry" extension. Getting the name right before integration will be important so that it doesn't get uploaded into the Stable extension manager and served up as both names following a rename which would require manual deletion of the original name package.

• When entering Photogrammetry for the first time I observed it took a long time to enter. This is because it is automatically trying to install all the python package dependencies. Before doing this action I would suggest a dialog to the user asking to confirm whether they want to do this process now since it takes several minutes to complete (like this). It gives the user an out. Same goes for the "Recursive Spectral Clustering" module which has the same experience.

[oothomas]

"Have you thought about an extension icon? This is what users will see when they go to install it from the Extensions Manager. Nearly every existing extension has a unique icon and does not use the template provided extension icon."

I'll add icons for both modules asap. Can we proceed without it for testing purposes?

[muratmaga]

I'll add icons for both modules asap. Can we proceed without it for testing purposes?

We will have a proper icon for the Photogrammetry in next couple weeks (it is being designed), but for the time being we can use the generic one.

[oothomas]

I think all but the icon have been addressed. Just let me know if there are any other issues @jamesobutler and @muratmaga.

[muratmaga]

I think all but the icon have been addressed. Just let me know if there are any other issues @jamesobutler and @muratmaga.

I am not seeing any commits or the changes @jamesobutler requested to correct extension name. Please provide a commit link?

[oothomas]

Hi @jamesobutler and @jcfr

Here are more specifics regarding the changes you recommended.

"Regarding the CMakeLists.txt logic to throw the error on configure, I would probably recommend putting that after the CMake minimum statement and project name and possibly after extension metadata fields."
See: https://github.com/oothomas/SlicerPhotogrammetry/blob/master/CMakeLists.txt#L13

"A usual review provided by @lassoan is that the extension should be named without the "Slicer" prefix so that not all extensions start with that when browsing the extensions manager in the application. The GitHub repo should have the "Slicer" prefix though."
See: https://github.com/oothomas/SlicerPhotogrammetry/blob/master/Photogrammetry/CMakeLists.txt#L2
Would this prevent the issue you described from happening?

"When entering Photogrammetry for the first time I observed it took a long time to enter. This is because it is automatically trying to install all the python package dependencies. Before doing this action I would suggest a dialog to the user asking to confirm whether they want to do this process now since it takes several minutes to complete (like this). It gives the user an out. Same goes for the "Recursive Spectral Clustering" module which has the same experience."
See: https://github.com/oothomas/SlicerPhotogrammetry/blob/master/Photogrammetry/Photogrammetry.py#L824
The same was done for the Recursive Spectral Clustering module.

https://github.com/SlicerMorph/SlicerPhotogrammetry/blob/master/ClusterPhotos/ClusterPhotos.py#L21
I also corrected the module title. Thanks for pointing that out! 👍

"When you have chance, I suggest you also address:
SlicerMorph/SlicerPhotogrammetry#66"
See: https://github.com/oothomas/SlicerPhotogrammetry/blob/master/Photogrammetry/Photogrammetry.py#L76
I'll consider options for suppressing specific warnings as an alternative.

I'm actively refining the extension for the extension catalog, so I welcome any pointers and suggestions on how to improve it. Just let me know if there are any other issues delaying approval.