Skip to content

Added Jarvis Algorithm to compute convex hull #1027

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.

Sign up for GitHub

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

Jump to bottom
Merged
Panquesito7 merged 7 commits into from
Aug 17, 2020
Merged

Added Jarvis Algorithm to compute convex hull #1027

Panquesito7 merged 7 commits into from
Aug 17, 2020

Conversation

@rishabh-997
Copy link
Contributor

@rishabh-997 rishabh-997 commented Aug 14, 2020 •
edited by Panquesito7
Loading

Description of Change

Closes #1026.

I implemented the Jarvis algorithm which, for a set of points in the plane, creates a convex hull of the points in the smallest convex polygon that contains all the points of it.

Checklist

  • Added description of change
  • Added file name matches File name guidelines
  • Added tests and example, the test must pass
  • Added documentation so that the program is self-explanatory and educational - Doxygen guidelines
  • Relevant documentation/comments is changed or added
  • PR title follows semantic commit guidelines
  • Search previous suggestions before making a new one, as yours may be a duplicate.
  • I acknowledge that all my contributions will be made under the project's license.

Notes:

@Panquesito7 Panquesito7 added automated tests are failing Do not merge until tests pass enhancement New feature or request labels Aug 14, 2020
@rishabh-997
Copy link
Contributor Author

rishabh-997 commented Aug 14, 2020

This implements #1026 . Can you review @Panquesito7. I have tried my best to align it with all the guidelines and squashed all commit s to one :)

@Panquesito7 Panquesito7 removed the automated tests are failing Do not merge until tests pass label Aug 14, 2020
Panquesito7
Panquesito7 requested changes Aug 14, 2020
View reviewed changes
@rishabh-997
Copy link
Contributor Author

rishabh-997 commented Aug 14, 2020

Thanks for the review, I have updated the PR with your suggested changes.

Panquesito7
Panquesito7 requested changes Aug 14, 2020
View reviewed changes
Panquesito7
Panquesito7 requested changes Aug 14, 2020
View reviewed changes
Copy link
Member

@Panquesito7 Panquesito7 left a comment

Choose a reason for hiding this comment

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

For enhanced formatting and code, please enable GitHub Actions in your fork of this repository.
Anyways, though, how does the program work? I have compiled it and it gives no output.

@Panquesito7 Panquesito7 added the requested changes changes have been requested label Aug 14, 2020
@rishabh-997
Copy link
Contributor Author

rishabh-997 commented Aug 14, 2020

Anyways, though, how does the program work? I have compiled it and it gives no output.

We start from the leftmost point (or point with minimum x coordinate value) and we keep wrapping points in counterclockwise direction with the use of orientation() function. Next point is selected as the point that beats all other points at counterclockwise orientation, i.e., next point is q if for any other point r, we have “orientation(p, q, r) = counterclockwise” ,ie, 1....

I have just added test which asserts the expected value without printing anything. Should i display the hull as well in the function ??? You can add breakpoint to see the actual value returned

@Panquesito7
Copy link
Member

Panquesito7 commented Aug 14, 2020 •
edited
Loading

hull as well in the function

No, I think I got confused.
Keep it as-is, it's good. 🙂

You can show what will it output in the first comment block.
Thank you for the explanation, though.

Panquesito7
Panquesito7 reviewed Aug 14, 2020
View reviewed changes
Copy link
Member

@Panquesito7 Panquesito7 left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Looks really refined and good, excellent work! 👍
Now, let's output if the tests have passed, or not.

Panquesito7
Panquesito7 reviewed Aug 15, 2020
View reviewed changes
@rishabh-997
Added Jarvis Algorithm to compute convex hull
c6c5d81 
Fixed code formatting

Fixed code formatting

Fixed code formatting

Added Jarvis algorithm to compute convex hull

Capitalized class name

Added requested changes

Added requested changes

Added jarvis algorithm

Added jarvis algorithm to compute convex hull
@rishabh-997
Copy link
Contributor Author

