Allow 3d triangulation #1691

drewish · 2016-12-23T05:32:59Z

I needed to do some basic 3d triangulation and noticed that libtess2 supports it but the Triangulator wrapper did not. It wasn't too hard to hook it up but the mesh output is wonky. I've added some really basic unit tests to demonstrate what it's doing.

drewish · 2016-12-23T05:33:45Z

include/cinder/TriMesh.h

@@ -200,7 +200,7 @@ class TriMesh : public geom::Source {
 	size_t		getNumIndices() const override { return mIndices.size(); }
 	//! Returns the total number of triangles contained by the TriMesh.
 	size_t		getNumTriangles() const { return mIndices.size() / 3; }
-	//! Returns the total number of indices contained by the TriMesh.
+	//! Returns the total number of verticies contained by the TriMesh.


I was referring to these docs and this seemed like a bit of copy paste from a couple lines above.

drewish · 2016-12-23T05:34:27Z

include/cinder/Triangulate.h

 	//! Performs the tesselation, returning a TriMesh2d
-	TriMeshRef	createMesh( Winding winding = WINDING_ODD );
+	TriMeshRef	createMesh( Winding winding = WINDING_ODD, u_int8_t positionDims = 2 );


I'm not a fan of this interface but couldn't think of anything better.

drewish · 2016-12-23T05:35:22Z

src/cinder/Triangulate.cpp

+	if( positionDims == 2 ) {
+		result.appendPositions( (vec2*)tessGetVertices( mTess.get() ), tessGetVertexCount( mTess.get() ) );
+	} else {
+		result.appendPositions( (vec3*)tessGetVertices( mTess.get() ), tessGetVertexCount( mTess.get() ) );


I'd love suggestions on better ways to make this type conversion conditional.

paulhoux · 2016-12-23T11:17:14Z

Will this PR allow people to add both 2D and 3D vertices to the same Triangulate instance? Because I think that would be problematic, conceptually.

In that case it would be better to have a Triangulate2d and Triangulate3d, similar to other Cinder classes.

drewish · 2016-12-23T15:57:43Z

It seems like libtess2 uses 3 dimensions internally and leaves the z at 0 if you don't provide a value but I think it could be clearer to have specialized classes for each. I'll take a swing at that interface and see how it looks.

drewish · 2016-12-23T20:32:43Z

I took a swing at putting the 3d version in it's own class. I'm not sure if there's a good way to share code between the two classes. Since this is really just a proof of concept I set it up as a Triangulator3d class that used Triangulator::Winding and Exception. Ended up having to duplicate a fair bit of code but open to see what people think.

vinjn · 2016-12-24T02:38:44Z

How about converting into a templated class?

template<typename T>
class Triangulator {
};

typedef Triangulator<vec2>		Triangulator2;
typedef Triangulator<vec3>		Triangulator3;

MikeGitb

I know the code is copied from the existing class, but the allocate function seems buggy to me. However, the fact that people apparently used it successfully in the past might mean that I'm wrong.

MikeGitb · 2016-12-24T09:46:22Z

src/cinder/Triangulate.cpp

+	ma.memalloc = stdAlloc;
+	ma.memfree = stdFree;
+	ma.userData = (void*)&mAllocated;
+	ma.extraVertices = 2560; // realloc not provided, allow 256 extra vertices.


Comment doesn't fit the code (256 <-> 2560 ) - is the 0 there deliberate?

Yeah been noticing some other weird behavior since that structure has an outOfMemory member that isn't explicitly initialized to 0. So sporadically it seems to fail once it reuses memory the OS hasn't zeroed out.

Or rather, the TESStesselator's outOfMemory value wasn't being initialized. It seems like we could switch the malloc to a calloc in our stdAlloc.

But to your original point I'm not sure why that number changed or if it was accidental.

MikeGitb · 2016-12-24T09:46:24Z

src/cinder/Triangulate.cpp

+	ma.userData = (void*)&mAllocated;
+	ma.extraVertices = 2560; // realloc not provided, allow 256 extra vertices.
+
+	mTess = shared_ptr<TESStesselator>( tessNewTess( &ma ), tessDeleteTess );


This seems a bit fishy to me. I don't know how exactly the tess library works, but you are passing the address of a local variable (ma) to a function, that will take that address and store it in a heap allocated TESStesselator object which will then be used long after the local object is gone.

yeah agreed it seem suspect. it looks like tessNewTess stores a reference to it in
the mTess we get back. i'm not sure why it doesn't cause more obvious problems.

@MikeGitb what seems like a sane way to handle the TESSalloc? it seems like it should be an instance variable but then you'd need to include tesselator.h in Triangulate.h. so pointer or shared_ptr?

@drewish: If backwards compatibility was not an issue, I'd wrap the mTess pointer, the allocator and the used tess... functions and in particular the allocate function into a - forward declared - class and store a single std::unique_ptr. As things stand just creating a member variable for the allocator as you suggested seems to be the best solution.
However, as it is a private/protected variable that isn't shared with anything, I'd not use a shared_ptr but a unique_ptr.

drewish · 2016-12-24T14:52:52Z

src/cinder/Triangulate.cpp

+{
+	TriMesh result( TriMesh::Format().positions( 3 ) );
+
+	tessTesselate( mTess.get(), (int)winding, TESS_POLYGONS, 3, 3, 0 );


This returns a 1 for success but we're not checking it leading to very strange results when you hit the outOfMemory bug I mentioned in the other comment.

drewish · 2016-12-25T23:18:49Z

@vinjn yeah that was what i'd looked at initially but i wasn't clear how you'd get a scalar 2 or 3 to pass as a parameter out of the template type.

vinjn · 2016-12-26T02:15:34Z

int getVecDim(vec3) { return 3;}
int getVecDim(vec2) { return 2;}

or using type traits like ChanTraits.h
@drewish

drewish · 2016-12-26T05:17:48Z

I'm thinking this might be hard to generalize for random 3d input. I've been getting really strange behavior with multiple contours. Adding a second contour causes it to return no output. I think this is due to the way libtess builds the arrangement by projecting everything onto a single sweep plane. Duplicated points on the plan seem to clear the output. It looks like you can influence the choice of plane by passing in a normal. So I think the only way it would reliably work in 3d would be to either require a normal be provided when calculating the mesh or just limit it to a single input contour.

MikeGitb

I'm beginning to think that Triangulate needs a major overhaul. found two more bugs.

MikeGitb · 2016-12-29T02:45:25Z

src/cinder/Triangulate.cpp

+
+	tessTesselate( mTess.get(), (int)winding, TESS_POLYGONS, 3, 3, 0 );
+	result->appendPositions( (vec3*)tessGetVertices( mTess.get() ), tessGetVertexCount( mTess.get() ) );
+	result->appendIndices( (uint32_t*)( tessGetElements( mTess.get() ) ), tessGetElementCount( mTess.get() ) * 3 );


that cast from const int* (returned by tessGetElements) to uint32_t* is actually undefined bahavior (illegal)

yeah there's a few of these conversion that seemed sketchy but short of changing the libtess I wasn't sure what to do about that. the lib seems to use signed ints for a lot of things that seemed best as unsigned.

MikeGitb · 2016-12-29T02:46:10Z

src/cinder/Triangulate.cpp

+void Triangulator3d::addPolyLine( const vec3 *points, size_t numPoints )
+{
+	if( numPoints > 0 )
+		tessAddContour( mTess.get(), 3, &points, sizeof(vec3), (int)numPoints );


Is the & in front of points correct here? I don't think we want a pointer to a pointer here

Nope it's a bug. Added a test case to drive this code path and it failed until I removed the &.

drewish · 2016-12-29T05:21:07Z

src/cinder/Triangulate.cpp

-
-	mTess = shared_ptr<TESStesselator>( tessNewTess( &ma ), tessDeleteTess );
+
+	mAlloc = shared_ptr<TESSalloc>( new TESSalloc() );


@MikeGitb I tried doing a shared_ptr on this but got a rather cryptic error:

/Applications/Xcode.app/Contents/Developer/Toolchains/XcodeDefault.xctoolchain/usr/include/c++/v1/memory:2523:27: Invalid application of 'sizeof' to an incomplete type 'TESSalloc'

I assume the problem is that Triangulate has an implicitly defined destructor. That means that the compiler tries to generate the destructor in every translation unit that includes the header file. However, in most translation units, it doesn't have access to the Definition of TESSalloc which it apparently needs for the destructor of mAlloc.
Try defining an empty destructor in Triangulate.cpp

If you really want to use a shared_ptr instead of a unique_ptr may I suggest using std::make_shared?

sorry that was a typo, i meant to write unique_ptr. i agree that makes more sense so i'll try adding a destructor and see if that helps.

At least after adding the destructor I'm getting an error that's understandable:

/Applications/Xcode.app/Contents/Developer/Toolchains/XcodeDefault.xctoolchain/usr/include/c++/v1/memory:2523:27: Invalid application of 'sizeof' to an incomplete type 'TESSalloc'

Isn't that the same message as you got before?

drewish · 2016-12-29T05:22:07Z

src/cinder/Triangulate.cpp

+void Triangulator::addPolyLine( const PolyLine3f &polyLine )
+{
+	if( polyLine.size() > 0 )
+		tessAddContour( mTess.get(), 3, &polyLine.getPoints()[0], sizeof(vec3), (int)polyLine.size() );


Any reason not to just do polyLine.getPoints().data() here?

Nope, that also seems more logical to me (probably pre c++11 code when vector didn't have the data() member function). I'd usually also not write sizeof(vec3), but sizeof(*polyLine.getPoints().data()) as this makes the code more generic and less prone to refactoring erors (but admittedly also more ugly)

drewish · 2016-12-29T05:25:23Z

src/libtess2/tess.c

@@ -482,6 +482,7 @@ TESStesselator* tessNewTess( TESSalloc* alloc )
 	if ( tess == NULL ) {
 		return 0;          /* out of memory */
 	}
+	tess->outOfMemory = 0;


Since there was already one other edit to this lib this seemed like the cleanest way to make sure this value was initialized.

drewish · 2016-12-29T05:42:07Z

I went ahead and folded this back into one class. The only difference is flow is really in the mesh outputting so I just added a parallel set of calc/createMesh3d functions. The 3d version takes a normal and I think in the docs we just need to make it clear that you only want to tesselate things on the same plane unless you know what you're doing.

MikeGitb · 2016-12-30T15:44:17Z

I was wrong btw.: The cast from int* to uin32_t* seems to be legal (at least on systems, where uint32_t is the same as unsigned int

drewish · 2017-01-08T22:11:29Z

Since over on #1698 it sounded like PolyLine3 might not be around long I switched this interface to just use std::vector<vec3> instead.

drewish · 2017-01-23T03:43:00Z

src/cinder/Triangulate.cpp

-
-	tessTesselate( mTess.get(), (int)winding, TESS_POLYGONS, 3, 2, 0 );
+
+	float normal[3] = { 0, 0, 1 };


So I'd started passing this in to avoid computing it, but I need to test it out. I think the old behavior would flip the normal to use a (0,0,-1) if the winding order was backwards. I need to add test cases for that.

- Provide a normal for 2d tessleation to avoid computing one - Fix bug when adding array of vec2 - Adding some unit tests

MikeGitb · 2020-02-19T19:52:00Z

Any reason this didn't get merged?

drewish · 2020-02-20T18:04:07Z

Not that I'm aware of. Happy to rebase it or make any changes that are necessary.

andrewfb · 2020-02-24T03:36:55Z

Sorry for the unreasonable delay on this. Two quick questions as my head is no longer in this code unfortunately. First, should we consider moving to the latest here? https://github.com/memononen/libtess2 And second, does the tesselator do the right thing if I supply a mixture of 2D and 3D contours as input?

drewish force-pushed the triangulate-3d branch from 2780e38 to 975ca08 Compare December 23, 2016 05:37

drewish commented Dec 23, 2016

View reviewed changes

MikeGitb reviewed Dec 24, 2016

View reviewed changes

drewish commented Dec 24, 2016

View reviewed changes

MikeGitb reviewed Dec 29, 2016

View reviewed changes

drewish commented Dec 29, 2016

View reviewed changes

drewish force-pushed the triangulate-3d branch from a2b6128 to 8d14093 Compare December 30, 2016 04:33

drewish mentioned this pull request Jan 6, 2017

PolyLine3d doesn't really work #1698

Merged

drewish force-pushed the triangulate-3d branch 2 times, most recently from eb1430b to 2ffc84e Compare January 16, 2017 03:44

drewish force-pushed the triangulate-3d branch 2 times, most recently from 9365852 to e53ef4a Compare January 23, 2017 03:39

drewish commented Jan 23, 2017

View reviewed changes

drewish force-pushed the triangulate-3d branch from e53ef4a to 514dedd Compare February 28, 2017 04:45

drewish force-pushed the triangulate-3d branch from 514dedd to 2aa4f3e Compare July 26, 2017 04:03

drewish force-pushed the triangulate-3d branch from 2aa4f3e to 8bcb70c Compare December 31, 2017 19:44

Allow 3d triangulation

192a03e

- Provide a normal for 2d tessleation to avoid computing one - Fix bug when adding array of vec2 - Adding some unit tests

drewish force-pushed the triangulate-3d branch from 8bcb70c to 192a03e Compare March 27, 2018 14:03


		mTess = shared_ptr<TESStesselator>( tessNewTess( &ma ), tessDeleteTess );

		mAlloc = shared_ptr<TESSalloc>( new TESSalloc() );


		tessTesselate( mTess.get(), (int)winding, TESS_POLYGONS, 3, 2, 0 );

		float normal[3] = { 0, 0, 1 };

Allow 3d triangulation #1691

Are you sure you want to change the base?

Allow 3d triangulation #1691

Conversation

drewish commented Dec 23, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

paulhoux commented Dec 23, 2016 • edited Loading

drewish commented Dec 23, 2016

drewish commented Dec 23, 2016

vinjn commented Dec 24, 2016 • edited Loading

MikeGitb left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MikeGitb Dec 24, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

drewish commented Dec 25, 2016

vinjn commented Dec 26, 2016 • edited Loading

drewish commented Dec 26, 2016 • edited Loading

MikeGitb left a comment

Choose a reason for hiding this comment

MikeGitb Dec 29, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

drewish Dec 30, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

drewish commented Dec 29, 2016

MikeGitb commented Dec 30, 2016

drewish commented Jan 8, 2017

Choose a reason for hiding this comment

MikeGitb commented Feb 19, 2020

drewish commented Feb 20, 2020

andrewfb commented Feb 24, 2020

paulhoux commented Dec 23, 2016 •

edited

Loading

vinjn commented Dec 24, 2016 •

edited

Loading

MikeGitb Dec 24, 2016 •

edited

Loading

vinjn commented Dec 26, 2016 •

edited

Loading

drewish commented Dec 26, 2016 •

edited

Loading

MikeGitb Dec 29, 2016 •

edited

Loading

drewish Dec 30, 2016 •

edited

Loading