Add a function to easily create a concave polygon from a concave hull by robinmoussu · Pull Request #340 · dimforge/ncollide

robinmoussu · 2020-06-13T09:53:43Z

This is an early POC to show how I am creating a concave polygon from a concave hull.

Remarks regarding this draft:

I don't fully understand how the 2D/3D directory layout works, you can run the example I added with cargo run --example concave. cargo check works from build/ncollide2d, but not from the root directory.
I added gnuplot as a dev-dependency, but it's just there to display how the concave polygon is created, and what points are tested. If this patch is ever merged, I am not sure it should be kept. I didn't check if anything was already used to display graphics on the screen.
The example (concave.rs) serve as unit test. If this PR is merged it should be transformed into a proper unit test. I did it this way to be able to display the polygon, and show that it works.
It adds a dependency to the spade crate. You will decide if it's ok or not.
spade doesn't uses the same trait to abstract f32/f64 unfortunately this must be visible in the public API of new_concave_polygon. I am not happy about this.
This code use a few assumption about spade, I asked them if those were reliable. I am waiting the answer.

robinmoussu · 2020-06-13T10:03:40Z

src/shape/compound.rs

+                    let indexes = {
+                        let vertex_handle = face.as_triangle();
+                        let mut indexes = [0; 3];
+                        for i in 0..3 {
+                            indexes[i] = vertex_handle[i].fix();
+                        }
+                        indexes
+                    };


Note: this could be rewrote using the collect_tuple method of itertools:

let (a, b, c) = face .as_triangle() .iter() .map(|vertex_handle| vertex_handle.fix()) .collect_tuple() .unwrap();

But I don't know what is the best way to create an array by applying a function to another array.

I didn't know about collect_tuple but that looks like an elegant solution.

robinmoussu · 2020-06-13T10:04:27Z

src/shape/compound.rs

+    /// counter-clockwise convex polyline.
+    pub fn new_concave_polygon(isometry: Isometry<N>, hull: &[(N, N)]) -> Compound<N>
+    where
+        N: spade::SpadeFloat,


I don't like to have to leak that we internally uses spade here.

Unfortunately, I don't think we have a viable alternative here. I am not sure why the SpadeFloat trait does not have a blanket impl in the first place…

robinmoussu · 2020-06-13T11:53:49Z

src/shape/compound.rs

+
+    /// Creates a new 2D concave polygon from a set of points assumed to describe a
+    /// counter-clockwise convex polyline.
+    pub fn new_concave_polygon(isometry: Isometry<N>, hull: &[(N, N)]) -> Compound<N>


I don't know if new_concave_polygon should be added as a new constructor of Compound or if a new struct that implements the same traits as Compound should be created.

The issue with the current proposal is that the triangles used for the creation of the Compound shape are leaked, since it's possible to manipulate them with Compound::shapes() and then cast them with .as_shape::<ConvexPolygon<f64>>().

I think that it is OK to have this method here for now. I would prefer the following signature:

#[cfg(all(feature = "dim2", feature = "spade"))] pub fn from_convex_decomposition_of(position: Isometry<N>, vertices: &[Point2<N>]) -> Compound<N>

I think that ultimately we will want to use a TriMesh for this instead of a Compound. However, using a TriMesh is not possible right now because ncollide only supports it in 3D.

sebcrozet

Hi! Thank you for this PR!

sebcrozet · 2020-06-14T11:38:49Z

src/shape/compound.rs

+
+    /// Creates a new 2D concave polygon from a set of points assumed to describe a
+    /// counter-clockwise convex polyline.
+    pub fn new_concave_polygon(isometry: Isometry<N>, hull: &[(N, N)]) -> Compound<N>


I think that it is OK to have this method here for now. I would prefer the following signature:

#[cfg(all(feature = "dim2", feature = "spade"))] pub fn from_convex_decomposition_of(position: Isometry<N>, vertices: &[Point2<N>]) -> Compound<N>

I think that ultimately we will want to use a TriMesh for this instead of a Compound. However, using a TriMesh is not possible right now because ncollide only supports it in 3D.

sebcrozet · 2020-06-14T11:39:59Z

src/shape/compound.rs

+    /// counter-clockwise convex polyline.
+    pub fn new_concave_polygon(isometry: Isometry<N>, hull: &[(N, N)]) -> Compound<N>
+    where
+        N: spade::SpadeFloat,


Unfortunately, I don't think we have a viable alternative here. I am not sure why the SpadeFloat trait does not have a blanket impl in the first place…

sebcrozet · 2020-06-14T11:41:52Z

src/shape/compound.rs

+            delaunay
+                .triangles()
+                .filter_map(|face| {
+                    let indexes = {


Suggested change

let indexes = {

let indices = {

sebcrozet · 2020-06-14T11:43:39Z

src/shape/compound.rs

+                    let indexes = {
+                        let vertex_handle = face.as_triangle();
+                        let mut indexes = [0; 3];
+                        for i in 0..3 {
+                            indexes[i] = vertex_handle[i].fix();
+                        }
+                        indexes
+                    };


I didn't know about collect_tuple but that looks like an elegant solution.

sebcrozet · 2020-06-14T12:04:53Z

build/ncollide2d/Cargo.toml

 nalgebra        = "0.21"
 approx          = { version = "0.3", default-features = false }
 serde           = { version = "1.0", optional = true, features = ["derive"]}
+spade           = "1.8.2"


Suggested change

spade = "1.8.2"

spade = { version = "1", optional = true }

robinmoussu · 2020-06-16T08:32:37Z

I tested it a bit more, and unfortunately the triangulation approach I took doesn't works.

The hull is the black line, and the orange triangles are the one detected as being inside the hull, using the algorithm in this PR.

As you can see:

one triangle (ABC) is partially outside the hull
the area EDC isn't considered inside the hull.
there is no way to fill the hull using only triangles from the triangulation, since the triangle ADC wasn't generated (only ABC and ADC).

So, … I need to restart from scratch. I think we can close this PR unfortunately.

robinmoussu · 2020-06-17T09:46:36Z

I just noticed that there is maybe a way to solve the issue by using ConstrainedDelaunayTriangulation instead of FloatDelaunayTriangulation and adding all edges of the hull as constrains for the triangulation. I don't know if or when I will have to experiment it, so I'm letting this comment here in case someone (or my future self) is interested.

robinmoussu · 2020-06-17T09:46:36Z

I just noticed that there is maybe a way to solve the issue by using ConstrainedDelaunayTriangulation instead of FloatDelaunayTriangulation and adding all edges of the hull as constrains for the triangulation. I don't know if or when I will have to experiment it, so I'm letting this comment here in case someone (or my future self) is interested.

Add a function to easily create a concave polygon from a concave hull

a035683

robinmoussu commented Jun 13, 2020

View reviewed changes

sebcrozet requested changes Jun 14, 2020

View reviewed changes

Uh oh!

Conversation

robinmoussu commented Jun 13, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sebcrozet left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

robinmoussu commented Jun 16, 2020

Uh oh!

robinmoussu commented Jun 17, 2020

Uh oh!

robinmoussu commented Jun 17, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants