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

Bugfix: Using Time(double) to take double point timestamp #24

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

ameysutavani
Copy link

The **iter_timestamp is in seconds (of type double) but ros::Time::fromNSec(uint64_t) expects nanoseconds (of type uint64_t) which caused segfault in analyzePoints. Bug fixed by using ros::Time(double) to get point_time.

@ameysutavani
Copy link
Author

@Earthwings Thanks for this awesome tool. Issuing this PR to fix a small bug I noticed during my testing.

@Earthwings
Copy link
Owner

The **iter_timestamp is in seconds (of type double) but ros::Time::fromNSec(uint64_t) expects nanoseconds (of type uint64_t) which caused segfault in analyzePoints. Bug fixed by using ros::Time(double) to get point_time.

Thanks for looking into this and providing a patch!

I think there are different ways of specifying point timestamps in use. The pointcloud you're working with uses offsets in seconds relative to the cloud's header stamp, right? In that case negative values are possible, which would indeed crash.

To get that case correct, I'd expect the code to construct the point time as a combination of the offset value and the cloud stamp. Your patch seems to lack the latter part yet, doesn't it?

Finally I think both variants should be supported: For small time values assume relative time to the cloud header stamp, otherwise assume absolute time.

@ameysutavani
Copy link
Author

Hi @Earthwings . The point cloud that I am working with has point times NOT relative to the header stamp. Each point records time from the same global clock as the header.

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