rishabh-997 commented Aug 15, 2020 •
edited
Loading

Looks really refined and good, excellent work!

Thanks, I have added all the changes you suggested. Will be more careful with the guidelines in the next PR.

@ridhishjain-zepto
Copy link

ridhishjain-zepto commented Aug 15, 2020 •
edited
Loading

@rishabh-997 please make sure to add your implementation link in the repo directory, here after it gets merged!

Nice work though!!

Panquesito7
Panquesito7 reviewed Aug 16, 2020
View reviewed changes
rishabh-997 and others added 2 commits August 16, 2020 22:12
@rishabh-997 @Panquesito7
Update geometry/jarvis_algorithm.cpp
686ceb3 
Co-authored-by: David Leal <halfpacho@gmail.com>
@rishabh-997 @Panquesito7
Update geometry/jarvis_algorithm.cpp
ff4792d 
Co-authored-by: David Leal <halfpacho@gmail.com>
Panquesito7
Panquesito7 reviewed Aug 16, 2020
View reviewed changes
Copy link
Member

@Panquesito7 Panquesito7 left a comment

Choose a reason for hiding this comment

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

Please consider enabling GitHub Actions in your fork of this repository for enhanced code and formatting.

@rishabh-997 @Panquesito7
Update geometry/jarvis_algorithm.cpp
bfed4c8 
Co-authored-by: David Leal <halfpacho@gmail.com>
@rishabh-997
Copy link
Contributor Author

rishabh-997 commented Aug 16, 2020

enabling GitHub Actions in your fork

Yes sure, i will look how to add this and make sure to use it in my next PR. Thanks :)

Panquesito7
Panquesito7 approved these changes Aug 16, 2020
View reviewed changes
Copy link
Member

@Panquesito7 Panquesito7 left a comment

Choose a reason for hiding this comment

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

Excellent work; LGTM. 👍
Thank you for your contribution.

@kvedala please review as well. Thank you. 🙂

@Panquesito7 Panquesito7 added approved Approved; waiting for merge and removed requested changes changes have been requested labels Aug 16, 2020
@Panquesito7 Panquesito7 requested a review from kvedala August 16, 2020 17:03
kvedala
kvedala requested changes Aug 16, 2020
View reviewed changes
Copy link
Collaborator

@kvedala kvedala left a comment

Choose a reason for hiding this comment

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

Looks great. Some doc corrections and a suggestion.

geometry/jarvis_algorithm.cpp
*/
class Convexhull {
public:
std::vector<Point> points;
Copy link
Collaborator

@kvedala kvedala Aug 16, 2020

Choose a reason for hiding this comment

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

should the variables points and size be private?

Copy link
Collaborator

@kvedala kvedala Aug 16, 2020

Choose a reason for hiding this comment

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

isnt size redundant, given that the value is already present in the std::vector?

@rishabh-997 rishabh-997 requested a review from kvedala August 16, 2020 21:18
kvedala
kvedala reviewed Aug 16, 2020
View reviewed changes
@kvedala
Copy link
Collaborator

kvedala commented Aug 16, 2020

👍

@Panquesito7 Panquesito7 merged commit d8b4da6 into TheAlgorithms:master Aug 17, 2020
@kvedala
Copy link
Collaborator

kvedala commented Aug 17, 2020

@Panquesito7 do squash merge.

@Panquesito7
Copy link
Member

Panquesito7 commented Aug 17, 2020

@kvedala Sorry for the confusions, I will in the future. Thanks! 🙂

@rishabh-997 rishabh-997 mentioned this pull request Aug 17, 2020
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@kvedala kvedala kvedala approved these changes

@Panquesito7 Panquesito7 Panquesito7 approved these changes

Assignees

No one assigned

Labels

approved Approved; waiting for merge enhancement New feature or request

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

[FEATURE] Implement Jarvis algorithm to compute convex hull

4 participants

@rishabh-997 @Panquesito7 @ridhishjain-zepto @kvedala