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

Define PointIndices based on the global Indices type alias #3822

Merged
merged 5 commits into from
Mar 30, 2020

Conversation

kunaltyagi
Copy link
Member

This PR changes nothing on the surface but it enables someone to locally change the PCL_INDEX_SIGNED to false and find the places where Indices typedef isn't being used (There should be lots of such instances, but this can only find those that are "tested").

Rest can be done via grep

Verified

This commit was signed with the committer’s verified signature.
pentschev Peter Andreas Entschev
@kunaltyagi kunaltyagi added module: common changelog: fix Meta-information for changelog generation labels Mar 28, 2020
@kunaltyagi kunaltyagi added the needs: code review Specify why not closed/merged yet label Mar 28, 2020

Verified

This commit was signed with the committer’s verified signature.
pentschev Peter Andreas Entschev

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature.
@taketwo
Copy link
Member

taketwo commented Mar 29, 2020

PointIndices is a ROS heritage, I'd rather ditch it. I don't see any benefit in wrapping the vector of indices into a custom type. Inside PCL there is mixed usage of PointIndices and Indices, I propose to slowly switch to use the latter everywhere.


namespace pcl
{
struct PointIndices
{
using Indices = std::vector<index_t>;
Copy link
Member

Choose a reason for hiding this comment

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

I'm wondering if semantically it makes more sense to call this simply Type. The enclosing type is already named *Indices.

@SergioRAgostinho
Copy link
Member

PointIndices is a ROS heritage, I'd rather ditch it. I don't see any benefit in wrapping the vector of indices into a custom type. Inside PCL there is mixed usage of PointIndices and Indices, I propose to slowly switch to use the latter everywhere.

Just saw this now. 👍

@kunaltyagi
Copy link
Member Author

I'd rather ditch it

1.11 target?

@taketwo
Copy link
Member

taketwo commented Mar 29, 2020

Before we can proceed with deprecation, we'll need to update all interfaces that use PointIndices to have a Indices twin. I don't have an overview of how much work it is; maybe a lot.

common/include/pcl/pcl_base.h Outdated Show resolved Hide resolved
@kunaltyagi kunaltyagi changed the title Depend on a type alias in PointIndices for the global Indices type alias PointIndices depends on the global Indices type alias Mar 29, 2020
@kunaltyagi kunaltyagi changed the title PointIndices depends on the global Indices type alias Define PointIndices based on the global Indices type alias Mar 29, 2020
@SergioRAgostinho SergioRAgostinho merged commit 232c46b into PointCloudLibrary:master Mar 30, 2020
@SergioRAgostinho SergioRAgostinho removed the needs: code review Specify why not closed/merged yet label Mar 30, 2020
@kunaltyagi kunaltyagi deleted the index_1 branch March 30, 2020 11:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
changelog: fix Meta-information for changelog generation module: common
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants