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 support for inplace filters. #38

Open
wants to merge 3 commits into
base: lunar-devel
Choose a base branch
from
Open

Conversation

peci1
Copy link
Contributor

@peci1 peci1 commented Jun 25, 2020

Solves #27.

I'll add tests later, but comments on the design are already welcome.

This changes both API and ABI of the defined filters (median, mean, increment, transfer_function), but as they are libraries nobody should link to, I think it could be doable. If this kind of breaks is not acceptable, there is a workaround solution - creating new filters (but I don't like this solution as it'd create another set of filters which differ only in the "flag" that they are inplace).

I also updated the filter_chain update functions so that they automatically detect inplace functions and call their inplace update methods whenever possible.

@peci1
Copy link
Contributor Author

peci1 commented Jun 25, 2020

As for linking to the concrete filter plugins, I'm not sure if class_loader_hide_library_symbols is not needed in CMake. It is used e.g. in pcl_ros, and there's a half-answered question about its necessity for dynamically loaded plugins: ros-perception/perception_pcl#9 . If this CMake macro were used, then no changes to either API or ABI of the concrete plugins could break any downstream code.

@peci1
Copy link
Contributor Author

peci1 commented Jun 25, 2020

There's another design option - making InplaceFilterBase and InplaceMultiChannelFilterBase classes just mixins without any base class. That would allow "tagging" inplace even downstream classes for which it's not that easy to change the base class.

@tfoote
Copy link
Member

tfoote commented Sep 23, 2020

Thanks for contributing this. As it's an API changing update. Although it likely won't effect people we'll want to target this for a future distribution to be sure not to disrupt existing users and stay stable. So we'll want to retarget this to the rolling distribution.

@peci1
Copy link
Contributor Author

peci1 commented Sep 23, 2020

Is there a new ROS 1 distro coming?

@peci1
Copy link
Contributor Author

peci1 commented Jan 7, 2021

Friendly ping. If you're concerned about breaking the ABI/API, I can just rename the existing filters and create their inplace duplicates. Not really good from the code point of view, but it would do the job.

@tfoote
Copy link
Member

tfoote commented Feb 2, 2021

There's not a new rosdistro coming in ROS 1. To land in noetic the creating new parallel classes would be a way to support a backport.

There is in ROS 2 which is being developed off the ros2 branch. If you'd like to target this for future versions that could be useful.

@peci1
Copy link
Contributor Author

peci1 commented Jun 30, 2021

Okay, I've made the PR ABI compatible. The filters are now duplicated, but fortunately code can be shared.

I also added a non-virtual update(T&) function to FilterBase that dispatches based on RTTI to the correct override. It is a convenience function. It doesn't need to be there if it's not desired.

Once the approach is approved, I'll finish documentation and tests.

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.

2 participants