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 SlicerVESPA extension #1925

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

Conversation

AlexyPellegrini
Copy link

@AlexyPellegrini AlexyPellegrini commented Mar 27, 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
    • N/A will be host on Gitlab
  • 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)
      • N/A will be host on Gitlab
    • 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)
      • N/A will be host on Gitlab

@pieper
Copy link
Member

pieper commented Mar 27, 2023

Does this extension build C-GAL? If so I understand that would make the extension GPL.

@CharlesGueunet
Copy link

CharlesGueunet commented Mar 28, 2023

Dear @pieper ,

The SlicerVESPA extension has its own license, under the BSD3 terms. The code of this extension is only under this license.
As this extension build against CGAL, the resulting executable is subject to both the BSD3 license of the extension AND the GPLv3 due to the CGAL library. So the resulting executable is indeed under the GPL, but not the extension per sei.

Also, if someone has the commercial license of CGAL (it is under dual licensing) and build the extension, the resulting executable may not be GPL any more as the terms of the commercial license will replace it).

@pieper
Copy link
Member

pieper commented Mar 28, 2023

Thanks for the clarification @CharlesGueunet . As a rule we don't rely on GPL code for Slicer application (e.g. see this discussion). Is there a way the GPL code could be build as executables that are independently downloaded?

@lassoan
Copy link
Contributor

lassoan commented Mar 28, 2023

If we only build CLI executables (ensured by the EXECUTABLE_ONLY option) then the GPL requirements are fulfilled. I don't think that installing the extension would restrict usage and distribution of Slicer.

We just need to clearly document in the license file, extension description, and readme file that GPL code is used internally because most companies would want to avoid any relationship with any copyleft-licensed code.

@lassoan
Copy link
Contributor

lassoan commented Mar 28, 2023

I see only one limitation with this, which may be very significant for medical applications: tivoization. I.e., you can only use the CGAL executables on the system if the user can replace them. But in a regulatory approved medical device it may not be feasible to give the user such low-level access.

So, the non-permissive license warnings should be very clear and visible, and we should work towards replacing CGAL with software that uses permissive license (Boolean operation -> vtkbool, IsotropicRemesher -> ACVD, AlphWrapping -> WrapSolidify using ACVD for remeshing; subidivision, filling, and smoothing tools in VTK are probably about as good as CGAL).

@CharlesGueunet
Copy link

If my understanding is correct, Vespa (and the related CGAL) will already be contained in a specific CLI with its specific libraries. Slicer will call them but will not be impacted by the GPL.

you can only use the CGAL executables on the system if the user can replace them.

I am not sure what you means here. Can you provide an example of what such a replacement would be ?

We just need to clearly document in the license file, extension description, and readme file that GPL code is used internally because most companies would want to avoid any relationship with any copyleft-licensed code.
...
So, the non-permissive license warnings should be very clear and visible

We can improve the documentation on VESPA and on the extension as you need. The readme of the VESPA project already states this on the first lines.

I think some of the questions remarks may be shorter to discuss in a quick meet (I will write a summary here after), @lassoan do you think we can schedule a 20 min meet ?

@lassoan
Copy link
Contributor

lassoan commented Mar 29, 2023

The issue is that gpl3 prohibits tivoization. In short: if the application is not linked to gpl3 code (so the user could replace/modify the gpl3 component) but the user has no access to the file system from outside then the gpl3 requirements are not fulfilled. For example, you cannot modify program files on a CT scanner, because the system boots directly into the application and you cannot exit the application or switch to another. Only the manufacturer's service engineers can modify program files on the system.

Can you create a doodle poll and post the link? It would be nice if @pieper could join the meeting, too.

@CharlesGueunet
Copy link

Here is a poll to find the best meeting time, everyone that would like to join can fill it:

https://www.when2meet.com/?19411506-Z8cWU

@lassoan
Copy link
Contributor

lassoan commented Mar 29, 2023

It seems that next Thursday would be the first day when all interested people could join a meeting. We could discuss the overall long-term strategy.

In the meantime we can probably continue the discussion of specific questions here. For example, I'll review and test the extension and give feedback or ask questions.

@lassoan
Copy link
Contributor

lassoan commented Mar 29, 2023

I've tested the extension on Windows. Build was successful and all modules worked vrery nicely. Even if somebody cannot use the modules due to licensing restrictions, it is useful to have them as a reference (e.g., see if vtkbool is as robust as CGAL's Boolean operations, compare speed and quality of ACVD remesher to CGAL, see if shrinkwrapping with vtkSmoothingFilter+ACVD is as good as Alpha wrapping, etc.).

