Numpy version of point_cloud2.read_points_list and point_cloud2.read_points

[dheera]

In the spirit of ROS2 which delivers messages with ndarray / array instead of Python lists, this avoids slow Python iteration over points. In this new implementation there are no python loops over actual PointCloud2 points, everything is vectorized.

It works by simply "typecasting" the PointCloud2 data into a struct array using numpy views and recarrays.

[dheera]

I couldn't get the DCO working. Not sure what that is. I tried to follow the directions and I get

:) git rebase HEAD~3 --signoff
error: cannot rebase: Your index contains uncommitted changes.
error: Please commit or stash them.

I tried to follow the directions in the referenced link (https://docs.github.com/en/github/collaborating-with-pull-requests/reviewing-changes-in-pull-requests/checking-out-pull-requests-locally) and I get this

:) git fetch origin pull/155/head:master
fatal: couldn't find remote ref pull/155/head

Could someone please help post the exact command in the "DCO" documentation? Thanks!

[dheera]

I tried this and somehow it did something but it punished me with a merge commit and still DCO fails ...

:) git rebase HEAD~4 --signoff
Current branch master is up to date, rebase forced.
First, rewinding head to replay your work on top of it...
Applying: numpy-ified point_cloud2.py
Applying: map all numpy.record to python lists before testing
Applying: add author line
Applying: fix style issues
(base) ~/Dropbox/code/ros/common_interfaces/sensor_msgs_py :) git push
To github.com:/dheera/common_interfaces
 ! [rejected]        master -> master (non-fast-forward)
error: failed to push some refs to '[email protected]:/dheera/common_interfaces'
hint: Updates were rejected because the tip of your current branch is behind
hint: its remote counterpart. Integrate the remote changes (e.g.
hint: 'git pull ...') before pushing again.
hint: See the 'Note about fast-forwards' in 'git push --help' for details.

[dheera]

I then went googling and found this and tried

 :) git commit --amend --signoff
[master 1a11e58] Merge branch 'master' of github.com:/dheera/common_interfaces
 Date: Sat Jul 17 23:47:08 2021 +0000

and then it punished me with 2 more merge commits. This is quite unforgiving!

[flynneva]

It looks like you've duplicated your commits and there are only 4 (maybe 3?) "real" commits for this PR.

I'd recommend resetting back to there and force push to reset your branch without the redundant commits. This should remove your extra merge commits too.

[dheera]

@flynneva Is it possible to reset back without re-creating the branch? Every "git undo" argsoup I tried online in the past created more commits instead of undoing the last commit.

[flynneva]

Yeah, three steps but you can do it. "git reset HEAD~8" to reset back 8 commits (or however many you want to go back), "git restore ." to get rid of any changes that might still be in your local repo. "git push -f" to force push your new commit history.

Obviously this should only be done in unique situations since you are essentially erasing some git history that is out in the world. In this situation though it makes sense I think.

[hidmic]

@dheera DCO still needs to be fixed up. Also, would you mind adding a test to check the record-like structure of points? E.g. grab a point, check expected attributes are there.

[hidmic]

@dheera nit^2:

Suggested change

	nan_indexes = None
	for field_name in all_field_names:
	if nan_indexes is None:
	nan_indexes = np.isnan(points[field_name])
	else:
	nan_indexes = nan_indexes | np.isnan(points[field_name])
	nan_indexes = False
	for field_name in all_field_names:
	nan_indexes |= np.isnan(points[field_name])

[hidmic]

Also, if field_names is not None, we can probably filter columns out before doing this.

[dheera]

@hidmic Good points, also I think there may be a failure case if the fields are not consecutive, let me run through that again and create a test.

[aprotyas]

About the issues with DCO, I would suggest you just squash all your commits with git rebase -i HEAD~[X], where X is the number of commits you want to squash. Just make sure you sign the squashed commit.

[dheera]

About the issues with DCO, I would suggest you just squash all your commits with git rebase -i HEAD~[X], where X is the number of commits you want to squash. Just make sure you sign the squashed commit.

@aprotyas I tried that, and it just punished me with more commits. I'll try again ...

But anyway, I use this code in my robots already, but ultimately, if the owners of this repo can make it easier to contribute I'm happy to contribute. I hate to complain but honestly stuff like this really makes it hard for people who want to contribute to actually contribute; perhaps even a Google docs form would be easier if there really is any legal issue. Github 2FA should be secure enough that signing isn't necessary. But another easy way I can do it is really to just put my code in another repo in BSD (or even public domain) and you can just cut and paste it, this is a very trivial contribution and this DCO signing stuff is 10X the amount of work as actually writing the code. It would be really helpful if we avoided asking people who already do free work to do more work.

[aprotyas]

@dheera I hear you, and I think there's merit in the point you make - especially when dealing with DCO takes more commits than the actual contribution.

I believe there's also value in having a DCO, if for nothing else but to show that a contributor is allowed to make that contribution and that the project has the right to distribute it; out of these two objectives - Github 2FA really only solves the former. Given the repo owners would like to keep aligning with this objective, I think DCO is a low barrier of entry for developers. Another alternative is the use of CLAs, which is frankly asking for even more work from contributors. I guess the "least friction" mechanism I can think of is to implicitly have contributors "Sign off" to the terms of the DCO by stating that their act of committing to the repository in itself represents a "Sign off" - something that would have to be documented explicitly in the contributing guideline (that's how GitLab FOSS does it).

Ultimately, this conversation probably does not belong in this PR... and I'd be glad to continue the discussion elsewhere. To be honest, I don't have enough context to make informed suggestions in either direction, but the ROS discourse would be a good place to talk.

---

And now, back to our regularly scheduled programming - I can volunteer to attempt to force-push and fix the DCO issues for this PR if you want @dheera?

[clalancette]

The DCO is the least problematic thing we can use. We don't have the option to use nothing.

In 99% of the cases DCO is trivial. It only is problematic when one of the commits in the middle somehow misses it. In that case, the easiest thing to do is to:

1. Rebase the PR on the latest (git rebase master).
2. Go back to the branch point and rebase (git rebase -i origin/master)
3. During the interactive rebase, squash all commits together (mark all commits as squash except for the very first one).
4. Sign the resulting commit (make sure it contains "Signed-off-by: Name "

If you are still having problems and can't get that to work, one of us can do the rebasing and squashing for you. Though we cannot sign the DCO for you, as we cannot ascertain whether you are allowed to contribute this work (only you can do that).

[flynneva]

@dheera also just an FYI - you can automatically sign commits when you create them in the future with the -s option with git.

git commit -s -m "my signed commit" will result in a signed commit - avoiding the need to re-do the signing later on.

[flynneva]

@dheera do you care if I make another PR for this change to take this off your plate?

[clalancette]

Closing in favor of #175.