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 SlicerCineTrack extension #2030

Merged
merged 7 commits into from
Jun 27, 2024
Merged

Add SlicerCineTrack extension #2030

merged 7 commits into from
Jun 27, 2024

Conversation

slicercinetrack
Copy link
Contributor

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 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
  • 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)

@lassoan
Copy link
Contributor

lassoan commented Apr 12, 2024

Thank you for your contribution!

Before we can review the extension in detail, you need to make sure that it is fully built by using the top-level CMakeLists.txt file. You provided a Visual Studio project file, but that is not included in the build and anyway it would only work on Windows. If you need the TrackingDataProcessor executable then you can very easily make it a Slicer CLI module that is automatically built on all platforms. You can use any of the CLI modules in Slicer core as an example.

@lassoan lassoan added the Status: Awaiting Response ⏳ Waiting for a response/more information label Apr 12, 2024
@slicercinetrack
Copy link
Contributor Author

Hi @lassoan
Thank you for the comment. We just want to release the Track module under the extension. Should we remove the code for TrackingDataProcessor?
Thank you,
Ha Phan.

@slicercinetrack
Copy link
Contributor Author

Hi @jcfr,
Just want to check if we're good to have this merge in or anything is still pending?
Thank you,
Slicer Track team.

@pieper
Copy link
Member

pieper commented May 10, 2024

I took a quick look and it seems nice, but it's not really clear what "Track" refers to - it would help to have a few screenshots or ideally an animated gif in the readme indicating exactly what sorts of things are being tracked.

@haphantran
Copy link
Contributor

Hi @pieper,
Thank you so much for your kind words.
We updated our readme as in laboratory-for-translational-medicine/SlicerCineTrack#151.
Thank you,
HaPhan.

@pieper
Copy link
Member

pieper commented May 14, 2024

Thanks @haphantran , the images and descriptions are much better now. One suggestion is that the name doesn't quite tell me what the extension does. Maybe SlicerCineTrack or Slicer4DTrack would make it easier for people to discover?

@haphantran
Copy link
Contributor

Hi @pieper,
Thank you for your suggestion. We changed our extension to SlicerCineTrack. We updated our Website, repo name, documentation to reflect this new name.
HaPhan.

@pieper
Copy link
Member

pieper commented May 15, 2024

I think this is looking good - just a lint issue https://github.com/Slicer/ExtensionsIndex/actions/runs/9089156174/job/24997747489?pr=2030

any more suggestions @lassoan ?

@slicercinetrack slicercinetrack changed the title Add SlicerTrack extension Add SlicerCineTrack extension May 15, 2024
@haphantran
Copy link
Contributor

HI team,
Would you please review and let us know if this is good to merge? Please let us know if we need to make any change.
Thank you,
HaPhan.

@pieper
Copy link
Member

pieper commented Jun 1, 2024

I'd be okay with merging and sorting out any remaining issues in the context of preview builds.

@ElodieLug
Copy link

I'd be okay with merging and sorting out any remaining issues in the context of preview builds.

Hello! Just checking in on the merge. Could we get this prioritized? Thanks in advance.

@pieper
Copy link
Member

pieper commented Jun 26, 2024

@jcfr @lassoan any comments?

"$schema": "https://raw.githubusercontent.com/Slicer/Slicer/main/Schemas/slicer-extension-catalog-entry-schema-v1.0.0.json#",
"build_dependencies": [],
"build_subdirectory": ".",
"category": "SlicerCineTrack",
Copy link
Contributor

@lassoan lassoan Jun 26, 2024

Choose a reason for hiding this comment

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

Category name cannot contain Slicer (all extensions are for Slicer). CineTrack as category name would be too specific (it is not useful to have an category that only contains a single extension). Sequences, 4D imaging or Time sequence or Registration could be better category names.

Suggested change
"category": "SlicerCineTrack",
"category": "Sequences",

@lassoan
Copy link
Contributor

lassoan commented Jun 26, 2024

Thank you for your patience. I've tested the extension build and it worked well. Then tried to test it but it was not clear what input data can be used (and there was no link to a sample data set). This is not a hard requirement for integration, but without it users have no chance of figuring out how to use the extension.

To be fixed before integration:

  • installPackages must not be called at startup. It would result in long delay of starting Slicer, especially when there is no network connection. Instead, xlrd and openpyxl should be installed when they first used.
  • Module category should be Sequences and/or Registration (not SlicerCineTrack)

Further recommendations:

  • Add sample test data and describe in the documentation how to get it (preferably via Sample Data module; you can upload sample data files as release asset in your github repository)
  • Use data in the scene as input instead of requiring selection of input folders (it is fine to also have a file import option, but you must allow users to use the data that they have already in the scene)

@haphantran
Copy link
Contributor

Hi @lassoan,
Thank you for reviewing and testing the extension. We fixed the 2 items for integration with this PR
With the items in future recommendations, we will check and work on it right after.
Thank you,
Ha Phan on behalf of SlicerCineTrack team.

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.

Thank you, it is good to go then.

@lassoan lassoan merged commit de17847 into Slicer:main Jun 27, 2024
3 checks passed
@ElodieLug
Copy link

Perfect, thank you very much!

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.

6 participants