-
Notifications
You must be signed in to change notification settings - Fork 19
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
Simplify DCEL label management #469
Conversation
In the DCEL data structure, there are labels on the vertex records, (half) edge records, and face records. These labels keep track information such as which input geometry the record originates from and whether it will appear in the output geometry. The original approach to labelling was to use the same data structure for all record types. The label had two booleans per operand. The first boolean determined whether the record would appear in the final output, and the second boolean would show whether the first boolean was populated or not. This was essentially tri-state logic. The trouble with this approach was that the DCEL algorithm doesn't know whether each record should appear in the output geometry right up until the last stage. The labels are used in the earlier stages of the algorithm to record other facts, such as whether or not the records appear in the input geometry. The semantics of the fields subtly change over the course of the algorithm, making the code hard to understand and maintain. The new approach (implemented by this PR) changes how label management is performed. Instead of having a single labels data structure, the fields are slightly different for vertex, edge, and face records. This is to account for the differences in information available when processing vertexes vs. edges vs. faces. The number of fields is also expanded, so that concepts like "is this record explicitly present in the input geometry's control points" are separate from concepts like "is this record present in the input geometry but not explicitly in its control points". The populated fields are also removed, and tracked separately in each algorithm that modifies the labels.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
At a high level, from what I understand, the aim of this PR is to improve how we model the state of a record by tailoring the state data (labels) to be more specific to the record type (vertex, half edge or face). Is that correct?
@@ -13,7 +13,7 @@ func Union(a, b Geometry) (Geometry, error) { | |||
if b.IsEmpty() { | |||
return a, nil | |||
} | |||
g, err := setOp(a, b, selectUnion) | |||
g, err := setOp(a, or, b) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This reads much more nicely 👍🏼
Yes, that's right. The original implementation reused the labels struct or each of the 3 record types which saved a bit of code but was a fairly inaccurate/confusing modelling. |
Thanks for the review! 😁 |
Description
commit 45ccc4b
Author: Peter Stace peterstace@gmail.com
Date: Mon Nov 7 20:19:20 2022 +1100
Check List
Have you:
Added unit tests? No, relies on existing.
Add cmprefimpl tests? (if appropriate?) No, relies on existing.
Updated release notes? (if appropriate?) No, relies on existing.
Related Issue
Benchmark Results
Click to expand