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

Simplified mesh intersection logic #221

Merged
merged 48 commits into from
Jul 17, 2024

Conversation

Makogan
Copy link
Contributor

@Makogan Makogan commented Jul 8, 2024

Change the mesh intersection code to rely on epsilon checks and point insertion to construct the final intersected mesh.

@sebcrozet sebcrozet marked this pull request as draft July 8, 2024 21:12
@Makogan Makogan changed the title WIP simplified mesh interseciton logic Simplified mesh intersection logic Jul 9, 2024
@Makogan Makogan marked this pull request as ready for review July 9, 2024 18:38
@Makogan
Copy link
Contributor Author

Makogan commented Jul 9, 2024

I know there are merge conflicts but I don;t know if we want to merge to master or to a different branch. So I await further instructions.

@sebcrozet sebcrozet self-requested a review July 10, 2024 17:30
Copy link
Member

@sebcrozet sebcrozet left a comment

Choose a reason for hiding this comment

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

Thank you for this PR! Here is a first round of review, mostly for coding style.

crates/parry3d-f64/Cargo.toml Outdated Show resolved Hide resolved
Comment on lines 438 to 470
// I heavily recommend that this is left here in case one needs to debug the above code.
fn _mesh_to_obj(mesh: &TriMesh, path: &PathBuf) {
let mut file = std::fs::File::create(path).unwrap();

ObjData {
position: mesh
.vertices()
.into_iter()
.map(|v| [v.x as f32, v.y as f32, v.z as f32])
.collect(),
objects: vec![Object {
groups: vec![Group {
polys: mesh
.indices()
.into_iter()
.map(|tri| {
SimplePolygon(vec![
IndexTuple(tri[0] as usize, None, None),
IndexTuple(tri[1] as usize, None, None),
IndexTuple(tri[2] as usize, None, None),
])
})
.collect(),
name: "".to_string(),
index: 0,
material: None,
}],
name: "".to_string(),
}],
..Default::default()
}
.write_to_buf(&mut file)
.unwrap();
Copy link
Member

Choose a reason for hiding this comment

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

This export is generally useful. Let’s have move it to an inherent methods TriMesh::to_obj into a new file src/transformation/mesh_obj.rs. That file should be feature-gated behind #[cfg(feature = "obj")].

In addition, instead of takin the path as argument, creating a file and calling write_to_buf, it should just return the ObjData.

You could additionally add a TriMesh::to_obj_file that creates and writes to the file. It will needs to return an Result instead of unwrapping.

Copy link
Contributor Author

@Makogan Makogan Jul 11, 2024

Choose a reason for hiding this comment

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

I am not fond of returning the ObjData.

The way the obj crate is structured has many flaws. I used it because it was the best I could find, but in a perfect world I would have preferred something that separates the geometry data and meta-data information instead of nesting structures into structures.

src/transformation/mesh_intersection/mesh_intersection.rs Outdated Show resolved Hide resolved
src/transformation/mesh_intersection/mesh_intersection.rs Outdated Show resolved Hide resolved
src/transformation/mesh_intersection/mesh_intersection.rs Outdated Show resolved Hide resolved
src/transformation/mesh_intersection/mesh_intersection.rs Outdated Show resolved Hide resolved
src/transformation/mesh_intersection/mesh_intersection.rs Outdated Show resolved Hide resolved
src/transformation/mesh_intersection/mesh_intersection.rs Outdated Show resolved Hide resolved
src/transformation/mesh_intersection/mesh_intersection.rs Outdated Show resolved Hide resolved
@sebcrozet
Copy link
Member

I know there are merge conflicts but I don;t know if we want to merge to master or to a different branch. So I await further instructions.

@Makogan Yes, we want to merge to master.

@Makogan
Copy link
Contributor Author

Makogan commented Jul 12, 2024

Merged to master and made sure the current code passes the CI checks

Copy link
Member

@sebcrozet sebcrozet left a comment

Choose a reason for hiding this comment

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

Thank you for this first round of changes.
I noticed that a few remarks went unnoticed. Don’t forget that github tends to collapse multiple conversations if there are many of them.

Two other remarks:

  • The obj test files should be moved to a assets/tests directory at the root of the repository.
  • Please add authoring & licence information as a comment in the .obj files.

crates/parry3d-f64/Cargo.toml Outdated Show resolved Hide resolved
src/shape/trimesh.rs Outdated Show resolved Hide resolved
src/transformation/wavefront.rs Outdated Show resolved Hide resolved
src/transformation/wavefront.rs Outdated Show resolved Hide resolved
src/transformation/mesh_intersection/mesh_intersection.rs Outdated Show resolved Hide resolved
Copy link
Member

@sebcrozet sebcrozet left a comment

Choose a reason for hiding this comment

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

Thank you for this second round of changes.
There are still a few ones left and a couple I haven’t seen the first times.

Regarding the obj files: please move them to assets/tests instead of test_data.

Comment on lines 1406 to 1429
/// Find the index of a vertex in a poly line, such that the two
/// edges incident in that vertex form the angle closest to 90
/// degrees in the poly line.
#[cfg(feature = "dim3")]
pub(crate) fn angle_closest_to_90(points: &[na::Vector3<Real>]) -> usize {
let n = points.len();

let mut best_cos = 2.0;
let mut selected_i = 0;
for i in 0..n {
let d1 = (points[i] - points[(i + 1) % n]).normalize();
let d2 = (points[(i + 2) % n] - points[(i + 1) % n]).normalize();

let cos = d1.dot(&d2);

let cos_abs = cos.abs();
if cos_abs < best_cos {
best_cos = cos_abs;
selected_i = i;
}
}

selected_i
}
Copy link
Member

Choose a reason for hiding this comment

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

I’d rather have it in triangle.rs and private since that’s the only place it’s used.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It is also used in the mesh intersection code inside of triangulate_constraints_and_merge_duplicates, line 407. That's why i had to move it there. So that I had access to it.

src/shape/triangle.rs Outdated Show resolved Hide resolved
src/shape/triangle.rs Outdated Show resolved Hide resolved
src/shape/triangle.rs Outdated Show resolved Hide resolved
src/transformation/mesh_intersection/mesh_intersection.rs Outdated Show resolved Hide resolved
src/transformation/mesh_intersection/mesh_intersection.rs Outdated Show resolved Hide resolved
src/transformation/mesh_intersection/mesh_intersection.rs Outdated Show resolved Hide resolved
src/transformation/mesh_intersection/mesh_intersection.rs Outdated Show resolved Hide resolved
src/transformation/mesh_intersection/mesh_intersection.rs Outdated Show resolved Hide resolved
src/transformation/mesh_intersection/mesh_intersection.rs Outdated Show resolved Hide resolved
Makogan and others added 23 commits July 16, 2024 12:16
Co-authored-by: Sébastien Crozet <sebastien@crozet.re>
Co-authored-by: Sébastien Crozet <sebastien@crozet.re>
Co-authored-by: Sébastien Crozet <sebastien@crozet.re>
Co-authored-by: Sébastien Crozet <sebastien@crozet.re>
Co-authored-by: Sébastien Crozet <sebastien@crozet.re>
Co-authored-by: Sébastien Crozet <sebastien@crozet.re>
Co-authored-by: Sébastien Crozet <sebastien@crozet.re>
Co-authored-by: Sébastien Crozet <sebastien@crozet.re>
Co-authored-by: Sébastien Crozet <sebastien@crozet.re>
Co-authored-by: Sébastien Crozet <sebastien@crozet.re>
Co-authored-by: Sébastien Crozet <sebastien@crozet.re>
Co-authored-by: Sébastien Crozet <sebastien@crozet.re>
Co-authored-by: Sébastien Crozet <sebastien@crozet.re>
Co-authored-by: Sébastien Crozet <sebastien@crozet.re>
@sebcrozet
Copy link
Member

sebcrozet commented Jul 17, 2024

Thank you, I took care of the last few details and updated the changelog.

@sebcrozet sebcrozet merged commit 4ffa010 into dimforge:master Jul 17, 2024
5 checks passed
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.

None yet

2 participants