Output normals with a consistent orientation for convex meshes

[spelufo]

Hi, so this is a guess at what can be done to output consistent normals.

Let me know if you think this is an OK solution to the issue.

[csparker247]

Thanks for opening the PR! I'm certainly interested in some consistency here.

Can you describe in a comment what the high-level principle is for your change? It helps me make sure I'm correctly interpreting what I'm reviewing.

[spelufo]

The issue is that segments output by VC render don't have a consistent normal orientation. This can be seen by loading the OBJs into Blender and toggling in the option to visualize normals, and is also corroborated by running ink detection on both directions as jrudolph (on discord) has done and can be seen here https://vesuvius.virtual-void.net/. If the core of the scroll was known, the orientation could be determined by taking the dot product of a radius vector from the core of the scroll and a mesh normal, and checking it's sign. The convention that is adopted here, suggested by Stephen on discord is to have the mesh normals face inwards so that they point from verso to recto, towards the core.

I have the coordinates of the core and could use that for scroll 1, but I think the approach in this PR will have the same effect without hardcoding to a specific kind of data or requiring annotation. The intuition is that for convex meshes, and probably a lot of others that are not strictly convex, the "center" / average position of the vertices of the mesh can be used as the reference to orient the normals. The sign of the dot product of a vector to a vertex from this center point with a face normal will tell you if the face is looking inwards or outwards.

The (very ugly) code in this PR does this operation for all the faces in the mesh and simply counts how many face either way. A "vote" is taken and if more face inwards we're ok. If not we flip them all.

That's the idea. It may have some undesirable property I'm not thinking of but it is an improvement over getting unpredictable (in practice) orientations. Also, this place in the code may not be where or how to do it best. I'm not familiar with the code base, this looked like the right place to do it but you'll know best.

Hope that helps, thanks.

[csparker247]

Perfect, thanks!

[csparker247]

In general, I like it and it makes sense to me. My comments can be fixed fairly easily.

The one "blocker" is that I would like this to be optional behavior. It could just be a flag on this class, but I would prefer it to be a standalone OrientNormals class so we have a basis for the future features discussed in #50. If you want to do that, feel free. Otherwise, I'm happy to do that change, but it may be a few days before I can get to it.

Oh, and really, thank you for the contribution! I'm glad someone has been giving this some thought.

[spelufo]

Hey, I did some of the changes. I'll leave refactoring into a separate class to you if that's what you want, I'd get the details wrong.

[csparker247]

Sounds good. Thanks!

[csparker247]

@spelufo I've refactored this into a class, but I need to be able to push to your fork to update it here. I'm honestly not sure why it's not letting me.

Anyway, whenever you can see it, I'd like you to review the logic and make sure I didn't mess anything up. I made two changes:

1. As a standalone class, we only have access to vertex normals (without recomputing face normals), so I use the vertices in place of the face centroid from before.
2. Our vector linking the reference point (i.e. mesh centroid) and the vertex point is now ref - vert rather than vert - ref. In essence, we're testing if the vertex normal is codirectional with that, and if not, we flip all the normals.

[spelufo]

Hi, I'm not seeing the changes yet. I've added you as collaborator on my fork, perhaps try again or make a new branch here. I don't care if this PR isn't merged but the code makes it into VC.

[csparker247]

There we go. Weird git shenanigans, but it's all working now.

[spelufo]

LGTM. But it doesn't look like it is being called from anywhere yet, right? Will this be enabled by default?

[csparker247]

Perfect! Yep, it's not getting used yet. I'm currently working on adding it to the existing apps.

[csparker247]

This looks like it has a lot in it now, but it's largely all related to getting OrientNormals working everywhere we want. Here's a final change log:

• (meshing) Add OrientNormals and unit tests.
• (graph) Add OrientNormalsNode.
• (apps) Added --orient-normals to vc_generate_ppm, vc_mesher, and vc_render.
• (apps) Removed unused RenderMeshing.cpp.
• (core) Fixed winding order for faces of shapes::Cube so face normals all point outward. Likewise updated all tests which started failing because of this.
• (core) Moved the JSON serialization specializations from Metadata.hpp to util/Json.hpp, required for OrientNormalsNode serialization.
• (meshing) Removed unused .pcd files from unit test resources

This should basically just require real world testing, which I'll do tomorrow.

[csparker247]

Tested, found a bug, and all seems to work well now. One quick gotcha for anyone reviewing surface normals with MeshLab: the face normals (the default normals for shading in MeshLab) are drawn from the winding order of the faces. This can lead to you thinking that nothing has actually changed, but if you inspect the normals using Render -> Show Normal, you can see the truth:

[spelufo]

That's great. Thanks!