Skip to content

Executor integration with filters #3

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

Open
wants to merge 15 commits into
base: executors
Choose a base branch
from

Conversation

shrijitsingh99
Copy link
Owner

@shrijitsingh99 shrijitsingh99 commented Jul 5, 2020

This will serve as a base PR for all executor integration related aspects.

TODO:

Update: This will be the first PR to be merged into PCL, after this all the remaining PR's are to be merged.

@shrijitsingh99 shrijitsingh99 force-pushed the executor-integration branch from 6148799 to 5252e56 Compare July 5, 2020 23:42
@shrijitsingh99 shrijitsingh99 changed the base branch from master to latest August 7, 2020 10:39
@kunaltyagi
Copy link
Collaborator

Anything to review here or has the code arrived here after review?

@SergioRAgostinho
Copy link
Collaborator

It's 1000 loc 😱

@SergioRAgostinho
Copy link
Collaborator

Feedback from quickly skimming at the code. Move all those C++17 traits you added to the common module. I'm sure they'll be useful in other locations of the library.

@shrijitsingh99
Copy link
Owner Author

Uh oh! Sorry didn't realize this became a huge PR. Will split this into 2 PRs. Most of the LOC are directly from the files from the executor repo.

@shrijitsingh99 shrijitsingh99 changed the base branch from latest to executors August 7, 2020 11:53
@shrijitsingh99 shrijitsingh99 changed the title Executor Integration Executor integration with filters Aug 7, 2020
@kunaltyagi kunaltyagi self-requested a review August 7, 2020 23:56
@kunaltyagi
Copy link
Collaborator

Sorry, but could you split this in 1 of 2 ways:

  • 2 commits: 1 for the changes to Filter and FilterIndices and the other for everything else
  • 2 PRs: same split as in prev point

@shrijitsingh99
Copy link
Owner Author

shrijitsingh99 commented Aug 11, 2020

2 commits: 1 for the changes to Filter and FilterIndices and the other for everything else

Done

Move all those C++17 traits you added to the common module

Will do that once best_fit is ready, during the final porting

Copy link
Collaborator

@kunaltyagi kunaltyagi left a comment

Choose a reason for hiding this comment

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

2 things for commit 1:

  • Filter calls without executor should route to filter with best-fit executor: Please note as TODO somewhere
  • PCLPointCLoud2 is not supported yet: either an oversight or needs to be documented well

Copy link
Collaborator

@kunaltyagi kunaltyagi left a comment

Choose a reason for hiding this comment

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

LGTM (apart from comments)

Please allow for a migration path too. We might need to have a discussion on this but for starters, I see 2 ways:

  • Copy + rename old code
  • Have 3 classes:
    1. EaseOfImpl: common code, hidden in detail namespace
    2. Legacy: no CRTP, new name, derived from EaseOfImpl
    3. New hierarchy: old name, derived from EaseOfImpl

Eg:

FilterImpl {};

// maybe we wouldn't mark this as deprecated since this might be a very simple class
// Plus it also prevents us from using -Werror on the CI
FilterLegacy: FilterImpl{};

template <class Derived>
Filter: FilterImpl{
  void new_function() {
    static_cast<Derived*>(this)->virtual_function();
  }
};

template <class BaseClass>
ABCFilterImpl: BaseClass {};

// mark as deprecated
ABCFilterLegacy: ABCFilter<FilterLegacy>{
  // code specific to making legacy work
};

ABCFilter: ABCFilter<Filter>{};

@kunaltyagi
Copy link
Collaborator

No migration path needed. LGTM except:

  • no code in executor-less call to filter
  • warning
  • better documentation (1 line brief doesn't count)

@shrijitsingh99
Copy link
Owner Author

Any more changes needed or we are good to go?

@shrijitsingh99 shrijitsingh99 force-pushed the executors branch 2 times, most recently from 22236ea to c28c5f0 Compare August 23, 2020 23:58
@shrijitsingh99 shrijitsingh99 force-pushed the executor-integration branch 2 times, most recently from 5c90541 to 7030725 Compare August 27, 2020 11:36
@shrijitsingh99 shrijitsingh99 force-pushed the executors branch 2 times, most recently from c5d6bf1 to 8bd1938 Compare August 28, 2020 14:24
@shrijitsingh99 shrijitsingh99 force-pushed the executor-integration branch 3 times, most recently from a44d017 to 67a64f7 Compare August 29, 2020 23:25
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.

3 participants