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

Import the algorithms present in imagej-ops #1

Merged
merged 38 commits into from
Sep 18, 2023
Merged

Import the algorithms present in imagej-ops #1

merged 38 commits into from
Sep 18, 2023

Conversation

tinevez
Copy link
Collaborator

@tinevez tinevez commented Jul 3, 2023

This break the separation between the data structure and the algorithms. But there are reasons why we want to do this:

  • facilitate development of data structures without breaking ops
  • offer to consumers one lib that has algos and data structures
  • offer consumers not to have to pull all imagej-ops if they just want to play with meshes
  • if there will be an imagej-ops2, then it can have default implementations that link to the algos in this lib, instead of copying the code from imagej-ops1.

Copy link
Member

@ctrueden ctrueden left a comment

Choose a reason for hiding this comment

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

@tinevez Thanks for all your hard work on migrating all this goodness into one place! 🥇

I made a variety of suggestions. But the big ones I think are:

  • The PR won't merge as is, needs a rebase first to fix conflicts.
  • The algorithms that produce a mesh should ideally be changed to pass the preconstructed output mesh as a parameter, rather than internally allocating a new BufferMesh and forcing that as the output type.
  • I have opinions about the package structure and class naming, but it can wait till after we merge (but before we release).

src/main/java/net/imglib2/mesh/alg/hull/ConvexHull.java Outdated Show resolved Hide resolved
final List< TriangularFacet > facets = new ArrayList<>();
final List< TriangularFacet > facetsWithPointInFront = new ArrayList<>();

epsilon = computeHull( vertices, facets, facetsWithPointInFront );
Copy link
Member

Choose a reason for hiding this comment

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

src/main/java/net/imglib2/mesh/Meshes.java Outdated Show resolved Hide resolved
src/main/java/net/imglib2/mesh/MeshShapeDescriptors.java Outdated Show resolved Hide resolved
src/main/java/net/imglib2/mesh/MeshShapeDescriptors.java Outdated Show resolved Hide resolved
pom.xml Outdated Show resolved Hide resolved
pom.xml Outdated Show resolved Hide resolved
src/test/java/net/imglib2/mesh/alg/StupidMesh.java Outdated Show resolved Hide resolved
@@ -375,4 +376,58 @@ public static void scale( final Mesh mesh, final double[] scale )
vertices.set( i, x * scale[ 0 ], y * scale[ 1 ], z * scale[ 2 ] );
}
}

public static BufferMesh merge( final Iterable< Mesh > meshes )
Copy link
Member

Choose a reason for hiding this comment

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

I would suggest to accept a Mesh out as argument, and concatenate them all into that, rather than forcing the result to be BufferMesh.

src/main/java/net/imglib2/mesh/Meshes.java Outdated Show resolved Hide resolved
Original author: Tim-Oliver Buchholz (University of Konstanz)
I removed the parts specific to imagej-ops infrastructure, and made
it a static method. I also imported and adapted the JUnit tests from
Tim.
From Tim-Oliver's code.
Now that I can compute stuff separately on MATLAB, I can add
my own tests as well.
The test fails in my case. And I could not understand how in Tim-O
calculations we go from the inertia matrix to the ellipsoid semi-
axes. BoneJ has another definition.
Both give different results from what is expected.
With the BVV and a spinner to control smoothing.
net.imglib2.mesh.obj → net.imglib2.mesh.imp
net.imglib2.mesh.obj.Mesh → net.imglib2.mesh.Mesh (also interfaces for triangles and vertices)
@tinevez
Copy link
Collaborator Author

tinevez commented Sep 13, 2023

thank you @ctrueden !
I implemented most of your suggestions.

I could not find a good way to change the design as you suggested, that is: have method that produce a new mesh accepts a pre-allocated Mesh object that they mutate. The main mesh implementation we use is the BufferMesh (e.g. in the BVV), which requires the number of vertices and triangles to be stored to be known in advance.

@tinevez
Copy link
Collaborator Author

tinevez commented Sep 13, 2023

Also: careful I git-push-forced.

@tinevez tinevez merged commit 111420f into master Sep 18, 2023
1 check passed
@tinevez tinevez deleted the import-ops branch September 18, 2023 15:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants