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

Adds a ring field to the published pointcloud #431

Closed
wants to merge 3 commits into from

Conversation

Opletts
Copy link

@Opletts Opletts commented Dec 8, 2020

Fixes #416
Each PointCloud2 message has an additional ring field.
Published points are in the format of XYZIR.


This change is Reviewable

Copy link
Contributor

@fpasch fpasch left a comment

Choose a reason for hiding this comment

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

Reviewable status: 0 of 1 files reviewed, 2 unresolved discussions (waiting on @Opletts)


carla_ros_bridge/src/carla_ros_bridge/lidar.py, line 74 at r1 (raw file):

        channels = float(self.carla_actor.attributes.get('channels'))
        lower_fov = float(self.carla_actor.attributes.get('lower_fov'))
        upper_fov = float(self.carla_actor.attributes.get('upper_fov'))

please move "static" calculations into init


carla_ros_bridge/src/carla_ros_bridge/lidar.py, line 81 at r1 (raw file):

        fov_down = lower_fov / 180.0 * numpy.pi
        fov = (abs(lower_fov) + abs(upper_fov)) / 180.0 * numpy.pi

same here

@Opletts
Copy link
Author

Opletts commented Jan 7, 2021

I've made additional changes according to #432 since it's closed now. It's a much cleaner way to implement this as you can just use get_point_count().

@Opletts Opletts requested a review from fpasch January 13, 2021 10:24
Copy link
Author

@Opletts Opletts left a comment

Choose a reason for hiding this comment

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

Reviewable status: 0 of 1 files reviewed, 2 unresolved discussions (waiting on @fpasch)


carla_ros_bridge/src/carla_ros_bridge/lidar.py, line 74 at r1 (raw file):

Previously, fpasch (fpasch) wrote…
        channels = float(self.carla_actor.attributes.get('channels'))
        lower_fov = float(self.carla_actor.attributes.get('lower_fov'))
        upper_fov = float(self.carla_actor.attributes.get('upper_fov'))

please move "static" calculations into init

Done.


carla_ros_bridge/src/carla_ros_bridge/lidar.py, line 81 at r1 (raw file):

Previously, fpasch (fpasch) wrote…
        fov_down = lower_fov / 180.0 * numpy.pi
        fov = (abs(lower_fov) + abs(upper_fov)) / 180.0 * numpy.pi

same here

Done.

Copy link
Contributor

@joel-mb joel-mb left a comment

Choose a reason for hiding this comment

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

Reviewed 1 of 1 files at r3.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @fpasch)

@joel-mb
Copy link
Contributor

joel-mb commented Mar 1, 2021

Hi! I've run a benchmark and it seems that this can be time-consuming for some configurations (when using more than one lidar). I see two options to solve this:

(1) Introduce the ring attribute directly into the raw data (this needs to be done directly in CARLA) to avoid stacking the arrays.
(2) Make this new field optional by introducing a new pseudo-attribute.

I'll start talking with the CARLA core team to see if option (1) is feasible. Also, I will start working on option (2). Either way, this will happen after the ros2 branch becomes the new master.

@Opletts
Copy link
Author

Opletts commented Mar 1, 2021

Makes sense, I was a little worried about it being time consuming but it works well enough for a single lidar so I didn't bother.

(1) Introduce the ring attribute directly into the raw data (this needs to be done directly in CARLA) to avoid stacking the arrays.

This would be ideal, right? And if they do decide to introduce the ring attribute as part of the raw data, could they also add the time attribute alongside it as mentioned in #416?

(2) Make this new field optional by introducing a new pseudo-attribute.

This is a a good enough workaround, and if you're working on it, should I close this PR and leave the issue open?

@joel-mb
Copy link
Contributor

joel-mb commented Mar 1, 2021

This would be ideal, right? And if they do decide to introduce the ring attribute as part of the raw data, could they also add the
time attribute alongside it as mentioned in #416?

yeah, this would be ideal. I will report the addition of this new field as well. I'll let you know what's the final decision.

This is a a good enough workaround, and if you're working on it, should I close this PR and leave the issue open?

yes. please close this PR. I'll update all the progress about this feature there.

thanks!

@Opletts Opletts closed this Mar 1, 2021
@cacao1987
Copy link

time attribute

Hi,Opletts,have you add time attribute to the pointcloud already @Opletts

@Robotics010
Copy link

Hi @joel-mb ! Is there any news about adding ring field to carla lidar measurement? Could you refer to the opened issue in carla repo to track its progress?

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.

Generating ring and time fields for lidars
5 participants