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

Fix network region recognition. #925

Merged
merged 16 commits into from
Dec 8, 2022
Merged

Fix network region recognition. #925

merged 16 commits into from
Dec 8, 2022

Conversation

ikeough
Copy link
Contributor

@ikeough ikeough commented Dec 2, 2022

BACKGROUND:
While creating networks of walls in Hypar, I attempted to recognize closed areas as rooms by using Newtork.FindAllClosedRegions. I found for the shape pictured that the red cell would not be found. This was because the traversal algorithm was previously visiting all the nodes in such a way as to ignore 4 and 5 as having been previously traversed, resulting in an unclosed region.

image

DESCRIPTION:
This PR adds a new traversal method, Network.TraverseLeftWithoutLeaves, and uses this traversal method in the Network.FindAllClosedRegions method. This new traversal method ignores leaf nodes, both external to the graph, and internal to a cell, resulting in less traversals of unclosed paths. It reliably "turns left" resulting in proper closed regions.

TESTING:

  • See the NetworkTests.FourQuadrantsWithPointerFindsCorrectNumberOfClosedRegions test.

FUTURE WORK:

  • Network region recognition still does not work on rooms with a concave shape.

REQUIRED:

  • All changes are up to date in CHANGELOG.md.

This change is Reviewable

@ikeough
Copy link
Contributor Author

ikeough commented Dec 2, 2022

Fixes #900.

@ikeough ikeough requested review from andrewheumann and katehryhorenko and removed request for andrewheumann December 2, 2022 06:12
Copy link
Member

@andrewheumann andrewheumann left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Works great except for a couple cases, which can be left for future work.

You've already identified that Concave shapes are an issue — this could be just another manifestation of the same problem, but this will fail to find both regions:

var lines = new List<Line> {
    new Line((10.00, 0.00), (10.00, 10.00)),
    new Line((10.00, 10.00), (-0.00, 10.00)),
    new Line((-0.00, 10.00), (0.00, 0.00)),
    new Line((0.00, 0.00), (10.00, 0.00)),
    new Line((-2.00, 6.00), (12.00, 6.00)),
    new Line((5.00, -2.00), (4.00, 1.00)),
    new Line((4.00, 1.00), (6.00, 2.00)),
    new Line((6.00, 2.00), (5.00, 4.00)),
};

This one misses a region, returning 4 instead of 5, even though they should all be convex:

var lines = new List<Line> {
    new Line((10.00, 0.00), (10.00, 10.00)),
    new Line((10.00, 10.00), (-0.00, 10.00)),
    new Line((-0.00, 10.00), (0.00, 0.00)),
    new Line((0.00, 0.00), (10.00, 0.00)),
    new Line((-2.00, 6.00), (12.00, 6.00)),
    new Line((-2.96, -0.16), (2.13, 2.51)),
    new Line((2.13, 2.51), (7.81, 2.51)),
    new Line((7.81, 2.51), (13.11, 0.28)),
    new Line((7.81, 2.51), (11.60, 4.88)),
    new Line((2.13, 2.51), (-1.46, 4.62)),
};

Reviewed 1 of 2 files at r1, 2 of 2 files at r2, all commit messages.
Reviewable status: 1 change requests, 0 of 1 approvals obtained (waiting on @ikeough and @katehryhorenko)


Elements/src/Search/Network.cs line 555 at r2 (raw file):

        /// <summary>
        /// Traverse a network following the left-most candidate edge relative to the current edge.
        /// leaf nodes.

this comment was maybe unfinished?


Elements/src/Search/Network.cs line 559 at r2 (raw file):

        /// <param name="traversalData">Data about the current step of the traversal.</param>
        /// <param name="allNodeLocations">A collection of all node locations in the network.</param>
        /// <param name="visitedEdges">A collection of previously visited edge.</param>

previously visited edges


Elements/src/Search/Network.cs line 593 at r2 (raw file):

                var localEdge = (allNodeLocations[e] - allNodeLocations[traversalData.currentIndex]).Unitized();
                var angle = 180 - baseEdge.PlaneAngleTo(localEdge);

Could be future work, but this embeds an assumption that the network is 2D and in the XY plane. Can we at least leave a comment w/ a TODO to accept a normal or otherwise support non-planar networks?

Copy link
Member

@andrewheumann andrewheumann left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewable status: 1 change requests, 0 of 1 approvals obtained (waiting on @ikeough and @katehryhorenko)


Elements/src/Search/Network.cs line 498 at r2 (raw file):

        /// Traverse a network following the smallest plane angle between the current
        /// edge and the next candidate edge.
        /// leaf nodes.

unfinished comment?

Copy link
Contributor Author

@ikeough ikeough left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewable status: 1 change requests, 0 of 1 approvals obtained (waiting on @andrewheumann and @katehryhorenko)


Elements/src/Search/Network.cs line 498 at r2 (raw file):

Previously, andrewheumann wrote…

unfinished comment?

Done.


Elements/src/Search/Network.cs line 555 at r2 (raw file):

Previously, andrewheumann wrote…

this comment was maybe unfinished?

Done.


Elements/src/Search/Network.cs line 559 at r2 (raw file):

Previously, andrewheumann wrote…

previously visited edges

Done.


Elements/src/Search/Network.cs line 593 at r2 (raw file):

Previously, andrewheumann wrote…

Could be future work, but this embeds an assumption that the network is 2D and in the XY plane. Can we at least leave a comment w/ a TODO to accept a normal or otherwise support non-planar networks?

I've added a clarification to the Network.FromSegmentableItems method's docs. That's where the assumption is embedded first as the algorithm uses a line sweep on planar line segments to construct the network.

@ikeough
Copy link
Contributor Author

ikeough commented Dec 3, 2022

@andrewheumann
image
image
image

@ikeough ikeough removed the request for review from katehryhorenko December 5, 2022 22:44
Copy link
Member

@andrewheumann andrewheumann left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed 1 of 2 files at r3, 1 of 1 files at r4, all commit messages.
Reviewable status: :shipit: complete! 1 of 1 approvals obtained

@ikeough ikeough merged commit 67a934c into master Dec 8, 2022
@ikeough ikeough deleted the network-regions branch December 8, 2022 17:07
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

2 participants