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

Edge identity #240

Open
wants to merge 180 commits into
base: main
Choose a base branch
from
Open

Edge identity #240

wants to merge 180 commits into from

Conversation

pasbi
Copy link
Owner

@pasbi pasbi commented Apr 11, 2022

Until now, edges did not have an identity but were defined to be the connection between two unique points (instances of PathPoint).
There was a class Edge, but it was merely a descriptor of a connection of two points that could be copied around.
Such an edge descriptor is unique because PathPoints are unique per Path and because there could only be one Edge between two PathPoints within a Path.

Things become tricky when introducing joined points (without which faces would be impossible).
When PathPoints are joined, they act as the same point, i.e., they not only coincidentally have the same position, also their selection is synchronized and they are not distinguished by certain algorithms.

broken-faces

In this graph, the points 0, 2 and 3 are joined and 1 and 5 are joined.
Hence the graph exhibits two faces: 0--1--2 and 3--4--5--3.
Note that the blue face only exists if the blue path is closed, i.e., if considering the joined points 0 and 2 "the same".
This is okay in the first place.
After all, joined points were introduced to close paths.
However, if we consider 0 and 2 the same, then we cannot distinguish the two edges of the blue face anymore.

We might apply some tricks considering direction, which becomes very cumbersome quickly.

This PR proposes a fundamentally different approach:
A Path does not own a sequence of PathPoints but a sequence of Edges.
Those edges are not descriptors but they carry an identity.
Edges share ownership of two PathPoints.
That is, a PathPoint can be owned by more than one Edge, which makes the concept of joined points superfluous (implementing joined points makes a lot of noise in many places).

Roadmap:

  • implement Edge, change Path and related classes
  • make it compile again by removing invalidated code
  • reimplement the path tool
  • reimplement the serialization
  • reimplement face detection (cross fingers that it works better this time)
  • reimplement tools and actions like knife, join/disjoin points (those actions will destroy/create PathPoints now).
  • reimplement the conversion of Ellipse, Rectangle, etc. to PathObject
  • reimplement unit tests (as soon as possible)

Pascal Bies added 30 commits March 30, 2022 23:45
# Conflicts:
#	src/properties/splineproperty.cpp
#	src/properties/splineproperty.h
#	src/serializers/abstractserializer.h
#	src/serializers/jsonserializer.cpp
#	src/serializers/jsonserializer.h
That requires some re-implementation of QPainterPath features, like
`constains`.
The usage of `QPainterPath::operator-` has been replaced by
`Face::operator^` because it's easier to implement and gives the same
result if one path contains the other.
# Conflicts:
#	src/path/face.cpp
#	src/path/face.h
#	src/properties/CMakeLists.txt
#	src/properties/splineproperty.cpp
#	src/propertytypeenum.h
#	src/renderers/offscreenrenderer.cpp
#	src/variant.h
improves error messages if it's used in the wrong way.
don't expect options parameter, options are the same for all tests.
Face operator^ is principally impossible to implement because
edges don't have an identity (there might be multiple indistinguishable
edges between two points).

Therefore, PathVector::faces is simpler now. It's still not correct,
but the (now removed) complicated implementation (employing operator^)
wouldn't make it better.
Simple and wrong is better than complicated and wrong.

Fixing it properly probably requires a complete change of paradigms
(i.e., identifiable edges)
@pasbi pasbi mentioned this pull request Feb 11, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

1 participant