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

Added FAST feature detector by Edward Rosten #604

Open
wants to merge 11 commits into
base: develop
Choose a base branch
from

Conversation

Sayan-Chaudhuri
Copy link

@Sayan-Chaudhuri Sayan-Chaudhuri commented May 1, 2021

Description

This pull request adds the FAST feature detector which is used for corner detection in real time scenarios

  1. Fusing points and lines for high performance tracking.
  2. Machine learning for high-speed corner detection.
  3. Faster and better: A machine learning approach to corner detection

References

https://www.edwardrosten.com/work/fast.html

Tasklist

  • Add test case(s)
  • Ensure all CI builds pass
  • Review and approve

This is the famous Fast feature detector used for detecting feature points in real time videos and images.
Reference papers
1.Fusing points and lines for high performance tracking.
2.Machine learning for high-speed corner detection.
3.Faster and better: A machine learning approach to corner detection
Added FAST feature detector by Edward Rosten
test images also included
@Sayan-Chaudhuri
Copy link
Author

Would love to have feedback from @lpranam @mloskot @codejaeger if you find time in your busy schedule to review this PR.
I have tried to maintain the following things based on feedback in earlier pull requests.
1.Code is short
2.Consistent formatting for improved readability
3.Did not mix upper and lower case in file names

Copy link
Member

@lpranam lpranam left a comment

Choose a reason for hiding this comment

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

First Thank you for your time and contributions. I am yet to have a look into the actual algorithm had a glace and found problems with code.

  1. file name fast does not make any sense in absolute context
  2. we do not use camel case anywhere in the naming
  3. spaces and newlines are inconsistent
  4. template parameter names are single later which makes it hard to understand and impossible to review

maybe try formatting code with clang-format from #596 or with the clang-format provided in the example. Most of the styling problem will be solved with it just have to take care of the naming conventions. You may wanna have a look into code base for names

1.Used Clang format 12 to format the header file
2.Single letter template parameters renamed
3.Camelcases removed.
4.Filename changed to a meaningful one
@Sayan-Chaudhuri
Copy link
Author

@lpranam I have tried to follow your advice and made the changes .I would request you and @mloskot @codejaeger to kindly review it if you find time amidst your busy schedule.

@Sayan-Chaudhuri
Copy link
Author

The number of code lines increased due to formatting

@lpranam
Copy link
Member

lpranam commented May 2, 2021

@Sayan-Chaudhuri no one cares about number of lines increases due to adding necessary spaces and formatting. when in past I said that no one would require 500K lines that was because it 500K lines of code no one counts while space. White space is needed.

@Sayan-Chaudhuri
Copy link
Author

Actually I mentioned it because initially I was very happy to keep the code concise and readable. But then the lines increased and so I felt bad.. :) .

@mloskot
Copy link
Member

mloskot commented May 2, 2021

@Sayan-Chaudhuri I believe we have discussed the initial issues pointed out by @lpranam above during review of #597 (review)

Please, try to avoid similar issues over and over again. This will save time to us all!

Comment on lines 72 to 75
template <typename srcview>
std::ptrdiff_t
calculate_score(srcview& src, int i, int j, std::vector<point_t>& points, int threshold)
{
Copy link
Member

Choose a reason for hiding this comment

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

Is this formatting with indented calculate_score by the clang-format? /cc @lpranam

Copy link
Member

Choose a reason for hiding this comment

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

@lpranam I will answer myself, no, it is not clang-format output.

Your .clang-format generates expected output:

template <typename SrcView>
std::size_t calculate_score(
    SrcView const& src,
    std::size_t i,
    std::size_t j,
    std::vector<point_t>& points,
    std::size_t threshold)
{

Copy link
Author

Choose a reason for hiding this comment

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

I used the .clang-format file given by @lpranam in the above links. But i was able to install clang-13 and not 12. May be that's the reason. Going to look into this matter again and update. Thanks for verifying @mloskot .

Copy link
Member

Choose a reason for hiding this comment

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

I'd suggest to use clang-format 12 but
I'd also expect clang-format 13 to give output close to clang-format 12

Copy link
Author

Choose a reason for hiding this comment

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

I was initially unable to get clang-format 12 but now I have been able to do so.

test/core/image_processing/fast_feature_detector.cpp Outdated Show resolved Hide resolved
@Sayan-Chaudhuri
Copy link
Author

@mloskot Yeah, I am noting down every feedback and trying hard so as not to avoid them. The code is entirely written by me because from our discussion , I understand that copied code is wrong due to several factors. Regarding the blank lines as separator, I actually didnt made any changes after i formatted the code with clang. Ok, so I shall surely add the separators. From your feedback of PR#597, I thought it was bad to mix upper/lower case in filenames. Will keep this mistake in mind. Thanks for taking out time to provide the feedback.

…iable type

1. No mixing of upper and lower cases. all names in lowercase
2. Inserted blank lines between code sections to improve readability
4. Changed all west const to east const
5. Template parameter types changed to camelcase
6.Single variable per line
7.loop variable types changed to std::ptrdiff_t , other  integer types changed to std::size_t wherever possible
8. Inserted function description
@Sayan-Chaudhuri
Copy link
Author

@lpranam @mloskot I apologise sincerely for repetitive mistakes. I have looked into the codebase again, read your feedback and have tried to incorporate the necessary changes.

@mloskot mloskot added the cat/feature New feature or functionality label May 4, 2021
@mloskot mloskot mentioned this pull request May 6, 2021
3 tasks
@Sayan-Chaudhuri
Copy link
Author

@mloskot @lpranam Apologies for the mistakes committed in my previous commit. I have reread the contributing guidelines as instructed by you, and have tried to incorporate each and every instruction from the guidelines. I have doubt over the below mentioned points in the contributing guidelines
1)All public headers should be placed in boost/gil/ or boost/gil//.

2)All non-public headers should be placed boost/gil/detail or boost/gil//detail.

As per my understanding, all the header files that will not be used by a library user should be put in detail folder. But in the already existing header files in the library, I do not find any such convention being followed e.g histogram_equalization.hpp. Also , there is no detail folder inside the image_processing subfolder of boost/gil. So, if you can kindly advice the course of action for this.

I understand repeated mistakes from my side is hindering further course of action from your side regarding the PR. I hqve checked my code several times against the guidelines. I hope that this time, my mistakes will not be hindering the review process. I would kindly request you to have a relook at my code if you find time amidst your busy schedule.

Copy link
Member

@mloskot mloskot left a comment

Choose a reason for hiding this comment

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

The chessboard(2).png file seems not used anywhere.
It should be removed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cat/feature New feature or functionality
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants