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

Fix wavefront tests + new test for different pos + add them in ci #268

Merged
merged 7 commits into from
Sep 20, 2024

Conversation

Vrixyz
Copy link
Contributor

@Vrixyz Vrixyz commented Sep 12, 2024

@Vrixyz Vrixyz requested a review from sebcrozet September 12, 2024 08:13
&cuboid_mesh,
false,
&Isometry::translation(2.0, 0.0, 0.0),
&sphere_mesh, //.clone().scaled(&Vector3::new(1.001, 1.001, 1.001)),
Copy link
Contributor Author

@Vrixyz Vrixyz Sep 12, 2024

Choose a reason for hiding this comment

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

That comment will have to be removed, but I added that because setting both positions to 200;0;0 resulted in a lot of holes in the resulting intersection. I can imagine this is floating point imprecision but it was surprising.

Copy link
Member

Choose a reason for hiding this comment

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

Interesting. I think it’s worth opening an issue with the example that generates holes.

Copy link
Contributor Author

@Vrixyz Vrixyz Sep 13, 2024

Choose a reason for hiding this comment

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

I opened #270 ; it can even be reproduced in a position of 2;0;0

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.

Thanks!

&cuboid_mesh,
false,
&Isometry::translation(2.0, 0.0, 0.0),
&sphere_mesh, //.clone().scaled(&Vector3::new(1.001, 1.001, 1.001)),
Copy link
Member

Choose a reason for hiding this comment

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

Interesting. I think it’s worth opening an issue with the example that generates holes.

Comment on lines +47 to 48
# Adds `TriMesh:to_obj_file` function.
wavefront = ["obj"]
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Adding documentation on cargo.toml is idiomatic to rust ecosystem (see serde or bevy)


[package.metadata.docs.rs]
rustdoc-args = ["-Zunstable-options", "--generate-link-to-definition"]
features = ["wavefront"]
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This allows to see the function in rustdoc.rs

@@ -88,3 +89,7 @@ thiserror = { version = "1", optional = true }
oorandom = "11"
ptree = "0.4.0"
rand = { version = "0.8" }

[package.metadata.docs.rs]
rustdoc-args = ["-Zunstable-options", "--generate-link-to-definition"]
Copy link
Contributor Author

Choose a reason for hiding this comment

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

this is already present in parry3d (we're in parry3d-f64 here)

@@ -40,4 +40,4 @@ mod to_trimesh;
pub mod utils;

#[cfg(feature = "wavefront")]
pub mod wavefront;
mod wavefront;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This module doesn´t have to be public, as it's implementing functions directly on TriMesh, which is public.

Making it public warns about undocumented module, as well as having nothing to show in it (the added function is within TriMesh)

@Vrixyz Vrixyz requested a review from sebcrozet September 16, 2024 15:28
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.

Thanks!

@Vrixyz Vrixyz merged commit 22a4261 into dimforge:master Sep 20, 2024
7 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.

2 participants