Skip to content

Add volume and area calculations #12

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.

Sign up for GitHub

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

Open
casperlamboo wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Add volume and area calculations #12

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
61 changes: 51 additions & 10 deletions src/lib.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -168,6 +168,39 @@ impl IndexedMesh {
Ok(())
}
}

/// Calculates the volume of the mesh. In order for a correct volume calculation the mesh must
/// be valid. Use `validate` to make sure that the mesh doesn't contain any holes before
/// calculating the volume.
pub fn volume(&self) -> f32 {
Copy link
Owner

@hmeyer hmeyer May 20, 2021

Choose a reason for hiding this comment

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

I think this method should return an Optional[f32] or a Result[f32, SomeErrorType], and include a call to validate.
Furthermore, for this to be the true volume, I think we'd have to validate that the winding number of all triangles is the same, no?

Copy link
Contributor Author

@casperlamboo casperlamboo May 20, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

I think this method should return an Optional[f32] or a Result[f32, SomeErrorType], and include a call to validate.

I think I would switch it around; have the volume > 0 check in the validate function. but it is your call. (if you know your mesh to be valid it is annoying if a library spends computation on validation)

Furthermore, for this to be the true volume, I think we'd have to validate that the winding number of all triangles is the same, no?

I believe that the connected indices check in the validate function ensures that the winding order is the same for all tris.

Copy link
Owner

@hmeyer hmeyer May 20, 2021

Choose a reason for hiding this comment

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

I believe that the connected indices check in the validate function ensures that the winding order is the same for all tris.
Good point. I agree.

I think I would switch it around; have the volume > 0 check in the validate function. but it is your call. (if you know your > mesh to be valid it is annoying if a library spends computation on validation)
How about this:

  • rename validate() into is_closed_and_has_correct_winding().
  • call is_closed_and_has_correct_winding() inside volume().
  • create validate() as volume() > 0

Copy link
Contributor Author

@casperlamboo casperlamboo May 20, 2021

Choose a reason for hiding this comment

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

In my opinion it is bad practice to combine these kinds of validate checks inside "computation functions". It is better to execute such validation checks only once during or just after the construction of the data type. For instance coding a ray tracer; the direction of the ray always needs to be a unit length vector, but i'm not going to normalise the vector every time I compute something. Instead the normalisation is executed once when constructing the ray.

I would propose something like this.

impl IndexedMesh {
    ...
    pub fn validate(&self) -> Result<()> {
        self.is_closed()?;
        self.non_zero_tris()?;
        if self.volume() > 0.0 {
            Err(::std::io::Error::new(
                ::std::io::ErrorKind::InvalidData,
                "mesh has a negative volume",
            ))
        } else {
            Ok(())
        }
    }

    fn is_closed() -> Result<()> { ... }
    fn non_zero_tris() -> Result<()> { ... }
}

What you think about this? downside with my approach compared to yours is that if a user wants to know if a model is valid and want to know it's volume the volume is essentially computed twice.

Copy link
Owner

@hmeyer hmeyer May 21, 2021

Choose a reason for hiding this comment

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

I think it is questionable to assume the one winding is the correct and thus the other winding is invalid.
Meaning even a mesh that has negative winding might be valid.

The thing about volume is, that the calculation is only correct, if the mesh is closed and has correct winding.
But the above volume() function will also return a volume for meshes that are not closed or have no correct winding. In my opinion this would be a bug.
If you really wouldn't want to include the other validation checks in volume(), then volume would have to renamed to:
fn volume_assuming_the_mesh_is_closed_and_has_clockwise_winding()

// based on https://doi.org/10.1109/ICIP.2001.958278
let mut volume = 0.0;
for face in self.faces.iter() {
let a = self.vertices[face.vertices[0]];
let b = self.vertices[face.vertices[1]];
let c = self.vertices[face.vertices[2]];
volume += -c[0] * b[1] * a[2] +
b[0] * c[1] * a[2] +
c[0] * a[1] * b[2] -
a[0] * c[1] * b[2] -
b[0] * a[1] * c[2] +
a[0] * b[1] * c[2];
}
volume / 6.0
}


/// Calculates the surface area of the mesh.
pub fn area(&self) -> f32 {
let mut area = 0.0;
for face in self.faces.iter() {
let a = self.vertices[face.vertices[0]];
let b = self.vertices[face.vertices[1]];
let c = self.vertices[face.vertices[2]];
area += tri_area(a, b, c);
}
area
}
}

/// Write to std::io::Write as documented in
Expand Down Expand Up @@ -545,6 +578,7 @@ mod test {
use super::*;
const BUNNY_99: &[u8] = include_bytes!("testdata/bunny_99.stl");
const BUNNY_99_ASCII: &[u8] = include_bytes!("testdata/bunny_99_ascii.stl");
const BUNNY_VALID: &[u8] = include_bytes!("testdata/bunny_valid.stl");

// Will sort the vertices of the Mesh and fix the indices in the faces.
fn sort_vertices(mut mesh: super::IndexedMesh) -> super::IndexedMesh {
Expand Down Expand Up @@ -821,24 +855,31 @@ mod test {
}

#[test]
fn bunny_tri_area() {
fn bunny_area() {
let mut reader = ::std::io::Cursor::new(BUNNY_99);
let stl = BinaryStlReader::create_triangle_iterator(&mut reader)
.unwrap()
.as_indexed_triangles()
.unwrap();

let mut total_area = 0.0;
for face in stl.faces.iter() {
let a = stl.vertices[face.vertices[0]];
let b = stl.vertices[face.vertices[1]];
let c = stl.vertices[face.vertices[2]];
total_area = total_area + tri_area(a, b, c);
}

// area of bunny model according to blender
let blender_area: f32 = 0.04998364;

assert!(total_area.approx_eq(blender_area, F32Margin::default()));
assert!(blender_area.approx_eq(stl.area(), F32Margin::default()));
}

#[test]
fn bunny_volume() {
let mut reader = ::std::io::Cursor::new(BUNNY_VALID);
let stl = BinaryStlReader::create_triangle_iterator(&mut reader)
.unwrap()
.as_indexed_triangles()
.unwrap();

// volume of bunny model according to blender
let blender_volume: f32 = 690.9882461277;

assert!(blender_volume.approx_eq(stl.volume(), F32Margin::default()));
}

}
Binary file added src/testdata/bunny_valid.stl
View file
Open in desktop
Binary file not shown.