Skip to content

Fix bugs in closest_point_on_lines#63

Merged
WhyPenguins merged 3 commits intothoth-tech:t2-2024from
matt439:closest_point_on_lines
Sep 26, 2024
Merged

Fix bugs in closest_point_on_lines#63
WhyPenguins merged 3 commits intothoth-tech:t2-2024from
matt439:closest_point_on_lines

Conversation

@matt439
Copy link

@matt439 matt439 commented Aug 26, 2024

Description

I attempted to use the closest_point_on_lines function in my circle_triangle_intersects function and found that it behaved strangely. Further investigation revealed two major bugs. First, despite result being initialised, it is never returned. Second, min_dist is initialised as -1, which means that the comparison min_dist > dst will always resolve to false as dst must be a positive number returned from point_point_distance. The special case when lines.size() == 0 is handled correctly by returning 0,0 with line_idx=0, however I felt that an early return for this case would make the function easier to understand.

It is possible that the incorrect behavior of this function has lead to workarounds being used in projects. Therefore, I believe that this is a breaking change. Despite this, I believe that the functionality must be rectified for the sake of correctness.

Type of change

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as
    expected)
  • Documentation (update or new)

How Has This Been Tested?

I added tests to sktest which show the function working in real-time. They indicate the position of the closest point, as well as which individual line contains the closest point. I also added a few numerical tests which print their results to the console.

Testing Checklist

  • Tested with sktest
  • Tested with skunit_tests

Checklist

  • My code follows the style guidelines of this project
  • I have performed a self-review of my own code
  • I have commented my code in hard-to-understand areas
  • I have made corresponding changes to the documentation
  • My changes generate no new warnings
  • I have requested a review from peers on the Pull Request

Copy link

@Liquidscroll Liquidscroll left a comment

Choose a reason for hiding this comment

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

Looks good to me!

I pulled these changes and ran the tests locally. Great work on the interactive testing! Love it! As far as I can tell it is working correctly.

Copy link

@hugglesfox hugglesfox left a comment

Choose a reason for hiding this comment

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

Looks good. Great job on the testing

Copy link

@WhyPenguins WhyPenguins left a comment

Choose a reason for hiding this comment

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

Great work! 😄

@WhyPenguins WhyPenguins changed the base branch from develop to t2-2024 September 26, 2024 17:17
@WhyPenguins WhyPenguins merged commit 1d66eb7 into thoth-tech:t2-2024 Sep 26, 2024
@matt439 matt439 deleted the closest_point_on_lines branch November 1, 2024 01:36
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.

4 participants