Overhaul of Voxelization ops with new algorithm. Also included test - #657
Overhaul of Voxelization ops with new algorithm. Also included test#657andmccall wants to merge 14 commits into
Conversation
Removed unused LogService parameter from DefaultVoxelization3D.
|
Just discovered a bug, working on fixing it and should have a new commit soon. |
Giving extra space on all sides when calculating the dimensions and offset seems to allow fillHoles to work more consistently after voxelization.
…ed dimensions to make room for thicker walls
|
Think I have all the kinks worked out and it's ready for review. I did find that there were a couple @OpMethods I put in the GeomNamespace that I had to comment out for the JUnit tests to pass (Input Mismatch error), even though the function calls worked fine when I compiled them without tests. Since it sounds like everything is gonna move to SciJava Ops, I figured it wasn't worthwhile for me to figure out the issue as I'm not very familiar with JUnit testing. Anyway, I'll for SciJava-ops and work on converting this to the SciJava-Ops format. |
gselzer
left a comment
There was a problem hiding this comment.
@andmccall thanks so much for doing this work! It's great to see contributions!
I really love your code - I think it's a massive improvement over the existing functionality. Just some things I was thinking about as I was looking through.
(Also, for those who aren't aware, there was a bit of discussion here
| public void voxelization3D() { | ||
| // https://github.com/imagej/imagej-ops/issues/422 | ||
| /*Value of 184 here corresponds with: | ||
| RandomAccessibleInterval<BitType> result = (RandomAccessibleInterval<BitType>) ops.run(DefaultVoxelization3D.class, mesh, ROI); | ||
| ops.morphology().fillHoles(result, result, new DiamondShape(1)); | ||
| assertEquals(Ops.Geometric.Voxelization.NAME, ROI.size(), Regions.countTrue(result)); | ||
| */ | ||
| assertEquals(Ops.Geometric.Voxelization.NAME, 184, | ||
| Regions.countTrue((RandomAccessibleInterval<BitType>) ops.run(DefaultVoxelization3D.class, mesh, ROI))); | ||
|
|
||
| } |
There was a problem hiding this comment.
This comment has absolutely nothing to do with your work, since it's clear you're just copying the precedent, but man, I'm remembering how much I disliked these tests 😆 ; if the goal is to assert proper voxelization, it is of course not sufficient to count the number of trues - you'll also need to know where they are.
I would love a test like this for the eventual SciJava Ops version, but up to you whether you think we should do something like this now (probably shouldn't go here though):
| public void voxelization3D() { | |
| // https://github.com/imagej/imagej-ops/issues/422 | |
| /*Value of 184 here corresponds with: | |
| RandomAccessibleInterval<BitType> result = (RandomAccessibleInterval<BitType>) ops.run(DefaultVoxelization3D.class, mesh, ROI); | |
| ops.morphology().fillHoles(result, result, new DiamondShape(1)); | |
| assertEquals(Ops.Geometric.Voxelization.NAME, ROI.size(), Regions.countTrue(result)); | |
| */ | |
| assertEquals(Ops.Geometric.Voxelization.NAME, 184, | |
| Regions.countTrue((RandomAccessibleInterval<BitType>) ops.run(DefaultVoxelization3D.class, mesh, ROI))); | |
| } | |
| public void voxelization3D() { | |
| // Create a "square" mesh from [1, 1, 1] to [2, 2, 1] | |
| Mesh m = new NaiveDoubleMesh(); | |
| m.vertices().add(1, 1, 1); | |
| m.vertices().add(1, 2, 1); | |
| m.vertices().add(2, 1, 1); | |
| m.vertices().add(2, 2, 1); | |
| m.triangles().add(0, 1, 2); | |
| m.triangles().add(2, 3, 1); | |
| // Voxelize it | |
| Interval interval = new FinalInterval(new long[] {0, 0, 0}, new long[] {3, 3, 3}); | |
| RandomAccessibleInterval<BitType> voxelized = (RandomAccessibleInterval<BitType>) ops.run(DefaultVoxelization3D.class, m, interval); | |
| // Check the results | |
| Cursor<BitType> cursor = voxelized.cursor(); | |
| while (cursor.hasNext()) { | |
| boolean value = cursor.next().get(); | |
| long[] pos = cursor.positionAsLongArray(); | |
| if (pos[0] < 1 || pos[0] > 2) { | |
| // x dimension outside the square | |
| assertFalse(value); | |
| } | |
| else if (pos[1] < 1 || pos[1] > 2) { | |
| // y dimension outside the square | |
| assertFalse(value); | |
| } | |
| else if (pos[2] != 1) { | |
| // z dimension outside the square | |
| assertFalse(value); | |
| } | |
| else { | |
| // "In" the square | |
| assertTrue(value); | |
| } | |
| } | |
| } | |
There was a problem hiding this comment.
Hi @gselzer,
Another option that I just discovered yesterday is that we can compare the result of voxelization(mesh, originalBinary) to the result of net.imglib2.roi.boundary.Boundary(originalBinary, new DiamondShape(1)). It turns out that this produces identical results for nearly all meshes. This would allow the continued use of the slightly more complex ROI image with the pre-calculated corresponding mesh that was present in ImageJ ops (if this carried over to SciJava Ops).
There was a problem hiding this comment.
Correction: not DiamondShape(1), but Boundary.StructuringElement.FOUR_CONNECTED.
There was a problem hiding this comment.
Tried to address this without having to create a mesh from scratch, using the net.imglib2.roi.boundary.Boundary(originalBinary,Boundary.StructuringElement.FOUR_CONNECTED) method I mentioned. Does show that the shape of the voxelized mesh after matches the expected shaped based on the boundary function.
There was a problem hiding this comment.
To be clear, what you're saying here is that the old Op DefaultVoxelization3D produces nearly equivalent results to net.imglib2.roi.boundary.Boundary with FOUR_CONNECTED?
If so, it's unfortunate that we have a whole implementation here. Might be good to add in a @see tag describing the similarities between the two...
There was a problem hiding this comment.
To be clear, what you're saying here is that the old Op DefaultVoxelization3D produces nearly equivalent results to net.imglib2.roi.boundary.Boundary with FOUR_CONNECTED?
No, not at all. My new implementation does. Effectively:
Binary image -> Marching Cubes -> New Voxelization algorithm -> Binary image of Mesh
and
Binary image -> Boundary with FOUR_CONNECTED -> Binary image of boundary
The "Binary image of Mesh" and "Binary image of boundary" are usually* the same, which they should be, as the mesh is generated from the boundaries of the binary image. Thus we can use this to test a good voxelization algorithm.
* single pixels, or single pixel width lines are not converted into Mesh by marching cubes, so these are missing in "Binary image of Mesh", but not "Binary image of boundary". Thus noisy binary images will have a difference.
There was a problem hiding this comment.
To add to this, the old implementation is broken, even beyond the issues I pointed out originally on the image.sc post. As I've been testing it for this update, it routinely fails to produce good binarized images of meshes that I test. They often have either huge gaps or massive blocks where there should be a nice smooth wall. I don't think that implementation was ever even finished, as there's large chunks of code in DefaultVoxelization3D.class that are entirely unused, at least as far as I could figure out.
If you're interested, I can share the groovy script and data I've been using to test and compare the two.
There was a problem hiding this comment.
Binary image -> Marching Cubes -> New Voxelization algorithm -> Binary image of Mesh
and
Binary image ->
BoundarywithFOUR_CONNECTED-> Binary image of boundaryThe "Binary image of Mesh" and "Binary image of boundary" are usually* the same, which they should be, as the mesh is generated from the boundaries of the binary image. Thus we can use this to test a good voxelization algorithm.
- single pixels, or single pixel width lines are not converted into Mesh by marching cubes, so these are missing in "Binary image of Mesh", but not "Binary image of boundary". Thus noisy binary images will have a difference.
Aha, I see. Thanks, it's a great explanation 🏆 No @see needed in that case, although if you wanted an explanatory comment in the test might be insightful for later readers.
To add to this, the old implementation is broken, even beyond the issues I pointed out originally on the image.sc post.
If you're interested, I can share the groovy script and data I've been using to test and compare the two.
Do you think you could share it via that forum thread?🤔
There was a problem hiding this comment.
Do you think you could share it via that forum thread?
Posted it, but just remembered that there was a bug in the createOutput() of my most recent commit, that makes it so ops.geom().voxelization(mesh) won't work right. I'll be pushing the next commit with the fix soon.
|
|
||
| @OpMethod(op = net.imagej.ops.geom.geom3d.DefaultVoxelization3D.class) | ||
| public RandomAccessibleInterval<BitType> voxelization(final Mesh in, final int width, final int height, final int depth ) { | ||
| final RandomAccessibleInterval<BitType> result = (RandomAccessibleInterval<BitType>) ops().run( | ||
| Voxelization.class, in, width, height, depth ); | ||
| return result; | ||
| } |
There was a problem hiding this comment.
This is a breaking change 🛠️
Since your Op has no overlap in parameter types with the existing Op, we could consider offering the two Ops side-by-side? Could also consider some sort of deprecation for the old Op...
There was a problem hiding this comment.
This would change the algorithm used, so I'm not sure you would want to do this, but you could do:
final RandomAccessibleInterval<BitType> result = (RandomAccessibleInterval<BitType>) ops().run( Voxelization.class, in, new FinalInterval(width, height, depth));
This means that the output from any old code would change, but the function call wouldn't.
I imagine that is not the preferred option, but it doesn't really matter to me, can definitely keep the old algorithm in there in whatever manner you deem best. I simply forgot about the whole "can't make breaking changes" thing, never done any library coding before.
There was a problem hiding this comment.
This would change the algorithm used, so I'm not sure you would want to do this, but you could do:
final RandomAccessibleInterval<BitType> result = (RandomAccessibleInterval<BitType>) ops().run( Voxelization.class, in, new FinalInterval(width, height, depth));This means that the output from any old code would change, but the function call wouldn't.
Yes, this option is certainly available, however I think I'd prefer avoiding breakages if possible. I wouldn't say that the previous implementation was a bug, it's just a different algorithm (although certainly a strange one)
I simply forgot about the whole "can't make breaking changes" thing, never done any library coding before.
It happens to the best of us, don't worry 🙂
There was a problem hiding this comment.
Separated out my new algorithm to defaultVoxelize3D class, instead of replacing defaulVoxelization3D. I did mark the old one as deprecated as well.
|
|
||
| @Parameter(type = ItemIO.INPUT, required = false) | ||
| private int height = 10; | ||
| private Interval dimensions; |
There was a problem hiding this comment.
Let's think about the tasks a user might need to accomplish using this Op. I can see the user wanting to:
- Obtain a voxelized image across the entire mesh bounding box
- Obtain a voxelized image across some custom interval (e.g. if the mesh is massive maybe they don't want to voxelize the whole thing).
Unless you can think of additional tasks, I think that this collection of goals would map nicely to a UnaryHybridCF Op, instead of a UnaryFunctionOp. This would allow users to call the Op with any preallocated output image defined on whatever interval they want, which would make the second goal easy. Additionally, if they called the Op as a Function, then you could use the logic you have for the bounding box to create a new image over the entire mesh interval.
What do you think about restructuring the Op to be a UnaryHybridCF? Do you think you could get rid of the Interval parameter that way?
(As an aside, in SciJava Ops hybrid Ops aren't a thing, we'd just write two separate Ops for that. But we'll cross that bridge later 😄)
There was a problem hiding this comment.
I would also wonder whether you could get rid of the scale and offset state within the Op if you did this. One key aspect of SciJava Ops is that Ops are stateless, so it'd be great to remove any state if possible if we want to eventually port this work over there!
There was a problem hiding this comment.
I'm not sure as to the difference between these 😅, or how it would work without the interval. I'll have to take a closer look, but will be quite busy until next week at the earliest. Just wanted to reply cause my work schedule tends to slingshot between totally free and a completely packed schedule, and I think it's swinging back to packed for a little while.
There was a problem hiding this comment.
Just wanted to reply cause my work schedule tends to slingshot between totally free and a completely packed schedule, and I think it's swinging back to packed for a little while.
I definitely understand, there is no rush!
There was a problem hiding this comment.
Had the time to go through and figure this out, and I agree, I think UnaryHybridCF is the way to go, so I switched the new algorithm over to that. I also got rid of scale and offset along with it as you suggested. Made the code much simpler.
|
|
||
| @Parameter(type = ItemIO.INPUT, required = false) | ||
| private int depth = 10; | ||
| private boolean scaleMeshToDimesions = false; |
There was a problem hiding this comment.
You said here:
Additionally, if the mesh wasn't previously an image the vertex coordinates in the mesh could be negative, which doesn't work very well with converting to images. Also, it just wasn't that hard to program.
If the goal is to describe the voxels a mesh passes through, setting this parameter to true would almost certainly obfuscate that information, since the scale and the offset are not reflected in the result.
What do you think about changing the type of the Op as I mentioned above? If that were the case, we would no longer have to worry about the scale and offset being necessary to transfer meaning between the mesh and the voxelized image...
|
|
||
|
|
||
| //region Obtain projection (p) of origP onto plane of triangle | ||
| // Find the normal to the plane: n = (b - a) x (c - a) |
There was a problem hiding this comment.
I really appreciate the comments here! This kind of context will ease future maintenance.
Would you be willing to add similar comments to the other functions as well?
There was a problem hiding this comment.
Absolutely. I love adding comments like this to math, as it takes me forever to wrap my head around this stuff and I get lost without the comments.
I did have a related question about SciJava, as I just want to clarify that I'm understanding everything correctly. It seems like a lot of the SciJava Ops are intentionally kept to one function, and these functions are chained together into a computer. So would the ideal be to break apart this Voxelization class into functions: ProjectionOntoPlane?, FindNearestPointInTriangle, GetDistanceToTriangle, and maybe others; then compile all these individual functions into a Voxelization computer? Or would it be better to do this as an Inplace? I'm not really sure if the default is to create more functions, or to create functions only if there's reason to believe they'd be useful to others (and I have no idea if they would be).
There was a problem hiding this comment.
I love adding comments like this to math, as it takes me forever to wrap my head around this stuff and I get lost without the comments.
Me too 😁
It seems like a lot of the SciJava Ops are intentionally kept to one
function, and these functions are chained together into acomputer.
There's definitely some terminology here. Functions, Computers, and Inplaces are all first-class algorithm types in SciJava Ops (and here in ImageJ Ops), but they all handle their outputs differently:
Functions create a new object every time.Computers fill up an output buffer given to them as a parameter.Inplaces overwrite one of their inputs with the output data (this is uncommon in our libraries but exists out in the wild).
So would the ideal be to break apart this Voxelization class into functions: ProjectionOntoPlane?, FindNearestPointInTriangle, GetDistanceToTriangle, and maybe others; then compile all these individual functions into a Voxelization computer?
I don't think it's worth making these functions into Ops right now - we could always do that later if there's a use for others!
There was a problem hiding this comment.
Tried to add comments to everywhere that I thought it might help in understanding. Though as I'm writing this I realize I didn't add comments to MeshFeaturesTest, though I didn't see many explanatory comments in the tests outside of "verified with MatLab", so not sure if I should add some there?
…D Op. Also created better Feature Test.
|
@gselzer, |
gselzer
left a comment
There was a problem hiding this comment.
Thanks for addressing the feedback. One more idea for you...
| } | ||
|
|
||
| @OpMethod(op = net.imagej.ops.geom.geom3d.DefaultVoxelize3D.class) | ||
| public RandomAccessibleInterval<BitType> voxelize(final Mesh in) { |
There was a problem hiding this comment.
One thing I would like to avoid is a profileration of Op names. In other words, an Op's name is supposed to reference it's underlying purpose. If we can't specify a theoretical difference between, in this case, voxelization and voxelize, then those two things should share the same Op name.
In a perfect world, I think we should (going off memory, no guarantee it works):
- Make your new Op be of type
Ops.Geometric.Voxelization.class, but higher priority - Add here any new signatures enabled by your new Op (for example, the
Mesh -> RAI<BitType>version right below should stay) - For all
OpMethodsignatures that apply to both Ops, change to:
- @Deprecated
- @OpMethod(op = net.imagej.ops.geom.geom3d.DefaultVoxelization3D.class)
- public RandomAccessibleInterval<BitType> voxelization(final Mesh in) {
- @SuppressWarnings("unchecked")
- final RandomAccessibleInterval<BitType> result =
- (RandomAccessibleInterval<BitType>) ops().run(net.imagej.ops.geom.geom3d.DefaultVoxelization3D.class, in);
- return result;
- }
+ @OpMethod(ops = {
+ net.imagej.ops.geom.geom3d.DefaultVoxelization3D.class,
+ net.imagej.ops.geom.geom3d.DefaultVoxelize3D.class
+ })
+ public RandomAccessibleInterval<BitType> voxelization(final Mesh in) {
+ @SuppressWarnings("unchecked")
+ final RandomAccessibleInterval<BitType> result =
+ (RandomAccessibleInterval<BitType>) ops().run(Ops.Geometric.Voxelization.class, in);
+ return result;
+ }
That should mean that ops.geom().voxelization(myMesh) should always match the new Op, but ops.geom().voxelization(myMesh, 5, 5) would match the old Op.
There was a problem hiding this comment.
Just wondering, would it be better to change the class name of the old Voxelization method to something like DeprecatedVoxelization3D, and make the new Voxelization method the DefaultVoxelization3D. Then update all the Op references to use the appropriate names? I know things are switching to SciJava ops, but I just don't want to leave ImageJ Ops messy.
There was a problem hiding this comment.
would it be better to change the class name of the old Voxelization method to something like
DeprecatedVoxelization3D, and make the new Voxelization method theDefaultVoxelization3D
I think that this would be another breaking change, right? For example, ops().run(net.imagej.ops.geom.geom3d.DefaultVoxelization3D.class, my_mesh) is public API returning a RAI<BitType>. If we did the name switch you're talking about, then a user who was calling this will now get a different result when they thought they were targeting a particular implementation by class name.
Maybe we can learn from this though - do you think your Op name can pertain to the particular algorithm you're using somehow? Such that if years later another person has a new algorithm, the naming conflict would be easier?
There was a problem hiding this comment.
Maybe just DefaultVoxelization3D_EucledianDistance.class?
There was a problem hiding this comment.
EuclideanDistanceVoxelization3D?
There was a problem hiding this comment.
Sure, I'll make the change, test and push.
|
This pull request has been mentioned on Image.sc Forum. There might be relevant details there: https://forum.image.sc/t/new-voxelization-algorithm-for-imagej-ops/119292/4 |
…d it as Ops.Geometric.Voxelization. Also, fixed createOutput to actually generate an appropriately sized output image. Lastly, added comments to test to make it clear why this test works.
|
@gselzer, |
|
I actually thought of a way to improve efficiency. Gonna try it real quick, shouldn't take long. |
…ing it for every pixel analyzed.
|
@gselzer, |
gselzer
left a comment
There was a problem hiding this comment.
Loving how this is turning out! Thanks again @andmccall
Just had a few more ideas but it should be a quick commit and then I think good to merge!
| * %% | ||
| * Redistribution and use in source and binary forms, with or without | ||
| * modification, are permitted provided that the following conditions are met: | ||
| * |
There was a problem hiding this comment.
Just to minimize changes, it looks like nothing meaningful changed in this file - can you undo the changes?
There was a problem hiding this comment.
Sorry, I'm not really sure when/how all these weird format changes came in (probably some auto-format from IntelliJ). I'll go and fix all the formatting changes, but we should keep the "@deprecated Use {@link EuclideanDistanceVoxelization3D} instead." yes? I also thought I had the whole plugin marked with a @Deprecated .
There was a problem hiding this comment.
D'oh! Yes, please keep the @deprecated 😅
| * This is a voxelizer that produces a binary image with values set to true along | ||
| * the surface of the mesh. |
There was a problem hiding this comment.
Any information that you can provide that encapsulates the Op's behavior (i.e. that it uses euclidean distance to populate the dataset) would be helpful for users!
| * the surface of the mesh. | ||
| * </p> | ||
| * | ||
| * @author Andrew McCall (University at Buffalo) |
There was a problem hiding this comment.
What do you think about adding a @see here referencing the old implementation, and noting the reason for the priority bump?
| @Parameter(type = ItemIO.INPUT, required = false) | ||
| private double wallThickness = 1.0; |
There was a problem hiding this comment.
Can you add a description to this parameter? I first assumed this was "distance from triangle plane has to be less than wallThickness", but this is actually "distance from triangle plane has to be less than wallThickness/2"
| /** | ||
| An ideal voxelization algorithm should be able to convert a {@link Mesh} generated from a binary image back into | ||
| a surface-pixel outline of the original binary image. This surface-pixel image should match the result of | ||
| processing the original binary image with {@link Boundary} using | ||
| {@link Boundary.StructuringElement.FOUR_CONNECTED}. When working with real images these can mismatch due to | ||
| isolated single-pixel objects in the original binary image not being incorporated into the {@link Mesh} | ||
| */ | ||
| final Img<BitType> out = new ArrayImgFactory<>(new BitType()).create(getTestImage3D()); |
There was a problem hiding this comment.
No changes necessary, just wanted to say that I love this explanation 😍
|
@gselzer, Alright, I think I got all the changes made. Had to manually make the whitespace changes for |
Wanted to address a major issue with the voxelization Op not working appropriately. The old algorithm was returning every pixel of every triangle's entire bounding box as true. This new method only sets the pixels that the surface goes through to true.
In general, this method should now be more symmetric with marching cubes, so that after:
Image and result should match in the majority of cases. The notable exception would be images with holes themselves, generating a surface encompassed entirely by a surface in marching cubes. Voxelization would generate both surfaces, but I believe all encompassed holes would be filled after the fillHoles call, though I haven't tested this yet.
I did add some comments in the code for some optional changes that could be made. Originally I had an additional parameter for the wall thickness, but wasn't sure if this was really beneficial. I also wasn't sure if I should be using another Op (fill holes) during the voxelization Op testing, so I left it out and just compared the result to a static number.
Edit: was just looking through and noticed I forgot to delete the LogService parameter, which I was using for troubleshooting an issue.