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

Consolidate redundant references to GlobalCurve #1605

Closed
4 tasks done
hannobraun opened this issue Feb 21, 2023 · 1 comment · Fixed by #1617
Closed
4 tasks done

Consolidate redundant references to GlobalCurve #1605

hannobraun opened this issue Feb 21, 2023 · 1 comment · Fixed by #1617
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

hannobraun commented Feb 21, 2023

(This issue is part of a larger cleanup effort. See #1589.)

Currently, there are references to GlobalCurve in two different places: Curve and GlobalEdge. Both of those objects are referenced by HalfEdge. An instance of HalfEdge is only valid, if the GlobalCurve being referenced is the same one in both places. This is verified by a validation check.

This is another part of the object graph that is redundant and can be simplified. I have the following in mind:

  1. Remove the reference to GlobalCurve from Curve. (Remove reference to GlobalCurve from Curve #1607)
    This reference is currently used in the approximation code, to ensure that coincident Curves result in the same approximation. It shouldn't be too much of a problem to adjust this, maybe by moving that approximation caching logic up to the HalfEdge level.
  2. Remove GlobalCurve. (Merge GlobalCurve into GlobalEdge #1610)
    Once the previous step is done, the only remaining purpose of GlobalCurve (unless I'm forgetting something) is to track its identity, so we can uniquely identify edges that are supposed to be the same. This role is redundant, since GlobalEdge already provides that. At this point, it should be possible to remove GlobalCurve.
  3. Merge Curve into HalfEdge. (Merge Curve into HalfEdge #1616)
    At this point, Curve only contains a SurfacePath and nothing more, meaning its further existence as a separate object serves no purpose. It can just be inlined into HalfEdge.
  4. Rename SurfacePath to Curve. (Rename SurfacePath to Curve #1617)
    The previous item frees up that name, so it can be taken over by the more awkwardly named SurfacePath.

I'm going to start working on this shortly, as part of my larger simplification effort (#1589).

@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 21, 2023
@hannobraun hannobraun self-assigned this Feb 21, 2023
@hannobraun
Copy link
Owner Author

I've recognized some more opportunities while I started working on this. Added two more items to the list in the issue description.

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