Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Discontinuous normals in ppm files #43

Closed
1 of 4 tasks
KhartesViewer opened this issue Oct 13, 2023 · 16 comments · Fixed by #53
Closed
1 of 4 tasks

Discontinuous normals in ppm files #43

KhartesViewer opened this issue Oct 13, 2023 · 16 comments · Fixed by #53
Labels
bug Something isn't working

Comments

@KhartesViewer
Copy link

KhartesViewer commented Oct 13, 2023

What happened?

I was testing some code to read ppm files. This code would read the ppm file, use the xyz information in the file to create a mesh in xyz space, and use the normal information in the file to create lines in xyz space extending from the mesh. After hiding most of the mesh and most of the normals (in order to create a picture I could make sense of), I got this:
discontinuous_normals
I noticed there were a number of discontinuities, which seemed to be located at the boundaries of the triangles from the original obj mesh that was used to create the ppm file.
I dug into the VC code, and in PPMGenerator.cpp came across the PhongNormal function, which contains the following lines:

 return cv::normalize(
        (1 - nUVW[0] - nUVW[1]) * nA + nUVW[1] * nB + nUVW[2] * nC);

This formula seems odd to me; I would have expected:

nUVW[0] * nA + nUVW[1] * nB + nUVW[2] * nC);

I'm not currently in a position to compile and run the VC code, so I cannot test whether my version would solve the discontinuous-normal problem. But in any case, I believe the discontinuous normals could be the cause of the "sharkbite" problem that was previously reported (bug #33).

Steps to reproduce

No response

Version

No response

How did you install the software?

None

On which operating systems have you experienced this issue?

  • macOS
  • Windows
  • Linux

Relevant log output

No response

Code of Conduct

  • I agree to follow this project's Code of Conduct
@KhartesViewer KhartesViewer added bug Something isn't working triage Needs evaluation by developer labels Oct 13, 2023
@KhartesViewer KhartesViewer changed the title Bug in PhongNormal? Discontinuous normals Oct 13, 2023
@KhartesViewer KhartesViewer changed the title Discontinuous normals Discontinuous normals in ppm files Oct 13, 2023
@csparker247
Copy link
Contributor

Thanks. Can you upload the mesh or point set for the subregion you've shown so I have a small dataset to test with?

Going off memory, the shading is interpolating normals for barycentric coordinates nUVW. To guarantee the point is in the triangle, the sum of barycentric coordinates on a triangle is 1. So here we use two of the provided coordinates as given and generate the third as the remaining magnitude after subtracting the first two from 1. It's probably worth comparing this calculated value to the one provided to the function, but if they differ significantly, we should also check if the values being provided to Phong are correct. By this point in the process, they should sum to 1. We're only shading points in specific triangles. If our barycentric point is outside the triangle, we're using the wrong triangle OR we need to better handle the boundary condition (two triangles intersecting one pixel at subpixel coordinates).

@csparker247 csparker247 removed the triage Needs evaluation by developer label Oct 13, 2023
@csparker247
Copy link
Contributor

As a side note, the cell map PPM sidecar file tells you the original triangle ID for each pixel. You can use that to color each normal/point according to its triangle association. Might help your visualization.

@KhartesViewer
Copy link
Author

OK, I'll have to think about the best way to get you the pointset. I have a python script that reads in a full ppm file, computes the xyzs and normals everywhere, and then creates a mesh (an obj file for visualization in meshlab) over a small subset for visualization. If you want, I can send that script along, once I am sure it is readable. But it sounds like you want a subset of the obj mesh that was fed into PPMGenerator; if that is what you want, I would have to figure out how to create it. I never ran the VC code to generate a ppm file, I just used ppm files I found on the server. So I don't really have a starting mesh/pointset of the kind that I think you are requesting.
I did wonder if the code was making use of the barycentric identity nUVW[0] + nUVW[1] + nUVW[2] = 1., but if that is the case, there is still something odd. From the code:
(1 - nUVW[0] - nUVW[1]) * nA + nUVW[1] * nB + nUVW[2] * nC);
But 1 - nUVW[0] - nUVW[1] reduces to nUVW[2], so the code is equivalent to:
nUVW[2] * nA + nUVW[1] * nB + nUVW[2] * nC);
and I can't think of a reason for multiplying both nA and nC by nUVW[2].
I don't think the problem is due to pixels being assigned to the wrong triangle. I did a separate experiment where I tried to generate a ppm file from an obj file, and due to bugs in my code, some of the pixels near the triangle borders got assigned to the wrong side of the border. This was very visible in the xyz surface created from the ppm (without even looking at the normals); there was a clear stair-step effect at the triangle borders. I haven't run across this artifact in the ppm file I downloaded from the server, so I don't think that's the cause of the discontinuous normals.

@csparker247
Copy link
Contributor

Ahh yes, good eye. It probably should be 1 - uvw[2] - uvw[1].

You can just point me to the PPM you're using and the ROI you're cropping to for your mesh.

I'm not actually sure that this isn't also connected to aliasing of the normals along the boundary of two (original) triangles. But we'll need to look at more than just the one row of normals that you're showing.

@KhartesViewer
Copy link
Author

KhartesViewer commented Oct 14, 2023

