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

Redesign Shape trait #176

Closed
hannobraun opened this issue Feb 11, 2022 · 0 comments · Fixed by #254
Closed

Redesign Shape trait #176

hannobraun opened this issue Feb 11, 2022 · 0 comments · Fixed by #254
Labels
topic: core Issues relating to core geometry, operations, algorithms type: development Work to ease development or maintenance, without direct effect on features or bugs

Comments

@hannobraun
Copy link
Owner

hannobraun commented Feb 11, 2022

Shape is a trait, used inside the CAD kernel, that is implemented for all shape types from the fj crate, providing them with an API that the rest of the CAD kernel can interact with. It currently looks like this (simplified version):

pub trait Shape {
    fn bounding_volume(&self) -> AABB;
    fn faces(&self) -> Faces;
    fn edges(&self) -> Edges;
    fn vertices(&self) -> Vertices;
}

Faces, Edges, and Vertices are self-contained structs that are constructed in the respective method. This works well enough for now, but this design is currently reaching its limits.

Issues due to floating point inaccuracy are a constant concern. See #78 for some context. Just be aware that the situation has continued to develop (and improve!) since then. What I wrote there, no longer reflects my latest insights.

I remain convinced though, that these issues are best dealt with at an architectural level. Instead of being really careful everywhere we're dealing with vertices/edges/faces, I'd rather be very careful in a few select places, where vertices/edges/faces are constructed, so the rest of the code doesn't have to deal with this problem. Vertex, and specifically Vertex::create_at, are a reflection of that philosophy.

Current problems

There is at least one problematic piece of code that doesn't uphold the requirements of Vertex::create_at:
https://github.com/hannobraun/Fornjot/blob/f5204f6a714d9e01f238ec2459366dede9ed7adc/src/kernel/shapes/sketch.rs#L67-L91

This is the implementation of Shape::vertices for fj::Sketch. It creates vertices when called. This method is also called by the Shape::edges and (indirectly) by Shape::faces implementations, meaning the same vertices are being re-created multiple times, which is expressly forbidden by Vertex::create_at. I don't think this is causing any problems right now, but those rules are there for a reason: to prevent subtle problems from creeping in.

This can't be fixed within the current structure, except by adding some kind of caching to every Shape implementation that is going to deal with this problem.

Upcoming problems

When I started writing this issue, I was convinced that I was going to need to address this before making the next steps towards #97. Now that I've written and thought about this more, I'm no longer sure. I might be able to get away with not addressing this for a while.

But in any case, vertices/edges/faces are really interdependent. I don't think we can get away with those standalone structs indefinitely, while upholding the rules required to protect against floating point accuracy issues.

Proposed solution

I have the following solution in mind:

  1. Rename Shape to ToShape.
  2. Add a new struct, named Shape, that initially has three fields for Vertices/Edges/Faces. It can be refined later, as needed, but this is enough for now.
  3. Merge the vertices/edges/faces methods of ToShape into a single to_shape method that returns Shape.

I'm not sure what to do about BoundingVolume. It's a bit weird to leave it in ToShape. Long-term, it should be possible to move it to Shape, but for now, I think we need shape-specific implementations to keep it working correctly. It can just stay where it is until this becomes inconvenient, or a better solution presents itself.

@hannobraun hannobraun added type: development Work to ease development or maintenance, without direct effect on features or bugs topic: core Issues relating to core geometry, operations, algorithms labels Feb 11, 2022
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 type: development Work to ease development or maintenance, without direct effect on features or bugs
Projects
None yet
Development

Successfully merging a pull request may close this issue.

1 participant