Fix indices winding in convex hull generator

[pouleyKetchoupp]

Invert indices to match Godot's winding like the previous QuickHull algorithm was doing.

Follow-up to #48533 (comment) (CC @lawnjelly)

[pouleyKetchoupp]

CC @mortarroad in case you would like to review.

[lawnjelly]

That will work as long as the faces are all single triangles. I'm not sure if it will work if it is a fan, but I don't know whether the bullet convex hull generator generates any fans. I'll see if I can try this out.

Also the inserts at the front each time is not the most efficient, but I don't know if you can read from bullet how many indices there are in the face in advance. Mind you if it is always 3 you could resize the output face indices to 3 in advance and write in reverse order.

EDIT: Yes it does produce fans. Let me see if this will work as is or need tweaking.

Ok with a flat face the result looks the same, but the topology will change I think. So instead what I think you need to do is this:

Write vertex 0 the same (the base vertex of the fan)
Write all other vertices in reverse order

Something like:

face.indices.push_back(e->get_target_vertex());

// then for the rest
face.indices.insert(1, e->get_target_vertex());

If you simply reverse the order, the base vertex of the fan will change and hence the topology. BTW don't worry excessively about the presizing the output in advance, it's just a nitpick. It won't be the bottleneck.

[pouleyKetchoupp]

@lawnjelly I just found out there was a method get_next_edge_of_vertex() that returns vertices in the expected order (get_next_edge_of_face() was in reverted order).

From what I've tested, it does exactly the change you suggested compared to my previous version, so it should be all good now.

[lawnjelly]

Great that sounds even better. 👍

[lawnjelly]

Didn't want to approve this last night until I'd tested .. just as well. I don't think it works at all (it may work for triangles but not for faces). I'll see if I can screencap some pics. I think we might have to go back to the get_next_edge_of_face.

This is the correct test shape convex hull:

This is after this PR:

And here is a list of the indices of the faces, produced using the old_method (wrong winding) versus the get_next_edge_of_vertex new_method:

new_method : 1, 3, 2, 
old_method : 1, 5, 2, 0, 

new_method : 2, 1, 3, 
old_method : 2, 7, 3, 0, 

new_method : 3, 2, 1, 
old_method : 3, 4, 1, 0, 

new_method : 4, 0, 5, 
old_method : 4, 6, 5, 1, 

new_method : 5, 0, 7, 6, 
old_method : 5, 6, 2, 

new_method : 6, 5, 0, 7, 
old_method : 6, 7, 2, 

new_method : 7, 0, 4, 
old_method : 7, 6, 4, 3, 

[mortarroad]

Hm? Is this really broken? I didn't actually verify the winding, but I remember testing which one works with the Godot physics. It caused objects to fall through the ground, so I already flipped it back then.

[mortarroad]

Hm, I suppose it is wrong. getNextEdgeOfFace() says that it returns counter-clockwise, and apparently Godot does things clockwise... I will provide a patch soon.

[lawnjelly]

Try this:

do {
	face.indices.push_back(e->get_target_vertex());
	e = e->get_next_edge_of_face();
} while (e != e_start);
		
// reverse the winding order of the triangle fan while retaining the topology
int num_to_exchange = (face.indices.size() - 1) / 2;
int *p = face.indices.ptrw();
for (int c = 0; c < num_to_exchange; c++) {
	SWAP(p[c + 1], p[face.indices.size() - 1 - c]);
}

Verified working (with the 0, 1, 2 order change for the plane as in the PR originally):

face before 1, 5, 2, 0, 
face after 1, 0, 2, 5, 
face before 2, 7, 3, 0, 
face after 2, 0, 3, 7, 
face before 3, 4, 1, 0, 
face after 3, 0, 1, 4, 
face before 4, 6, 5, 1, 
face after 4, 1, 5, 6, 
face before 5, 6, 2, 
face after 5, 2, 6, 
face before 6, 7, 2, 
face after 6, 2, 7, 
face before 7, 6, 4, 3, 
face after 7, 3, 4, 6, 

Actually I copied the whole ptrw() thing from the invert() implementation in Vector, but I suspect the ptrw() would be better outside the loop. I have no idea why it was inside the loop in the invert() function. I've changed it to outside the loop. There's a few things in Vector which are a bit dodgy like that.

[akien-mga]

Superseded by #50282.