At the time of this review the package was named laser_scan
API review
Proposer: Tully Foote
Present at review:
- List reviewers
Question / concerns / comments
- Location and name suggestions??
- kwc: naming inconsistency: package is 'laser_scan_utils' but namespace is 'laser_scan'. That implies that the package name should be 'laser_scan'
- stu: swap 'angle_max' and 'angle_min' in getUnitVectors
jml: How does this interact with "TransformListener::transformLaserScanToPointCloud," since this provides similar (but inferior) functionality?
vijay: A LaserMedianFilter is defined, but I imagine there could be many more types of laser filters (eg. intensity filter, between-ranges filter)
It might be useful to have an abstract LaserFilter type that any laser filter would inherit off of.
- eric: in Median filter, num_ranges and averaging_length are not clearly defined. At least one likely needs re-naming
eric: in Projector, what is the need for preservative parameter, when the range also defaults to -1?
- brian: Do we intend to keep using NEWMAT data types? Should we instead use ublas or something similar?
Meeting notes
Interface with libtf?
Proposal:
- Move libtf laser projection into here
- Make selection of quality vs. speed selectable
- Pull laser_scan message into this package (Decided against)
Message dependencies:
- Don't want to add deep dependency tree in order to get laser_scan message
- Put laser_scan in robot_msg, its own package, or just depend for messages
Remove preservative
Newmat
- Can keep using
- Don't return Newmat externally
Conclusion
Package status: API partially reviewed. Second review needed.
Rename to laser_scan
Move laser_scan into robot_msg
Move tf laser_scan transform code into laser_scan
Remove num_ranges, and just allocate when scan received (num_ranges may change)
Hide getUnitVectors
Propose and review filter base-class and extensibility
Review TF integration API