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

Bound all edges by vertices #1020

Closed
hannobraun opened this issue Aug 30, 2022 · 2 comments · Fixed by #1057
Closed

Bound all edges by vertices #1020

hannobraun opened this issue Aug 30, 2022 · 2 comments · Fixed by #1057
Assignees
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

Edges are defined by two things:

  • The (infinite) curve they are on.
  • The vertices that bound the (finite) edge on that curve.

Except, this is not completely true. There are also continuous edges, which are edges that connect to themselves and don't have any vertices to bound them. They can only exist on continuous curves (curves that connect to themselves; right now only circles are supported).

This exception causes quite a few problems:

I believe that all of these problems can be solved by abolishing the concept of continuous curves (and, by extension, continuous surfaces and faces) and making sure that all Edges are bound by vertices (and, by extension, all faces are bound by edges). This promises a lot of simplification, which would pay dividends (in saved work) pretty quickly.

This isn't a new idea. In fact, I believe this is a pretty normal way to do CAD kernels, and I mentioned this previously in #250. I didn't want to do it earlier, because a) I'm a bit of an idiot, and b) I didn't see a clear way to do this. The kernel data structures have improved a lot over the last months though (#691 was a big one, in this context), so it should now be possible to implement this relatively easily.

Over the last week or so, I've been working on this, and I've made big strides towards making it possible, by updating the approximation code to support this new notion of edges (#1003, #1006, #1011, #1012, #1013). I also have a work-in-progress branch that starts making the actual change to Edge (#1019), but unfortunately this causes issues in the sweep algorithm.

For now, I will turn my attention to other things, but I expect that I'll have the opportunity to address this soon. The sweep algorithm is also blocking #993, which in turn is blocking my top priority, boolean operations (#42, #43, #44). Once the sweep algorithm is sorted out, this will hopefully become easy to finish.

@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 Aug 30, 2022
@hannobraun
Copy link
Owner Author

I forgot to update this issue: I've been working on this for about a week. See #993 (comment) for context.

@hannobraun hannobraun self-assigned this Sep 7, 2022
@hannobraun
Copy link
Owner Author

My WIP implementation in #1019 is pretty much finished, but there's a bug in the approximation code that causes invalid triangle meshes to be created, for some models. The root cause for that is a reversed curve that generates a slightly different approximation than its non-reversed but coincident counterpart.

The solution here is to never revert curves (I talked a bit about that in #993 (comment)). Unfortunately, we can't start doing that until this issue is addressed, so we have a bit of a dependency loop. One way to address that, would be to put both things into the same pull request, but that would include addressing #695, which is a non-trivial amount of work. This would make the pull request much bigger than I feel comfortable with.

I'm currently looking for temporary hacks that can paper over the issue. This would allow #1019 to get merged, and pave the way for fixing the problem properly.

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