-
Notifications
You must be signed in to change notification settings - Fork 277
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
Delay and time jump in object position update when using the transform tool #1358
Comments
I have tested various commits/releases of I went ahead and built |
After some more debugging, I believe this issue was introduced in #851. The serialization/deserialization for models is done via converting a SDF element to/from a string, which seems to cause a bottleneck in the GUI <-> server communication. If don't serialize diff --git a/include/ignition/gazebo/components/Model.hh b/include/ignition/gazebo/components/Model.hh
index 5ba0420d..6085e9a4 100644
--- a/include/ignition/gazebo/components/Model.hh
+++ b/include/ignition/gazebo/components/Model.hh
@@ -87,9 +87,7 @@ namespace components
IGN_GAZEBO_REGISTER_COMPONENT("ign_gazebo_components.Model", Model)
/// \brief A component that holds the model's SDF DOM
- using ModelSdf = Component<sdf::Model,
- class ModelTag,
- serializers::SdfModelSerializer>;
+ using ModelSdf = Component<sdf::Model, class ModelTag>;
IGN_GAZEBO_REGISTER_COMPONENT("ign_gazebo_components.ModelSdf", ModelSdf)
}
} Here's what I now see with the patch above applied to the If I don't apply the patch above, I still see the delay/jumpy behavior when testing @azeey and @ahcorde, would you mind testing the patch I gave above to see if it resolves the issue for you? Obviously, this doesn't solve the real issue of how we're handling |
I tested the above patch and the lag is gone. |
Great, thanks for verifying! We'll need to figure out a better way to serialize/deserialize |
yeah serialization performance issue came up when we were doing the performance improvements work. I think @mjcarroll has some ideas on how to improve serialization. |
I can also confirm the patch fixes the issue for me. Great job investigating @adlarkin! I'm not sure though if the problem is the effectiveness of the serialization or the fact that |
This patch also fixes the issue for me without making any changes to the diff --git a/src/systems/scene_broadcaster/SceneBroadcaster.cc b/src/systems/scene_broadcaster/SceneBroadcaster.cc
index 800d338c3..a53719075 100644
--- a/src/systems/scene_broadcaster/SceneBroadcaster.cc
+++ b/src/systems/scene_broadcaster/SceneBroadcaster.cc
@@ -314,8 +314,8 @@ void SceneBroadcaster::PostUpdate(const UpdateInfo &_info,
// we can skip the ECM serialization
bool jumpBackInTime = _info.dt < std::chrono::steady_clock::duration::zero();
bool changeEvent = _manager.HasEntitiesMarkedForRemoval() ||
- _manager.HasNewEntities() || _manager.HasOneTimeComponentChanges() ||
- jumpBackInTime || _manager.HasRemovedComponents();
+ _manager.HasNewEntities() ||
+ jumpBackInTime;
auto now = std::chrono::system_clock::now();
bool itsPubTime = (now - this->dataPtr->lastStatePubTime >
this->dataPtr->statePublishPeriod[_info.paused]); |
This works for me but I am receiving an error message now as below; [Err] [EntityComponentManager.cc:1010] Failed to create component of type [0] for entity [11092]. Type has not been properly registered. |
Here's another possible solution, it seemed to fix the delay, but it may have other unintended consequences: diff --git a/src/systems/scene_broadcaster/SceneBroadcaster.cc b/src/systems/scene_broadcaster/SceneBroadcaster.cc
index 800d338c..e1faa9ab 100644
--- a/src/systems/scene_broadcaster/SceneBroadcaster.cc
+++ b/src/systems/scene_broadcaster/SceneBroadcaster.cc
@@ -330,10 +330,14 @@ void SceneBroadcaster::PostUpdate(const UpdateInfo &_info,
set(this->dataPtr->stepMsg.mutable_stats(), _info);
// Publish full state if there are change events
- if (changeEvent || this->dataPtr->stateServiceRequest)
+ if (this->dataPtr->stateServiceRequest)
{
_manager.State(*this->dataPtr->stepMsg.mutable_state(), {}, {}, true);
}
+ else if (changeEvent)
+ {
+ _manager.ChangedState(*this->dataPtr->stepMsg.mutable_state());
+ }
// Otherwise publish just periodic change components when running
else if (!_info.paused)
{ |
I spent some time looking into taking this approach, and it looks like this fix should work - I opened a PR with this change and added test coverage if anyone would like to take a look and test it out: #1392 |
Environment
source: ign-gazebo6 (91a79da) (and binary 3.12.0 for comparison)
Description
citadel.mp4
fortress.mp4
Interestingly, the behavior is different when I try it on ign-gazebo 6.5.0 binaries. The small delay is still there, but it doesn't seem to jump in time.
fortress_binaries.mp4
The text was updated successfully, but these errors were encountered: