-
Notifications
You must be signed in to change notification settings - Fork 270
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
Run server and client in the same process #793
base: main
Are you sure you want to change the base?
Conversation
I made a comment in the related issue #556 (comment) I want to make it work then I will clean up te code. |
Signed-off-by: Louise Poubel <[email protected]>
Signed-off-by: Louise Poubel <[email protected]>
Signed-off-by: ahcorde <[email protected]>
Signed-off-by: ahcorde <[email protected]>
476a11a
to
ae902c3
Compare
I updated the base PR to match with the recent refactoring in the scene3D. Current status
to reproduce: /home/ahcorde/ignition_fortress_leak/install/bin/ign gazebo --gui-config src/ign-gazebo/src/gui/gui.config camera_sensor.sdf -v 4 --same_process --render-engine ogre --render-engine-gui ogre --render-engine-server ogre If you want to try Ogre two, you should modify the gui.config file and replace /home/ahcorde/ignition_fortress_leak/install/bin/ign gazebo --gui-config src/ign-gazebo/src/gui/gui.config camera_sensor.sdf -v 4 --same_process --render-engine ogre2 --render-engine-gui ogre2 --render-engine-server ogre2 Limitationseverything should run with ogre or ogre2 but we cannot mix both renders when gui and server are running in the same process. |
Signed-off-by: ahcorde <[email protected]>
Signed-off-by: ahcorde <[email protected]>
Signed-off-by: ahcorde <[email protected]>
Signed-off-by: ahcorde <[email protected]>
Signed-off-by: ahcorde <[email protected]>
Signed-off-by: ahcorde <[email protected]>
Decisions that I have madeThere are two differences main way to launch ign-gazebo: with sensors and without sensors. This have some implications. When we are runninig with sensorsThere are two RenderUtils available: One in the sensor system plugin and another in the GUI. This is the easiest case because there are two system plugins that remove entities: Sensors and Physics which runs at the same speed. But the Scene 3D is running just at 30Hz. This may cause some issues in the Entity tree plugin. Some of the EachRemoved cycles are lost. I have created some events to be able to remove/add entities in the Entity tree plugin, this event are emitted in the renderutils class.
The GzSceneManager should not update renderutil because the sensor thread will do it. When running without sensorsIn this case the physics system plugin will be the only one with access to the removeEach calls. In this case this plugin will send an event called UpdateGUIECM to avoid missing any removals. There is another event called EnableSensors this event is used
RenderOn of the limitiation that we have, it is that we can only update the render scene in the same thread, otherwise we will get a crash. When running in the same process there are two Renderutils trying to update the scene3d in different threads, it's important to load in the first time the Scene3D to set the option Notes
|
Recommended branch of ign-gui https://github.com/ignitionrobotics/ign-gui/pull/new/ahcorde/6/new_scene |
With the latest changes, the events for keeping GUI plugins in sync with server side seems to be working from testing. I also want to brainstorm ideas on how we could reuse more of the current ECM infrastructure. I think the problem all came down to the GuiRunner running the gui plugins at 30 Hz in a separate thread. What do you think of the following proposal:
The idea above is so that the gui plugins can keep using the |
Signed-off-by: ahcorde <[email protected]>
if (this->entityItems.find(_entity) != this->entityItems.end()) | ||
{ | ||
return; | ||
} |
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.
Broke this off into #974
Signed-off-by: Louise Poubel <[email protected]>
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 took a look at the sensors - 3D scene synchronization, and it concerns me that we're instantiating 2 RenderUtil
classes that compete to create and update entities. I'm also wondering what impact the GUI running the render loop will have on the sensors' update rate. See more detailed comments below.
@@ -316,6 +331,45 @@ void SensorsPrivate::Run() | |||
this->renderThread = std::thread(&SensorsPrivate::RenderThread, this); | |||
} | |||
|
|||
////////////////////////////////////////////////// | |||
void SensorsPrivate::Render() |
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.
Is this function ever called?
return rendering::VisualPtr(); | ||
auto vis = this->dataPtr->scene->VisualByName(name); | ||
this->dataPtr->visuals[_id] = vis; | ||
return vis; |
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.
Are these changes here because Sensors
and Scene3D
are trying to create the same entity? I think we should solve the underlying issue instead of protecting against it.
Right now I see lots of error messages:
$ ign gazebo sensors_demo.sdf --same-process
[GUI] [Err] [SceneManager.cc:187] Visual: [ground_plane] already exists
[GUI] [Err] [SceneManager.cc:187] Visual: [box] already exists
[GUI] [Err] [SceneManager.cc:187] Visual: [cameras_alone] already exists
[GUI] [Err] [SceneManager.cc:187] Visual: [camera_with_lidar] already exists
[GUI] [Err] [SceneManager.cc:187] Visual: [rgbd_camera] already exists
[GUI] [Err] [SceneManager.cc:187] Visual: [thermal_camera] already exists
[GUI] [Err] [SceneManager.cc:187] Visual: [Construction Cone] already exists
[GUI] [Err] [SceneManager.cc:1054] Visual: [sun::sunVisual] already exists
[GUI] [Err] [SceneManager.cc:267] Entity with Id: [36] already exists in the scene
[GUI] [Err] [SceneManager.cc:1098] Light with Id: [38] already exists in the scene
[GUI] [Err] [SceneManager.cc:1054] Visual: [sun::sunVisual] already exists
We need to have a single authority on who's managing the lifecycle of entities in the scene when we run sensors and GUI in the same process. Otherwise, they're both trying to do duplicate work, which may eventually cause undefined behaviour.
My suggestion is that during sameProcess
, we should have a single RenderUtil
and SceneManager
instantiated, instead of 2 trying to update the same scene.
if (!this->sameProcess) | ||
{ | ||
IGN_PROFILE("Update"); | ||
this->renderUtil.Update(); |
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.
Can you comment on the reasoning for having the GUI drive the render loop? Sensors are pickier about update rates, so I think they should have control.
Didn't make it into |
Signed-off-by: ahcorde [email protected]
🎉 New feature
Summary
Related to this issue #556
Test it
ign gazebo -r -v 4 --same-process shapes.sdf ign gazebo -r -v 4 --same-process camera_sensor.sdf # -> crash
Checklist
codecheck
passed (See contributing)Note to maintainers: Remember to use Squash-Merge