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.

Sign up for GitHub

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

Jump to bottom

Surface orientation is not well-defined #173

Closed
hannobraun opened this issue Feb 11, 2022 · 3 comments · Fixed by #289
Closed

Surface orientation is not well-defined #173

hannobraun opened this issue Feb 11, 2022 · 3 comments · Fixed by #289
Labels
topic: core Issues relating to core geometry, operations, algorithms type: bug Something isn't working
Milestone
Straight edges, f...

Comments

@hannobraun
Copy link
Owner

hannobraun commented Feb 11, 2022 •
edited
Loading

Surface orientation, i.e. knowing which side of a surface is the outside/above and which is the inside/below, is not well-defined. That used to not matter much, but as the CAD kernel has grown more sophisticated, the need for a well-defined surface orientation concept has arisen.

One symptom of this problem are the inner walls of extruded shapes:

Screenshot from 2022-02-11 15-45-24

Those inner walls should be red when looking at them directly, just like the outer ones.

This issue originally tracked one specific symptom (inner walls of the star model) of the larger problem (surface orientation is not well-defined). It has been repurposed to track the larger problem, but comments below might still refer to the original problem.
@hannobraun hannobraun added type: bug Something isn't working topic: core Issues relating to core geometry, operations, algorithms labels Feb 11, 2022
@hannobraun
Copy link
Owner Author

This bug applies to all inside walls, so also those of the spacer model.

I've looked into this some more, and I'm no longer sure that #97 is really related. Still not sure how this bug crept in (a hunch that I had about that turned out to be wrong), but I think a proper fix would involve thinking about surface orientation, and how we want to represent that concept. See #230 for some additional notes.

hannobraun added a commit that referenced this issue Feb 23, 2022
@hannobraun
Remove Edge::reverse
892e6ac 
This simplifies some code, without any loss in functionality, as far as
I can tell.

Close #151

The issue this commit addresses is nominally blocked. However, given
that #173 exists, this mechanism doesn't have its desired effect anyway.

A proper fix will likely involve a deeper look at surface orientation,
and I don't think this hack will be part of that. Given that, and that
this removal simplifies some ongoing work, I feel good about doing this
now.
@hannobraun hannobraun mentioned this issue Feb 23, 2022
Merged
@hannobraun hannobraun changed the title Inside walls of star model are not shaded correctly Surface orientation is not well-defined Mar 3, 2022
@hannobraun
Copy link
Owner Author

As I alluded to in my previous comment, this bug is just a symptom of a larger problem: That surface orientation is not well-defined. I'm starting to run into more limitations related to that, and instead of working around that in hacky ways, I've decided to just accept the incorrect shading for now, until we implement a better surface orientation concept.

I'm repurposing this issue to track that, instead of just one specific symptom. I've updated the issue title and description to reflect that.

hannobraun added a commit that referenced this issue Mar 3, 2022
@hannobraun
Fix sweeping of un-symmetrical sketches
dceeadc 
This fixes #230, while making #173 worse (now all bottom faces of
extruded shapes are black). I think this is an acceptable trade-off, as
it enables some progress with the sweep code, without requiring more
infrastructure work right away.
@hannobraun hannobraun mentioned this issue Mar 3, 2022
Merged
hannobraun added a commit that referenced this issue Mar 5, 2022
@hannobraun
Shade "back sides" the same as "front sides"
4244676 
The shader used to shade the "back side" of a face, as defined by the
vertex normal, differently than the front sides.

This minimally invasive change removes that distinction. A surface
normal is still needed for shading, to compute the angle between the
surface and the light, but the direction of that normal is ignored.

Close #173
@hannobraun hannobraun mentioned this issue Mar 5, 2022
Merged
@hannobraun
Copy link
Owner Author

I've come up with a minimally invasive change (#289) to address this issue. Instead of adding infrastructure to define surface orientation properly, I've decided to make the concept of surface orientation irrelevant, for the time being.

This is not a long-term solution, but it is a practical solution that's good enough for now.

@hannobraun hannobraun mentioned this issue Mar 14, 2022
Closed
@hannobraun hannobraun mentioned this issue Apr 11, 2022
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
topic: core Issues relating to core geometry, operations, algorithms type: bug Something isn't working
Projects
None yet
Milestone
Straight edges, flat faces, simple sk...
Development

Successfully merging a pull request may close this issue.

Shade "back sides" the same as "front sides" hannobraun/fornjot
1 participant
@hannobraun