Fix bug in CylinderMesh when computing normals

[jbcolli2]

While writing unit tests for CylinderMesh I realized the following bug. When the top radius and bottom radius are different, the normal should be slanted in the y direction to some degree.

However, in primitive_meshes.cpp, the y component of the normals along the side of the cylinder was always set to zero. Essentially, the normals were being computed as if it were a right cylinder, as opposed to a sort of trapezoidal cylinder with different radii on top and bottom.

[akien-mga]

This y component is constant so it could be computed only once outside the for loop.
Style wise, you should also have spaces around / (run clang-format locally to make sure to us the right style).

[akien-mga]

The whole class might need a thorough review because there might be other calculations which expect a cylinder to be... a cylinder.
It's a bit weird IMO that this class supports making a cone frustum.

[jbcolli2]

Thanks for the help. I've fixed the formatting.

I'm a first time contributor, so I could be wrong but I don't think the tangent needs to be changed. I'm not positive what the tangent is calculating as there are at least two orthogonal tangents to the normal. But it seems like the tangent is pointing along the rings of the cylinder, and so that vector wouldn't change if the side gets slanted. Let me know if I'm thinking of this wrong.

I have a PR in with unit tests for all the PrimitiveMesh subclasses. I tested CylinderMesh pretty well and it looks good. The class doesn't do much except create the mesh, and I tested the vertex positions and normals. The only thing I see that could be off are the texture coordinates, and it doesn't seem to me that they would obviously change if the shape is a cone frustrum.

[akien-mga]

Makes sense yes.

Seems like you missed part of my review comment:

This y component is constant so it could be computed only once outside the for loop.

I.e. you could compute const real_t normal_y = (bottom_radius - top_radius) / height; before the for loop so it's computed only once.

[akien-mga]

Thanks! And congrats for your first merged Godot contribution 🎉

[jbcolli2]

Thanks for your help! :-)

[akien-mga]

Cherry-picked for 3.6.

[timothyqiu]

Cherry-picked for 3.5.2