Remove geom3D Ops in favor of imglib2-mesh usage#267
Conversation
gselzer
force-pushed
the
scijava-ops-image/port-mesh-ops
branch
3 times, most recently
from
9e1e68f to
c1074b5
Compare
|
Also of note - as shown here, many |
ctrueden
force-pushed
the
scijava-ops-image/port-mesh-ops
branch
2 times, most recently
from
1ada092 to
7d8cbb9
Compare
ctrueden
force-pushed
the
scijava-ops-image/port-mesh-ops
branch
from
7d8cbb9 to
397ebe2
Compare
gselzer
force-pushed
the
scijava-ops-image/port-mesh-ops
branch
from
397ebe2 to
c4bf27d
Compare
|
This PR has been rebased on top of #280, because while the direct opification of imglib2-mesh cannot happen without imglib v7, imglib v7 can happen without the opification of imglib2-mesh! |
| mesh = ops.op("geom.marchingCubes").input(mask).apply() | ||
| println("mesh = ${mesh} [${mesh.triangles().size()} triangles, ${mesh.vertices().size()} vertices]") | ||
|
|
||
| meshVolume = ops.op("geom.size").input(mesh).apply().getRealDouble() |
There was a problem hiding this comment.
@elevans @ctrueden let's debate whether this causes a breakage in backwards compatibility. On one hand, this line of code will no longer work unless we write a wrapper in this repo around it (and the whole point of this PR is to get rid of those), which would lead to it being a breaking change. This is pretty convincing for me, but playing devil's advocate, the apply() will return you an Object, and so you are violating the return type contract i.e. on paper we didn't do anything wrong.
I still lead towards this being a breaking change that requires a major version bump, though.
gselzer
force-pushed
the
scijava-ops-image/port-mesh-ops
branch
2 times, most recently
from
038090e to
ac123a1
Compare
As a temporary hack, we pin imglib2-mesh to 1.0.0 to avoid Op clashes, because imglib2-mesh 1.1.0 now integrates the geom Ops, resulting in Op matching clashes until #267 is merged.
gselzer
force-pushed
the
scijava-ops-image/port-mesh-ops
branch
from
9b693fa to
603226c
Compare
These two updates need to be done together, because imglib2-mesh 1.1.0, which adds the Ops manifest, is also updated to depend on imglib2 v7. Co-authored-by: Curtis Rueden <[email protected]>
It's a complicated method, so it's worthwhile to document the need and rationale
This required changes to avoid using ${project.version} all over the
place. Many of these version declarations, while not necessary right
now, will result in fewer changes down the line
3 participants
This PR removes all 3D geometry Ops, in favor of direct reliance upon the imglib2-mesh library.
The main question, now, is what happens to the tests. If they are kept here, they are vulnerable to action-at-a-distance errors. If they are migrated to imglib-mesh2, we will need (at minimum) a test-scope dependency on the SciJava Ops Engine, which feels fishy. @ctrueden @tpietzsch thoughts?
Still a draft PR because this work is directly tied into imglib/imglib2-mesh#10. That PR must be merged and a release of imglib2-mesh must be made before this PR is merged.