Fix obj loader

[Olli1080]

This pull request enables the obj loader to load files which do not use exactly one normal per vertex and also allow for loading of files where normal indices are used for multiple vertices. (e.g. obj files exported by blender).

To do so it accumulates the normals used per vertex over the faces and computes normals for the vertices.

[mvieth]

Hi, thanks for the fix, I would like to ask two things: firstly, could you post a file where the problem occurs, so that I can understand the problem better? And secondly, you have changed a lot of formatting (most importantly, indentation), which makes the changes very unclear and difficult to review. Could you revert all formatting changes, please?

[Olli1080]

Github doesn't allow me to upload the file directly as an obj file so i attach it as a zip.
franka_link0.zip
This file was created by importing a dae file from https://github.com/frankaemika/franka_ros/tree/develop/franka_description/meshes/visual and exporting it as a obj file using blender.
In the file every vertex has more than one normal assigned but some normals where used for multiple faces as well (i guess deduplication efforts by blender).
The previous code assumed that "vn" is 1:1 for vertices.

Sorry for the formatting issues i'll fix them and push the changes.

[Olli1080]

The diff should now only cover the relevant lines.

[mvieth]

Thank you, the diff is much clearer now. And I think your solution of storing normals and averaging them per vertex makes sense.
It would be good if we could create a test for this fixed behaviour. There is one at https://github.com/PointCloudLibrary/pcl/blob/master/test/io/test_io.cpp#L1038 for reading a PCLPointCloud2 from an obj file, but that does not test the values of x, y, z, or the normal. Here is an idea:

pcl::PointCloud<pcl::PointNormal> cloud;
pcl::fromPCLPointCloud2(blob, cloud);
EXPECT_EQ(cloud.size(), 8);
for(const auto& point: cloud) {
  Eigen::Vector3f expected_normal(point.x, point.y, point.z);
  expected_normal.normalize();
  EXPECT_NEAR(expected_normal.x(), point.x, 1e-4);
  EXPECT_NEAR(expected_normal.y(), point.y, 1e-4);
  EXPECT_NEAR(expected_normal.z(), point.z, 1e-4);
}

Which basically tests whether the vector from the cube center to the point is the same as the point normal. If you think this is good, would you be willing to add this to this pull request?

I am also wondering about the use of normals in the obj file format, maybe you have an intuition about this. In the file you uploaded, for each face, all vertices have the same normal (the normal of the face). If this is the intended use of the normals, it is strange why the creators of the file format chose to have the normals specified per vertex (redundant) instead of per face (e.g. f <normal index> <v1>/<vt1> <v2>/<vt2> ...). Another possibility is that it is intended to be the normal of the individual vertex, so that the normals of the different vertices of one face may differ, but if the coordinate indices of two vertices (of different faces) are the same, then the normal indices must also be the same. PCL currently writes obj files following this second logic. But I am wondering what programs that read obj files usually expect, what do you think? I tried googling but did not find a definitive answer so far.

[Olli1080]

I integrated the suggested test into the existing one.
From what i've seen the normals are sometimes interpreted as vertex normals but other times as face normals (hence same normal for all vertices of a face).
These links highlight some of the oddities:
https://www.reddit.com/r/GraphicsProgramming/comments/173hfox/are_vn_in_obj_files_really_face_normals/
https://stackoverflow.com/questions/58535803/what-does-the-normals-vn-in-wavefront-obj-files-represent

The obj file i provided also has one more irregularity e.g. in line 3512 it reuses vn 1227 in (i guess) an attempt to compress the file by deduplication.
I don't think the approach of pcl to saving obj normals per vertex is wrong as (from what i've seen) differing shading types are somewhat ignored.

[mvieth]

Thanks again for the pull request and the background information!