Skip to content

Dealing with normals of sharp edges #139

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

Closed
ffreyer opened this issue Mar 11, 2021 · 6 comments
Closed

Dealing with normals of sharp edges #139

ffreyer opened this issue Mar 11, 2021 · 6 comments

Comments

@ffreyer
Copy link
Collaborator

ffreyer commented Mar 11, 2021

For a smooth underlying shape, you want normals to vary smoothly as well. That way shading will restore smooth of the curvature that is lost in triangulation. If you have sharp edges though, this effect is undesired. In a cube for example, you want normals to jump when moving from face to face, even if you're looking at the same position.

One way to deal with this is to duplicate vertices (i.e. position and normal) like I did in #36 a while ago. This is the case now for some primitives, likeRect3D or Pyramind.

Screenshot from 2021-03-11 12-46-18
(The bottom face has normals in the wrong direction here...)

Other primitives don't do this, like for example Cylinder.

Screenshot from 2021-03-11 12-47-09

We could go through all the mesh generating functions and duplicate vertices where needed, but I don't think that a good idea. My concerns are the following:

  • We should avoid repeating data. Currently a Rect3D generates 24 coordinates instead of 8. There are also only 6 unique normals, but we generate 24. That's a lot of extra fat...
  • It's weird to get duplicate positions from coordinates as user. For example, in MakieTeX I needed to transform a 3D bounding box and I thought I could just do map(v -> transform * v, coordinates(bbox)) and then remembered that that is 3x more work than it should be.

I think the way to deal with is detach positions, normals, etc, for example by changing faces to have one index per attribute like in obj files. Or by adding some remapping functionality that can map vertex indices (1:24 for Rect3D) to attribute indices (1:8 for positions, 1:6 for normals). Making these changes should also mean that reading obj files (and probably others?) becomes easier. (I think JuliaIO/MeshIO.jl#64 would become unnecessary and we could go back on it, for example.)

@goretkin
Copy link

I am not very familiar with this, but I believe there are conflicting desiderata for the representation.

  1. A general search for "openGL normal per face not per vertex" suggests that per-vertex normal vectors are important for graphics performance. For GPUs, flat representations generally seem to win, sometimes even if they are redundant by e.g. a factor of 3x.

  2. But in fact the geometry of Rect3D does contain 8 vertices. If 24 is needed to make GPUs happy, that's fine. But 8 is needed to make the mind's eye happy, and as you point out, it is better for other computations.

So it seems beneficial to support both.

I think the way to deal with is detach positions, normals, etc, for example by changing faces to have one index per attribute like in obj files.

How are obj files loaded in to the GPU? Does the representation need to flattened before sending?

@ffreyer
Copy link
Collaborator Author

ffreyer commented Mar 11, 2021

A general search for "openGL normal per face not per vertex" suggests that per-vertex normal vectors are important for graphics performance. For GPUs, flat representations generally seem to win, sometimes even if they are redundant by e.g. a factor of 3x.

I'm not suggesting that this is the representation that should end up on a GPU. Afaik you usually upload your mesh once and try to do all the transformations on the gpu, so reconstructing the flat representation from the compressed one would be a one time cost. I also think the verbose version should still be around and usable if possible .

How are obj files loaded in to the GPU? Does the representation need to flattened before sending?

The current process is using MeshIO/FileIO to load it into a GeometryBasics mesh and to put that on the GPU (in GLMakie). Generally an obj file represents faces as

f pos/uv/normal pos/uv/normal ... pos/uv/normal

where pos/uv/normal are 3 indices (or sometimes it's just pos, or pos/uv). These can be different and refer to indices into a position/uv/normal array. So depending on the file you may need to remapping of these indices and duplicate normals, uvs and or positions to get a valid GeometryBasics mesh. This remapping process was added in the pr I linked.

@goretkin
Copy link

I'm not suggesting that this is the representation that should end up on a GPU.

But "this" representation is what currently ends up on the GPU, right? That's the reason why Rect3D contains 24 vertices currently?

@ffreyer
Copy link
Collaborator Author

ffreyer commented Mar 11, 2021

Pretty much, yea.
https://github.com./JuliaPlots/GLMakie.jl/blob/dc6c460f1f86af5f856a290887ddd919a9f8615c/src/GLAbstraction/GLUtils.jl#L174

I'm thinking of something like this: (view this as pseudocode)

normal_buffer = GLBuffer(mesh.normals[mesh.normal_indices])

where GLBuffer is just a reference to the gpu array so the intermediate array isn't kept.

@SimonDanisch
Copy link
Member

I think we should be able to do this with normals = view(normals, normal_indices).

@ffreyer
Copy link
Collaborator Author

ffreyer commented Feb 11, 2025

Fixed via FaceView

@ffreyer ffreyer closed this as completed Feb 11, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants