Skip to content

Overhaul of Voxelization ops with new algorithm. Also included test - #657

Open
andmccall wants to merge 14 commits into
imagej:masterfrom
andmccall:master
Open

Overhaul of Voxelization ops with new algorithm. Also included test#657
andmccall wants to merge 14 commits into
imagej:masterfrom
andmccall:master

Conversation

@andmccall

@andmccall andmccall commented Feb 13, 2026

Copy link
Copy Markdown

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:

mesh = ops.geom().marchingCubes(image);
result = ops.geom().voxelization(mesh, image);
ops.morphology().fillHoles(result, result, new DiamondShape(1));

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.

andmccall and others added 2 commits February 13, 2026 14:06
@andmccall

andmccall commented Feb 16, 2026

Copy link
Copy Markdown
Author

Just discovered a bug, working on fixing it and should have a new commit soon.
Edit: done. Sorry for the changes after the pull request, didn't notice these issues in initial testing.

andmccall and others added 5 commits February 16, 2026 11:21
Giving extra space on all sides when calculating the dimensions and offset seems to allow fillHoles to work more consistently after voxelization.
@andmccall
andmccall marked this pull request as draft February 16, 2026 20:07
…ed dimensions to make room for thicker walls
@andmccall
andmccall marked this pull request as ready for review February 17, 2026 18:54
@andmccall

Copy link
Copy Markdown
Author

@gselzer and @ctrueden,

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 gselzer left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

@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

Comment on lines 193 to 202
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)));

}

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

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

Suggested change
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);
}
}
}

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

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

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Correction: not DiamondShape(1), but Boundary.StructuringElement.FOUR_CONNECTED.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

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.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

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...

@andmccall andmccall Jul 7, 2026

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

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.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

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.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

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.

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?🤔

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

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.

Comment thread src/main/java/net/imagej/ops/geom/GeomNamespace.java Outdated
Comment on lines -281 to -287

@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;
}

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

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...

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

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.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

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 🙂

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

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;

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

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!

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

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.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

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!

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

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;

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

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)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

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?

@andmccall andmccall Feb 18, 2026

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

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

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

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 a computer.

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!

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

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?

@andmccall

Copy link
Copy Markdown
Author

@gselzer,
Tried to address all your points with the most recent commit, and I responded to each point individually above. Let me know what you think when you have the chance. Sorry it took so long, work has been quite busy. Finally getting some help here soon, so hopefully the SciJava Ops version won't take quite so long.

@gselzer gselzer left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

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

  1. Make your new Op be of type Ops.Geometric.Voxelization.class, but higher priority
  2. Add here any new signatures enabled by your new Op (for example, the Mesh -> RAI<BitType> version right below should stay)
  3. For all OpMethod signatures 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.

@andmccall andmccall Jul 6, 2026

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

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.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

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

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?

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Maybe just DefaultVoxelization3D_EucledianDistance.class?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

EuclideanDistanceVoxelization3D?

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Sure, I'll make the change, test and push.

@imagesc-bot

Copy link
Copy Markdown

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.
@andmccall

Copy link
Copy Markdown
Author

@gselzer,
Pushed the changes that we discussed, and fixed the createOutput bug. Let me know if you think of anything else.

@andmccall

Copy link
Copy Markdown
Author

I actually thought of a way to improve efficiency. Gonna try it real quick, shouldn't take long.

@andmccall

Copy link
Copy Markdown
Author

@gselzer,
Everything looks good to me and I think it's ready, unless you see something else? I'm gonna get started on the SciJava ops version.

@gselzer gselzer left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

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:
*

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Just to minimize changes, it looks like nothing meaningful changed in this file - can you undo the changes?

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

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 .

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

D'oh! Yes, please keep the @deprecated 😅

Comment on lines +52 to +53
* This is a voxelizer that produces a binary image with values set to true along
* the surface of the mesh.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

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)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

What do you think about adding a @see here referencing the old implementation, and noting the reason for the priority bump?

Comment on lines +65 to +66
@Parameter(type = ItemIO.INPUT, required = false)
private double wallThickness = 1.0;

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

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"

Comment on lines +198 to +205
/**
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());

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

No changes necessary, just wanted to say that I love this explanation 😍

@andmccall

Copy link
Copy Markdown
Author

@gselzer, Alright, I think I got all the changes made. Had to manually make the whitespace changes for DefaultVoxelization3D in GitHub's file editor, as IntelliJ really didn't like the whitespace layout in the original file. Let me know if the explanations for the new algorithm make sense.

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.

3 participants