Details

Reviewers

Hans Goudey (HooglyBoogly)
Campbell Barton (campbellbarton)

Commits

rBc39d514a4eac: Geometry Nodes: Flip Faces Node

Summary

T93569

Currently there is no way to flip normals in geometry nodes. This node would make that possible
by flipping the winding order of selected faces. The node is purposely not called "Flip Normals",
because normals are derived data, changing them is only a side effect.
The real change is that the face corners of every polygon are reversed.

Only changed a duplicate of the mloop of the input mesh.
Done in a single loop, did not use MutableSpan::slice or MutableSpan::reverse since both v and e values had to be changed anyway

Also in the task description it was mentioned: The code should work on each corner attribute separately for better performance.
Don't exactly know what that means but hope that's what I'm doing.

Diff Detail

Event Timeline

Alan Babu (alanaman) requested review of this revision.Jan 12 2022, 8:47 AM

Alan Babu (alanaman) created this revision.

Alan Babu (alanaman) updated this revision to Diff 46957.Jan 12 2022, 8:51 AM

paul szajner (szap) added a subscriber: paul szajner (szap).Jan 12 2022, 8:58 AM

Campbell Barton (campbellbarton) added a subscriber: Campbell Barton (campbellbarton).Jan 12 2022, 11:11 AM

Campbell Barton (campbellbarton) added inline comments.

source/blender/nodes/geometry/nodes/node_geo_face_flip.cc
41–48	Shouldn't all the polygons custom data be reversed too? (UV's, vertex colors ... etc). This could be made into a generic utility in BKE_mesh as it's non-trivial and could be useful elsewhere.
41–48	Correction (the polygon-loop's custom data).

Pratik Borhade (PratikPB2123) added a subscriber: Pratik Borhade (PratikPB2123).Jan 12 2022, 11:12 AM

Yep, I will look into it.
Should I do the 'plan changes' thingy to stop reviews

@Alan Babu (alanaman) you can, although for short iteration cycles it doesn't matter so much.

Still finding my way around the code. Might take some time to do it.

Hans Goudey (HooglyBoogly) added inline comments.Jan 12 2022, 7:21 PM

source/blender/nodes/geometry/nodes/node_geo_face_flip.cc
23	Clang format (https://wiki.blender.org/wiki/Tools/ClangFormat) will automatically remove this unnecessary whitespace.
41–48	Right, that's what the comments about attributes were for in the todo task. Let me know if you have questions about that.
44	This looks like it could use `for (const int j : IndexRange(poly.loopstart, poly.totloop / 2) {` for slightly nicer syntax and a const variable

I think we need a copy of the data in all the layers of ldata so I used CustomData_copy().

Found a function for flipping already there(BKE_mesh_polygon_flip()) so used that.

mloopuv and mloopcol were pointing to the data of the input mesh. I don't know if I updated that correctly.

There was also something called mlooptri. Don't know what that is. Hope that doesn't need changes.

Tested with UVmaps and vertex colors. They weren't jumping around.

Alan Babu (alanaman) added a reviewer: Campbell Barton (campbellbarton).Jan 12 2022, 8:57 PM

Please apply clang format on the patch (https://wiki.blender.org/wiki/Tools/ClangFormat)

The attribute copying should use the attribute API on the MeshComponent.
It should loop through all of the attributes, and for attributes on the corner domain, retrieve write access and flip the loops for all selected polygons.

Like is discussed on the todo task, the goal is to not use BKE_mesh_polygon_flip because of the inefficient way it accesses attribute data.

source/blender/nodes/geometry/nodes/node_geo_face_flip.cc
51	Since the polys don't need to be changed, this can be a `Span` rather than a `MutableSpan`

Hans Goudey (HooglyBoogly) requested changes to this revision.Jan 12 2022, 9:10 PM

This revision now requires changes to proceed.Jan 12 2022, 9:10 PM

The previous mloop reversal was causing the internal triangulation to change because the 1st vertex was changing. Fixed it.

I was only getting a GMutableSpan from the OutputAttribute. Could'nt find a way to work with its data.

The attribute_math::convert_to_static_type(data_type, [&](auto dummy){}) was the only way I could find to get a MutableSpan.
I kinda copied it from another file. Don't know what it does or how it works.

perhaps a bunch of headache could be avoided, if normals could be set like crease can on output?

Gregory (Apofis) awarded a token.Jan 15 2022, 5:53 PM

Gregory (Apofis) added a subscriber: Gregory (Apofis).

This works well in my testing, the rest is basically code style comments and recommendations for smaller improvements.

The attribute_math::convert_to_static_type(data_type, [&](auto dummy){}) was the only way I could find to get a MutableSpan.
I kinda copied it from another file. Don't know what it does or how it works.

This is a way to write code that works with multiple data types at the same time. It's a template function that will end up instantiating the lambda you give it for every attribute data type: int, float, float2, etc.

perhaps a bunch of headache could be avoided, if normals could be set like crease can on output?

Face normals are derived data and cannot be controlled directly. That's an unrelated task, for more information see T93551.

source/blender/nodes/geometry/nodes/node_geo_face_flip.cc
49	There is no need to copy the whole mesh to the stack, this can just use a reference.
59	Code style things: Give `a` and `b` more descriptive names Declare them as const variables Declare them each on a separate line.
64–66	This can be simplified to `for (const AttributeIDRef &id : component.attribute_ids()) {` Actually though, a call to `component.attribute_foreach` will be a bit simpler/faster since it provides access to the data type and domain directly.
71	The more common variable name for this is `attribute` rather than `OA`. It's worth trying to keep the code style of various nodes consistent I think.
88–90	Debug code should be removed.