Skip to content

Conversation

@mastep84
Copy link
Member

Added function 'SelectEdgeByProjectedCoordinate' selecting the edge for which the specified coordinate is closest to its perpendicular projection onto this edge.

…or which the specified coordinate is closest to its perpendicular projection onto this edge.
@sreiter
Copy link
Member

sreiter commented Dec 30, 2020

What you're caclulating is not necessarily the distance to the projected point. If the projection lies outside of a line segment, the distance to the closest endpoint of the line segment (edge) is computed. The naming is somewhat misleading and probably unnecessarily complex, I guess.

Why not simply SelectClosestEdge?

maxDeviationAngle, selectFlipped);
}

Edge* SelectEdgeByMinPerpendicularDistance(
Copy link
Member

Choose a reason for hiding this comment

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

As commented somewhere, this is not accurate - if the projection of the given coordinate on an edge lies outside of that edge, the distance to the closest endpoint is considered.

return NULL;

EdgeIterator iter = grid.begin<Edge>();
Edge* bestEdge = *iter;
Copy link
Member

@sreiter sreiter Dec 30, 2020

Choose a reason for hiding this comment

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

maybe initialize with NULL and add a check below inside the loop:

if (bestEdge == NULL || dist < bestDist)
{...}

You could then use a for loop and avoid code duplication. You could also remove the empty-check above.

…ClosestEdge' and streamlining the code according to the suggestions made.
@mastep84
Copy link
Member Author

mastep84 commented Jan 5, 2021

Dear Sebastian,

thank you for your review -- indeed, the naming was somewhat misleading. I have applied modifications according to your suggestions streamlining the code.

Best regards,
Martin

@sreiter sreiter merged commit df97012 into UG4:master Mar 20, 2021
@sreiter
Copy link
Member

sreiter commented Mar 20, 2021

Dear Martin,

I'm sorry that it took so long to merge your request. Thanks for taking the time to consider my comments :)

Best,
Sebastian

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.

2 participants