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

Consider promoting rs2_lcm::camera_description_t to drake lcmtypes #9801

Closed
RussTedrake opened this issue Oct 25, 2018 · 16 comments
Closed

Consider promoting rs2_lcm::camera_description_t to drake lcmtypes #9801

RussTedrake opened this issue Oct 25, 2018 · 16 comments

Comments

@RussTedrake
Copy link
Contributor

the realsense driver publishes it. if we want to consume it from drake, it probably needs to be promoted. any objections? cc @sammy-tri ? @EricCousineau-TRI ?

@sammy-tri
Copy link
Contributor

I'm fine with promoting it to RobotLocomotion/lcmtypes.

@EricCousineau-TRI
Copy link
Contributor

EricCousineau-TRI commented Oct 26, 2018

Per Slack discussion, these types do not seem to require any Drake-specific LCM types, so it'd be nice to put them a little more upstream, so I'd agree thta RobotLocomotion/lcmtypes (or even upstream) would be the best place.

If we think LCM is worth the time investment, we should try to make a better set of standard messages.
If not, we should invest more time in using ROS 1.0, ROS 2.0, or another messaging system.

@sammy-tri
Copy link
Contributor

Are there particular issues with the existing message types which we should consider correcting before moving into a common repo? I think changing out the encoding and transport layers (to say the least!) is perhaps beyond the scope of this issue unless the existing defects are particularly bad.

@jwnimmer-tri
Copy link
Collaborator

The only thing I've noted is that most of RobotLocomotion/lcmtypes use a common header_t. You should probably add the header, when moving the messages into the RLG repository?

@sammy-tri
Copy link
Contributor

I think the messages using header_t are in the minority, though it still might be nice. It would require a flag day for existing users of these messages, but that might be worth it.

@RussTedrake
Copy link
Contributor Author

I was not proposing any changes to the type, only relocating it's definition. The standard argument to strongly disprefer changing the type is that one can no-longer playback old log files using the types on master. Perhaps in this particular case, we don't have log files that we value for the camera description? But if we do, then I would recommend to move the type without adopting a new header.

@RussTedrake RussTedrake changed the title Conside promoting rs2_lcm::camera_description_t to drake or robotlocomotion lcmtypes Consider promoting rs2_lcm::camera_description_t to drake or robotlocomotion lcmtypes Nov 5, 2018
@RussTedrake
Copy link
Contributor Author

@kmuhlrad -- can you update me on what you consider to be the current status of the camera config in manip station? (do we still need a better way to configure it?)

@kmuhlrad
Copy link
Contributor

kmuhlrad commented Mar 8, 2019

Right now the cameras are still added in the C++ via hardcoded transforms. I'm still using the custom config files for Python stuff, which repeats some of that information. No cameras exist in any .sdf files (although that makes less sense for the clutter clearing demo than for the class setup). Deciding a common format for configuring the cameras would be good.

@EricCousineau-TRI
Copy link
Contributor

Are y'all using ManipulationStation at all in C++?

If not, consider just adding the cameras through Python, which is rich enough language that you don't really need a config file? (just sugar functions, like xyz_rpy_deg?)

@kmuhlrad
Copy link
Contributor

kmuhlrad commented Mar 8, 2019

All of my uses are in Python. I think the point of the config files is to make sure that values aren't being repeated in multiple places. For example, I need access to the camera pose when doing downstream point cloud processing so I can put the point cloud in world frame, which shouldn't live in ManipulationStation.

@EricCousineau-TRI
Copy link
Contributor

EricCousineau-TRI commented Mar 8, 2019

Hm... What is the point cloud processing written in? How are these things coordinated?
Can you point to the code in underactuated?

I'm pretty sure that this can be resolved with either functions or middleware comms, without slowing down app evolution by upstreaming app code to libraries prematurely.

To put on my hacker / engineering hat: I'd only be fine (and only marginally so) with config files in ManipulationStation as long as there are massive disclaimers stating they're only tailored to acute use cases within pydrake.examples and nothing else, and have zero promises of compatibility, versioning, etc.

(The config files will ultimately be very application specific to start, and as such should live near the application so they can change quickly without getting in the way.)

@kmuhlrad
Copy link
Contributor

kmuhlrad commented Mar 8, 2019

All the processing I'm doing is in Python. Here's a slightly outdated version of the system: https://github.com/RobotLocomotion/6-881-examples/blob/master/perception/point_cloud_to_pose_system.py. Now that PointClouds support RGB, the RGB images would be connected elsewhere. The entire flow is in https://github.com/RobotLocomotion/6-881-examples/blob/master/perception/run_perception_system.py#L106. The config files I'm using just have the same camera poses as the C++, but changing one does not affect the other.

This discussion seems to be related to #10127

@EricCousineau-TRI
Copy link
Contributor

EricCousineau-TRI commented Mar 9, 2019

Ah, yeah, that discussion is very relevant. I was about to re-type the same response :)

We really should start modularizing ManipulationStation s.t. it's no longer a unifying (but highly conditional / multi-modal) interface, but rather a very compact and useful integration of useful elements that people can easily copy + paste to modify to their desire.

Truly, there should be a free function in underactuated that has the 6-881-specific camera poses + object tuples, that are easy to query for constructing ManipulationStation or for doing pose detection in the world frame.

I would love to help with this, but unfortunately have other things on my plate at the moment :(

The config files I'm using just have the same camera poses as the C++, but changing one does not affect the other.

Meep! That means brittle duplication! At this point, I would suggest one of the following:

  1. In the pose detection process, instantiate a throw-away ManipulationStation, call GetStaticCameraPosesInWorld(), and use those for getting stuff in the world frame. (My recommendation for now! unless it's super slow or noisy)
  2. Have whatever binary creates its ManipulationStation write to a temporary configuration file that is specific to underactuated. (blech on this one, but mayhaps better for the short term)
  3. Do middleware things to communicate camera extrinsics.
  4. Do the free function approach, and decompose the ManipulationStation interface into its most modular pieces - relevant to my peanut gallery badgering in Manipulation station (cupboard, iiwas, wsg, cameras, and objects) should all specified via yaml #10022 and Manipulation station needs refactoring to be able to swap grippers readily... #10851 (Yay! But a lot of work at the moment...)

@sammy-tri sammy-tri self-assigned this Mar 22, 2019
@RussTedrake RussTedrake added type: MIT manipulation Related to http://manipulation.mit.edu and removed type: MIT underactuated Related to http://underactuated.mit.edu labels Jan 26, 2020
@jwnimmer-tri
Copy link
Collaborator

Consider promoting rs2_lcm::camera_description_t ....

It took me a while to figure out what this was referring to. I think the referent is https://github.com/RobotLocomotion/realsense2-lcm-driver/blob/master/lcmtypes/camera_description_t.lcm.

I'm not sure why we need to copy the file in order to refer to it from Drake? We could add it as an external.

@jwnimmer-tri
Copy link
Collaborator

For the record... use of RobotLocotiomotion/lcmtypes by Drake is deprecated now. We should not add new items to it.

@jwnimmer-tri jwnimmer-tri changed the title Consider promoting rs2_lcm::camera_description_t to drake or robotlocomotion lcmtypes Consider promoting rs2_lcm::camera_description_t to drake lcmtypes Aug 10, 2021
@RussTedrake
Copy link
Contributor Author

I haven't needed this recently and don't currently intend to fix it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

5 participants