The ppm file is 20230827161846.ppm (I think you can figure out the rest of the path from that). The corners of the ROI are (I think, the code may have been modified a little): (3500, 1896), (4100, 1904). So its width is much greater than its height.
I didn't choose this ROI for any reason in particular, I was just looking for a typical region of a typical surface. Of course, if you are testing the code, you might want to find an area of higher curvature, so problems are more visible. My tests were more in the other direction, to see if there were any bugs that would manifest themselves even on fairly smooth surfaces.

@KhartesViewer
Copy link
Author

Almost forgot to show a sharkbite example. In the same ...1846 segment as above, here is a view of tif 64, centered at (6235, 1845):
sharkbite
And here is the corresponding cross section:
sharkbite_xsect

If you look carefully at the layer 64 tif, you will see sharkbite in a lot of places, this is just one example.

One interesting test will be to see whether the sharkbite goes away when the normal code is fixed. That is what I am hoping, anyway; I first started studying the normals because I was trying to figure out the cause of all the sharkbite I was seeing in this segment.

@csparker247
Copy link
Contributor

I can't seem to replicate this. We have a developer utility vc_ppm_to_pointset for extracting points from a PPM, so I modified it to also save the vertex normals, and this is what I get with the original PPM (your strip ROI loaded alongside the original mesh):

orig_strip01

You can kind of see what looks to be a discontinuity a little up and to the right from the center, but if you zoom in on that region, you can see that the normals actually appear pretty continuous, but the points themselves are slightly "discontinuous" with respect to the row:

orig_strip_cu03

That row discontinuity doesn't surprise me and just looks like rasterization to me. All of that said, I did make the barycentric coordinate change, and it also doesn't really change much with the normals:

fixed_strip02

And a diff of the two showing the only major differences are in vertex positions (less than half a pixel):

diff-strips

I still need to investigate how this affects the sharkbite, but I'm anticipating it won't change much.

@csparker247
Copy link
Contributor

Oh, and here are the extracted strips as PLY files. The fixed file is the one where I updated the normal calculation.

ppm-strips.zip

@KhartesViewer
Copy link
Author

Thanks for looking at this!
I notice one difference between your picture (the first one in your comment) and mine is that your normals are short (about the distance between two adjacent ppm points) and mine are a lot longer (8 times longer, if I am reading my code correctly). I believe that with longer normals, the discontinuities would be more obvious.
Anyway, I'll be curious to see how your sharkbite experiments turn out!

@csparker247
Copy link
Contributor

Okay, so extending the normals does show me the effect:

side_longer02

But it also appears to occur across triangle boundaries, which makes sense to me. Here's a top-down view. The biggest orange lines are the vertex normals of the original mesh (what we're interpolating from), and the green lines are the face normal that MeshLab creates from the vertex normals:

top_down00

And here's one with orthographic projection:

top_down_ortho01

That all seems fairly normal to me given the interpolation method, though: everything within the triangle is smooth, but it's not necessarily so across triangle boundaries.

@csparker247
Copy link
Contributor

Alright, I've also confirmed that this doesn't affect the sharkbite example you referenced.

Before (layer 64):

orig_layer_64

After (layer 64):

fixed_layer_64

Before (normal map):

orig_normals

After (normal map):

fixed_normals

I think this particular sharkbite is actually explained by the 3D geometry. Here's the above region in 3D:

sharkbite_region_01

sharkbite_region_02

Since it's a trough, you have convexity and the normals start crossing each other the further you get from the surface points. So by 32 pixels off of the surface (layer 64, ~0.25mm), the volumetric space we're sampling across adjacent triangles is just nowhere close to the same. This doesn't show that exactly, but it illustrates the point, I think:

sharkbite_cu_00

Anyway, I think I've decided that we found a small bug in the interpolation, so we'll get that fixed (#46). The remaining discontinuity is a limitation of the shading method and not a bug per se. I think you could make a case that we should have a method that produces smoother normals at the boundaries of triangles, and I'd happily review PRs implementing something like that.

@KhartesViewer
Copy link
Author

OK, sounds good, thanks for the thorough check. I'm closing the issue now.

@KhartesViewer
Copy link
Author

Oops, sorry to reopen this again just after closing it.
I was looking at your PR #46 and noticed that it doesn't seem to address the original bug I noted in PhongNormal() in PPMGenerator.cpp. Instead, it addresses a similar bug in BarycentricCoordinates.cpp.
I'm hesitant to mention this, since you have already devoted a lot of time to this issue, but I thought I should bring it up just in case. Please feel free to close this issue without comment if it looks like I'm off here.

@KhartesViewer KhartesViewer reopened this Oct 27, 2023
@csparker247
Copy link
Contributor

Bah, you're right! That invalidates all of the testing above. 🙃

Anyway, easy enough to fix and redo. I really should get rid of the double implementation. I'll do this sometime next week.

@csparker247
Copy link
Contributor

csparker247 commented Nov 1, 2023

Well, we got there in the end. Using PhongNormal:

normals_orig_00

Using BarycentricNormalInterpolation:

normals_fixed_01

And you'll be happy to know that it does, in fact, fix the sharkbite! Original:

sharkbite_orig

And fixed:

sharkbite_fixed

Thanks for looking over my shoulder on this! The fix is in #53.

@csparker247 csparker247 linked a pull request Nov 1, 2023 that will close this issue
@KhartesViewer
Copy link
Author

You're welcome! Thanks for the kind words!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants