-
Notifications
You must be signed in to change notification settings - Fork 413
Port TFClient impls to TypeScript #1009
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
Conversation
c27a53c to
8d1e1b8
Compare
| transThres?: number; | ||
| rate?: number; | ||
| updateDelay?: number; | ||
| topicTimeout?: number; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
preceded this PR, but topicTimeout should really be serviceTimeout ... however, not worth breaking anything for this i guess.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Preceded this PR by many years I think!!
| @@ -0,0 +1,22 @@ | |||
| export namespace geometry_msgs { | |||
| export interface TransformStamped { | |||
| // TODO: some sort of unholy union type of ros1 header and ros2 header??? | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ros 1 is deprecated, so I'd just not bother and point people to use older versions of this library if they need ros 1
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agreed in theory, but at that point why not rip out the entire ROS 1 implementation of TFClient?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
and actionlib..
| x: number; | ||
| y: number; | ||
| z: number; | ||
| w: number; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
minor point, but in the ROS 2 message definition, the w field actually defaults to 1.0, whereas all the other scalar numeric fields default to 0.0. Is that something we can/should do here too?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
interfaces can't be instantiated, so you can't set defaults
| rate: number; | ||
| } | ||
|
|
||
| export type TFSubscriptionResult = Record<never, never>; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
what does it mean that this one is type vs the others being interface?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
type lets me use type expressions, like the assignment I'm doing here. interface only allows describing an object type as a block of fields in curly braces
8d1e1b8 to
8ff4176
Compare
8ff4176 to
22e6300
Compare
sea-bass
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think all my initial comments are eh, let's do it
Lots of hand-written message types here.. need to figure out a way to automate
ros-typescript-generatorusing the Docker container I built for the integration tests