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

[filters] Add a filter accepting a functor to reduce boiler plate code for simple filters #3890

Merged
merged 33 commits into from
Jul 9, 2020

Conversation

kunaltyagi
Copy link
Member

@kunaltyagi kunaltyagi commented Apr 8, 2020

Currently modification of filter needs sub-classing of Filter or FilterIndices and quite some boiler plate code which can be reduced by working with a simple functor.

Please check the PR for a WIP implementation of a class to reduce code needed for a custom class to a simple functor which checks point-by-point.

If all goes well, we can reduce a lot of existing classes to instantiations of this class with a functor, as well as more functionality easily (eg: frustum-culling, passthrough, crop-hull)

I'd have preferred to pass in the function to the applyFilter but that's not possible.

TODO:

  • Add a check for is_derived_from on the impl class
  • Move impl to namespace detail
  • Proper year on License
  • Add a check on lambda for is_invocable from the same arguments as the std::function based appraoch

@kunaltyagi kunaltyagi added changelog: new feature Meta-information for changelog generation module: filters kind: proposal Type of issue needs: feedback Specify why not closed/merged yet labels Apr 8, 2020
@kunaltyagi kunaltyagi self-assigned this Apr 8, 2020
@kunaltyagi kunaltyagi marked this pull request as ready for review April 18, 2020 22:42
@Morwenn
Copy link
Contributor

Morwenn commented Apr 23, 2020

Shouldn't is_incokable and remove_cvref_t be in type_traits.h? They are useful enough; if I was looking for them that's where I would expect to find them.

@kunaltyagi
Copy link
Member Author

PTAL

filters/include/pcl/filters/lambda_filter_indices.h Outdated Show resolved Hide resolved
filters/include/pcl/filters/lambda_filter_indices.h Outdated Show resolved Hide resolved
filters/include/pcl/filters/lambda_filter_indices.h Outdated Show resolved Hide resolved
filters/include/pcl/filters/lambda_filter_indices.h Outdated Show resolved Hide resolved
filters/include/pcl/filters/lambda_filter_indices.h Outdated Show resolved Hide resolved
filters/include/pcl/filters/lambda_filter_indices.h Outdated Show resolved Hide resolved
filters/include/pcl/filters/lambda_filter_indices.h Outdated Show resolved Hide resolved
filters/include/pcl/filters/lambda_filter_indices.h Outdated Show resolved Hide resolved
filters/include/pcl/filters/lambda_filter_indices.h Outdated Show resolved Hide resolved
filters/include/pcl/filters/lambda_filter_indices.h Outdated Show resolved Hide resolved
@kunaltyagi kunaltyagi marked this pull request as draft June 8, 2020 22:54
@kunaltyagi
Copy link
Member Author

PTAL. More tests needed though

@kunaltyagi
Copy link
Member Author

kunaltyagi commented Jun 13, 2020

What about removing the const

Kind of doesn't work. The return type of getLambda function also needs to be const in order to keep the function as const.

If we drop all the 3 consts (variable, return type, function signature), then this works.

If we force the lambda to be copyable (lambdas might be an issue here), then we can return a copy (instead of a reference) of the stored functor and still keep the method as const

@SergioRAgostinho
Copy link
Member

Kind of doesn't work. The return type of getLambda function also needs to be const in order to keep the function as const.

I suggested to not store the lambda as const, getLamda can stay and should stay as is. I'm not really getting what use case you have in mind but take my code snippet and modify it to illustrate the issue. https://godbolt.org/z/ErKbyn

@kunaltyagi
Copy link
Member Author

Pushed a diff which is giving me errors

@SergioRAgostinho
Copy link
Member

SergioRAgostinho commented Jun 14, 2020

Managed to trigger your error once I started invoking

filter.getLambda()(/*arguments*/);

If I did

auto l = filter.getLambda();
l(/*arguments*/);

No errors we're being triggered. I'm unsure why. (Double checked. It's implicitly creating a copy of the object stored inside)

Multiple edits from here down
Nevertheless, what you suggested works

    const FunctorT&
    getLambda() const noexcept
    {
      return lambda_;
    }

    /* The non-const version of the method */
    FunctorT&
    getLambda() noexcept
    {
      return lambda_;
    }

But basically it means we're allowing the contained object to be fully modified and rewritten once initialized. Is this something we want? I don't really care to be honest. The class does not rely on what object it holds. The user has full control over it.

Alternatively we don't define the non-const method and always force the user to make a copy if it wants to modify the object.

I don't mind both approaches.

@kunaltyagi kunaltyagi marked this pull request as ready for review June 14, 2020 20:57
@kunaltyagi kunaltyagi added needs: code review Specify why not closed/merged yet and removed needs: feedback Specify why not closed/merged yet labels Jun 14, 2020
test/filters/test_functor_filter.cpp Outdated Show resolved Hide resolved
test/filters/test_functor_filter.cpp Outdated Show resolved Hide resolved
test/filters/test_functor_filter.cpp Outdated Show resolved Hide resolved
test/filters/test_functor_filter.cpp Outdated Show resolved Hide resolved
test/filters/test_functor_filter.cpp Show resolved Hide resolved
test/filters/test_functor_filter.cpp Outdated Show resolved Hide resolved
@kunaltyagi kunaltyagi changed the title [Proposal] Add a generic filter to reduce boiler plate code [filters] Add a filter accepting a functor to reduce boiler plate code for simple filters Jun 18, 2020
@kunaltyagi
Copy link
Member Author

PTAL

test/filters/test_functor_filter.cpp Outdated Show resolved Hide resolved
EXPECT_EQ(filter.getRemovedIndices()->size() + out_cloud.size(), cloud->size());
}
else {
EXPECT_EQ(filter.getRemovedIndices()->size(), removed_size);
Copy link
Member

Choose a reason for hiding this comment

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

It feels like removed_size should be 0 here. If true, why not explicitly setting 0 here?

Copy link
Member Author

Choose a reason for hiding this comment

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

It should be unmodified, not equal to 0 (based on code in base classes)

test/filters/test_functor_filter.cpp Outdated Show resolved Hide resolved
@SergioRAgostinho SergioRAgostinho removed the needs: code review Specify why not closed/merged yet label Jul 1, 2020
@kunaltyagi
Copy link
Member Author

kunaltyagi commented Jul 4, 2020

Merge if no other issues (and CI is green)

EDIT:
Ignore MSVC errors. CI OOMed

@SergioRAgostinho SergioRAgostinho merged commit c262ec6 into PointCloudLibrary:master Jul 9, 2020
@kunaltyagi kunaltyagi deleted the lambda-filter branch July 9, 2020 13:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
changelog: new feature Meta-information for changelog generation kind: proposal Type of issue module: filters
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants