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

Face ordering wrong after extruding? #339

Closed
therealprof opened this issue Mar 12, 2022 · 6 comments · Fixed by #484
Closed

Face ordering wrong after extruding? #339

therealprof opened this issue Mar 12, 2022 · 6 comments · Fixed by #484
Assignees
Labels
topic: core Issues relating to core geometry, operations, algorithms topic: display Displaying Fornjot models type: bug Something isn't working

Comments

@therealprof
Copy link
Contributor

I noticed (while playing with colors and transparency) that the back surface of a swept sketch is not transparent when viewed from behind. Is it possible, that the order of the vertices is wrong and thus everything attached to it (like the normals)?
Screenshot 2022-03-12 at 15 22 12
Screenshot 2022-03-12 at 15 22 26
Screenshot 2022-03-12 at 15 24 59
Screenshot 2022-03-12 at 15 25 27

@hannobraun
Copy link
Owner

Thank you for opening this issue, @therealprof.

Is it possible, that the order of the vertices is wrong and thus everything attached to it (like the normals)?

Yes, this is definitely the case. Some time ago, all operations were really careful about the triangles they generated, and everything was fine. Now the operations themselves are (mostly) no longer responsible for generating triangles (#97). That introduced some complications, which in turned caused bugs. This was tracked in #173.

I solved that issue recently, by adjusting the shading code such that it doesn't care whether it's shading the back or front side of a triangle (#289). I knew this was a stopgap, but I didn't know you come along breaking things again just a week later 😁

Fully solving this issue would require teaching the kernel about the orientation of surfaces. Right now, as far as the kernel is concerned, there is no such thing as a front or back side or a surface/face, so it can't really know what the "right" triangle orientation would be.

The question is, do we need to implement this concept of surface orientation to solve this new issue with the transparency, or is there a similar minimally invasive fix we can make in the graphics code? It's been a while since I did real work there, so I'm not sure.

@hannobraun hannobraun added type: bug Something isn't working topic: display Displaying Fornjot models topic: core Issues relating to core geometry, operations, algorithms labels Mar 14, 2022
@therealprof
Copy link
Contributor Author

I'm not exactly sure about the overall impact of incorrectly oriented surfaces, transparency, shading and culling are the issues which might arise on the rendering side of things. There might be additional problems on the kernel side but I'm not sure at the moment, anything requiring correct normals would quite certainly be impacted.

Regarding what to do about it... For sweeping, the solution is somewhat simple: Either the duplicated end surface or the already existing end surface (depending on positive or negative sweeping offset) need to be mirrored.

@hannobraun
Copy link
Owner

I'm not exactly sure about the overall impact of incorrectly oriented surfaces, transparency, shading and culling are the issues which might arise on the rendering side of things.

Shading is taken care of #289. I don't think we do culling, but I might misremember.

There might be additional problems on the kernel side but I'm not sure at the moment, anything requiring correct normals would quite certainly be impacted.

The kernel doesn't care. As far as it is concerned, there's simply no such thing as front/back or inside/outside. That's going to change, of course, but for now, there is no code in the kernel side that relies on these concepts.

Normals are only a thing on the graphics side. They are calculated from the triangle winding when converting the kernel-generated triangle mesh into the triangle mesh format that the graphics code uses.

Regarding what to do about it... For sweeping, the solution is somewhat simple: Either the duplicated end surface or the already existing end surface (depending on positive or negative sweeping offset) need to be mirrored.

Yes, it's probably not difficult. How do you mirror a face though? I think you can do that by inverting the curve that the face is swept from. Easy enough for flat faces that were swept from lines (just invert direction), but I don't think that Circle has any way to do that.

We could just leave circles un-fixed for now, or this might be an opportunity to rethink the circle representation (related issues: #100, #171).

@therealprof
Copy link
Contributor Author

If you have a mesh tesselated to triangles, changing the orientation is rather simple: you just have to swap the order of any two vertices for all triangles.

@hannobraun
Copy link
Owner

Tessellation happens after everything else though (with the exception of sweep side walls; see #97). So the operations that create the faces would already have to store the desired face orientations, which the tessellation code can then use when generating triangles. That's what I mean by "teaching the kernel about the orientation of surfaces". The kernel has no concept of that, and to influence tessellation in that way, it would need one.

An alternative is to coax the surface coordinate system in such a way that triangles end up being generated with the correct winding, which would be possible without additional infrastructure (just mirror the surface, as you say). That's what the last part of my previous comment was about.

@hannobraun
Copy link
Owner

I've started looking into this.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
topic: core Issues relating to core geometry, operations, algorithms topic: display Displaying Fornjot models type: bug Something isn't working
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants