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

In AdaptiveGridRouting, if there are several connection points with… #933

Merged
merged 2 commits into from
Dec 29, 2022

Conversation

DmytroMuravskyi
Copy link
Contributor

@DmytroMuravskyi DmytroMuravskyi commented Dec 27, 2022

… the same cost - choose one that is closer to the trunk.

BACKGROUND:

  • While testing routing pattern that consist of several parallel lines with many inputs each, it was discovered that routing can choose any paths between lines since they produce the same tree sum. This was not consistent since numeric noise decided which one is shortest.

DESCRIPTION:

  • If there are several connection points with the same cost - choose one that is closer to the trunk.

TESTING:

  • Added new AdaptiveGraphRoutingEqualTransitions test that covers this case.

FUTURE WORK:

REQUIRED:

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

COMMENTS:

  • AdaptiveGraphRoutingEqualTransitions uses more compact way where Vector3 name is often skipped.

This change is Reviewable

… the same cost - choose one that is closer to the trunk.
@wynged wynged self-requested a review December 27, 2022 18:48
Copy link
Member

@wynged wynged left a comment

Choose a reason for hiding this comment

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

Reviewed 3 of 3 files at r1, all commit messages.
Reviewable status: 1 change requests, 0 of 1 approvals obtained (waiting on @DmytroMuravskyi)


Elements/src/Spatial/AdaptiveGrid/AdaptiveGraphRouting.cs line 370 at r1 (raw file):

                    bool betterCost = order == TreeOrder.FurthestToClosest ?
                        localBestCost > bestCost : localBestCost < bestCost;
                    if (sameCost || betterCost)

can we logically seperate the sameCost behavior here from the better cost checking? Like do just the if same cost block, and inside compute the localCostToTrunk, and then if we get out of that compute the better cost?


Elements/src/Spatial/AdaptiveGrid/AdaptiveGraphRouting.cs line 403 at r1 (raw file):

            {
                var leafNode = leafsToTrunkTree[leaf];
                if(leafNode.Leafs.Any())

comment for what this check is doing?

Copy link
Contributor Author

@DmytroMuravskyi DmytroMuravskyi 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 @wynged)


Elements/src/Spatial/AdaptiveGrid/AdaptiveGraphRouting.cs line 370 at r1 (raw file):

Previously, wynged (Eric Wassail) wrote…

can we logically seperate the sameCost behavior here from the better cost checking? Like do just the if same cost block, and inside compute the localCostToTrunk, and then if we get out of that compute the better cost?

Both cases lead to the exactly same code and both need localCostToTrunk to store it for the future. The only difference is that we need to skip if sameCost && localCostToTrunk >= bestCostToTrunk
I think code is well structured: minimum repeating code and localCostToTrunk is not calculated if not needed.


Elements/src/Spatial/AdaptiveGrid/AdaptiveGraphRouting.cs line 403 at r1 (raw file):

Previously, wynged (Eric Wassail) wrote…

comment for what this check is doing?

Done.

Copy link
Member

@wynged wynged 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 1 files at r2, all commit messages.
Reviewable status: :shipit: complete! 1 of 1 approvals obtained


Elements/src/Spatial/AdaptiveGrid/AdaptiveGraphRouting.cs line 370 at r1 (raw file):

Previously, DmytroMuravskyi wrote…

Both cases lead to the exactly same code and both need localCostToTrunk to store it for the future. The only difference is that we need to skip if sameCost && localCostToTrunk >= bestCostToTrunk
I think code is well structured: minimum repeating code and localCostToTrunk is not calculated if not needed.

ok, works for me!

@wynged wynged merged commit 0f90f53 into hypar-io:master Dec 29, 2022
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