-
Notifications
You must be signed in to change notification settings - Fork 26
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
Experimental support to sensors and depth camera example #249
base: devel
Are you sure you want to change the base?
Conversation
I wanted to open an issue upstream on this but for the time being I did not had the time. However, the fundamental problems is that the https://github.com/ignitionrobotics/ign-sensors library does strongly couple two different functionalities:
For gym-ignition, we are actually interested just in A, and ideally we would like to disable B unless strictly necessary. This hard coupling is probably an heritage of the sensors codebase from Classic Gazebo, and is particularly problematic from sensors such as IMUs and Six-Axis FT sensors that in real life can reach update rates of ~KHz, and in which the non-determinism introduced by ignition-transport communication can drastically affect controller performances. |
Yes this is the situation I discovered in the sensors / rendering stack. It seems that the current logic about topics is that if you don't define a custom name, a default one is used. It seems that this behaviour is deeply rooted in the SDF specification, see
I'm not sure if this is what you meant, but the slow execution is only related to rendering-based sensors. The stack for IMUs is quite different (see |
To be honest, I don't know. There is a huge effort in reusing part of the SDF format also for Drake, and in that context I think it does not make a lot of sense to have |
I was referring to cases such as the one described in osrf/subt#307 and https://osrf-migration.github.io/ignition-gh-pages/#!/ignitionrobotics/ign-gazebo/pull-requests/498/page/1 . It does not matter how fast an operation is, unless there is an explicit synchronization under heavy operating system load you may have undesirable behaviors. |
We could give it a chance, I think we all agree that it would be really beneficial and publishing to the network should be an optional behaviour. If a change of behaviour is too much, maybe a precise string that prevents any streaming could get accepted (e.g. Even more, since the library is sold as Ignition Sensors is an open source library that provides a set of sensor and noise models accessible through a C++ interface and it would be really cool if there would be native support of getting pointers to sensors from C++ code with no transport involved also from ign-gazebo. I had to vendor the Sensors system and add a custom component to obtain a pointer to the sensor, and I suspect I should do the same for all other sensors that use their custom system.
Yep, I remember. Sorry if it was not clear, this PR already does not use transport. I didn't specify it in the first post, but the callback I mentioned are connected directly to the sensor objects, not the published topics. |
BTW, I get the following failure in CI:
I suspect we need to use xvfb or similar if we want rendering tests to succeed, similarly to what needs to be done for rendering OpenAI Gym environments in colab https://stackoverflow.com/a/52600239. Edit: fixed using https://github.com/The-Compiler/pytest-xvfb. |
Ah cool, I did not realized there is the callback mechanism. In that case (by vendoring the Sensors system) it is possible to avoid the problem. |
bde3902
to
e65d2b4
Compare
0a8c39e
to
d85529c
Compare
d85529c
to
8059726
Compare
This PR is stale for long time now, and I have no plans to start working on it soon. There's some outstanding work upstream in gazebosim/gz-sim#793 that will enable Ignition Fortress to run server and gui in a single process. This change could greatly simplify the implementation of sensors, and Fortress could be a good candidate for the introduction of sensors support. After a quick look to gazebosim/gz-sim#793, a possible alternative (and much simpler) implementation of sensors is the following:
Then, if I understood, a new event could be emitted to trigger the sensors rendering, and we could also control the rate. We should try to aim to make the sensor rendering rate independent from the simulator rate, because otherwise the simulation would be too slow (as experienced in this PR). Though, this might be a desired feature (e.g. to generate tight datasets), so I'd keep this use case as valid. If this works, we don't have to maintain a vendored Sensor system like proposed in this PR, which is a pain to maintain. In any case, I don't plan to work on this either very soon (we're still on Dome in |
This PR provides the initial support to rendering-based sensors #199. This implementation has not been really straightforward and it required quite a lot of time to understand the multithreading implementation of upstream's Sensors system and rendering stack. In particular, it does not seem to me that the simulator ensures a prompt image generation right after a single run and I had to enhance the synchronization mechanism to lock execution when needed.
In more detail, this PR:
Sensors
system.Sensors
system to be compatible with reproducible execution, i.e. being able to extract sensory data right after a simulator run.Sensors
system withWorld::enableSensors
.DepthCamera
class.Model::depthCamera
.Caveats and WIPs;
DepthCamera
class we need to know what's the rate of the server in order to prepare the images. Right now it's hardcoded. We should create a new component for the World entity with this information.std::promise
andstd::future
for locking the caller execution only if data is required.DepthCamera
class, that is returned to the user as const reference. The same buffer is moved from promise to promise as time passes. This approach is not bullet-proof and should be further tested.This PR is the first step to get a heightmap of the terrain gazebosim/gz-sim#337. It's not directly related to that, being more general to the supported sensors, but it allowed me to study the rendering stack and it was natural to put an extra effort and integrate it into gym-ignition.
cc @traversaro early feedback is welcome