A few questions/issues:

  • It seems that the extension is not supported on macOS (for example, the .so file filename is hardcoded). Has the build tested on macOS?
  • Add a note to the CMakeLists.txt that explains why CGAL-5.5.1-win64-auxiliary-libraries-gmp-mpfr.zip binaries (libmpfr-4.dll, libgmp-10.dll) are used instead of building from source. Building from source and static linking would be safer, because who knows what Python packages or other extensions bring in incompatible versions of these dlls.
  • Why ligmp, libmpfr symlinks were necessary?
  • It would be better to follow the superbuild template. It may mean that more targets are build than needed (unless you make it configurable what parts of CGAL are built), but that's much smaller issue than future maintenance issues (in 5 years you may not be around and somebody may need to update the extension to work with new CMake or VTK version or Python wrapper and then this extension may take 10x more effort to update, just because the first thing the maintainer has to do is to understand why this build structure is different from all other extensions and how differently it should be updated. If the SlicerVESPA build method is much better than the template then we need to update the template and most other extensions.
  • libgmp-10.dll and libmpfr-4.dll are installed into the module folder. This is a problem, because all DLLs in that folder are expected to be modules. These DLLs should be installed in the Slicer_THIRDPARTY_BIN_DIR. (See for example https://github.com/IGSIO/SlicerIGSIO/blob/master/SuperBuild/External_IGSIO.cmake)
  • For each module, in the CLI module XML file and in the README.md file, describe what the module is for (not everyone knows what "alpha wrapping" it is for in a few words and add a link to the algorithm (https://doc.cgal.org/latest/Alpha_wrap_3/index.html)

@AlexyPellegrini
Copy link
Author

AlexyPellegrini commented Mar 30, 2023

It seems that the extension is not supported on macOS (for example, the .so file filename is hardcoded). Has the build tested on macOS?

No, I have not tried to port it to Mac at all, but it shouldn't be hard to adapt windows specific code

Add a note to the CMakeLists.txt that explains why CGAL-5.5.1-win64-auxiliary-libraries-gmp-mpfr.zip binaries (libmpfr-4.dll, libgmp-10.dll) are used instead of building from source. Building from source and static linking would be safer, because who knows what Python packages or other extensions bring in incompatible versions of these dlls.

Building GMP and MPFR on Windows using MSVC is a nightmare, that why CGAL provides pre-built packages. The only real way to build them on Windows would be to rely on MSYS or Cygwin, but I don't want to constrain users to have them installed. I agree that a comment should be added to explain this.

Why ligmp, libmpfr symlinks were necessary?

Linker search for ligmp.so.6 and I didn't want to lose time for this so I just added symlinks. If you know a better, easier, solution, please let me know. It is probably possible to tweak the linker to search for the full name.

It would be better to follow the superbuild template.

I followed the template, at least I think I did. What is different ?

libgmp-10.dll and libmpfr-4.dll are installed into the module folder. This is a problem, because all DLLs in that folder are expected to be modules. These DLLs should be installed in the Slicer_THIRDPARTY_BIN_DIR.

I will fix that, thanks for the input.

For each module, in the CLI module XML file and in the README.md file, describe what the module is for (not everyone knows what "alpha wrapping" it is for in a few words and add a link to the algorithm

Yes, there is a lack of documentation in both the CLI module, but also VESPA.

@CharlesGueunet
Copy link

It seems that next Thursday would be the first day when all interested people could join a meeting. We could discuss the overall long-term strategy.

Here is a link to a google event including the google meet room, if everyone is ok with that technology (more convenient to setup on our side).

Also, concerning vtkbool, I can ensure it is not as robust as CGAL. Using exact computation, CGAL is able to give a clean and exact cut/merge even with intricate concave shapes. Maybe some other project may be considered ?

@pieper
Copy link
Member

pieper commented Mar 30, 2023

The Clipper2 option sounds worth exploring. If it works well enough, a clean new implementation would be way better than perpetuating a GPL'd codebase with nightmare build systems. I agree though that having something that works well enough for now is better than not having it, but providing clear explanations of the limitations will be important.

@jcfr
Copy link
Member

jcfr commented Apr 6, 2023

Last week I spent some time fixing up the build-system:

More work is needed, especially in the vtk-cgal/vespa project where the generation of the config files need to be improved. Ideally support for externally building VTK modules should be supported by VTK proper, in the meantime logic from VTKExternalModule1 may be re-used

Footnotes

  1. https://github.com/KitwareMedical/VTKExternalModule

@jcfr
Copy link
Member

jcfr commented Apr 6, 2023

the google meet room

I will not be able to join the meeting.

@jcfr jcfr changed the title Add s4ext for SlicerVESPA extension Add SlicerVESPA extension Aug 15, 2023
@jcfr jcfr added the Status: In Progress The review of this extension is in progress. label Aug 15, 2023
AlexyPellegrini and others added 2 commits May 2, 2024 03:27
Update scmurl, iconurl and screenshoturls to reference to the renamed
repository (`vtk-cgal/SlicerVESPA` -> `vtk/meshing/SlicerVESPA`)

Update description to list the provided features along with licensing caveats.
@jcfr
Copy link
Member

jcfr commented May 2, 2024

Waiting the build-system updates are finalized:

@jcfr
Copy link
Member

jcfr commented May 2, 2024

Unable to force push changes onto AlexyPellegrini/ExtensionsIndex@vespa-cli, the updated changes are published here main...jcfr:ExtensionsIndex:AlexyPellegrini-vespa-cli

Consider running the following command to update this pull request:

git clone git@github.com:AlexyPellegrini/ExtensionsIndex.git
cd ExtensionsIndex
git checkout vespa-cli
git remote add jcfr https://github.com/jcfr/ExtensionsIndex.git
git fetch jcfr
git reset --hard jcfr/AlexyPellegrini-vespa-cli
git push origin vespa-cli --force

@jcfr jcfr added Status: On Hold and removed Status: In Progress The review of this extension is in progress. labels May 2, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

Successfully merging this pull request may close these issues.

None yet

5 participants