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

agent_info_display plugin using ignition-gui3 #386

Merged
merged 8 commits into from
May 3, 2021

Conversation

scpeters
Copy link
Contributor

@scpeters scpeters commented Apr 20, 2021

This migrates the agent_info_display0 plugin from ignition-gui0 to use ignition-gui3. I started by copying a bit of the approach from OriginDisplay added in #375 but ran into some rendering problems. Copying the eventFilter from the Screenshot plugin and moving all ignition::rendering calls into that plugin fixed things for me. I was also able to get rid of the timerEvent by using the eventFilter.

It almost works

* The floating text moves, but the cars stop moving
* The plugin will seg-fault if you toggle visibility at
  the wrong time. There is a data race somewhere.
@scpeters scpeters force-pushed the scpeters/agent_info_display branch from b0614a3 to 39d1f34 Compare April 21, 2021 05:17
@francocipollone
Copy link
Collaborator

francocipollone commented Apr 21, 2021

There is a data race somewhere as rapidly toggling the visibility checkbox causes a seg-fault.

I wonder if this seg-fault is caused by the same at #384 (comment)

EDITED:

It is interesting because I used the same approach to toggle the mesh layers and it works properly but now when using text geometries(for the labels) I reach a seg fault as you.
I will dive into this so I'll let you know if I found something.

@scpeters
Copy link
Contributor Author

I think my problem with rendering is related to threads. Looking at the VisualizeLidar plugin in ign-gazebo, it seems that they make rendering calls from an eventFilter callback if the event is of type ignition::gui::events::Render::kType, since "This event is called in Scene3d's RenderThread, so it's safe to make rendering calls here"

https://github.com/ignitionrobotics/ign-gazebo/blob/ign-gazebo5/src/gui/plugins/visualize_lidar/VisualizeLidar.cc#L226-L227

I will try putting all my rendering calls in this type of callback

This fixes the rendering and the seg-faults by moving all
ignition::rendering API calls to the eventFilter function
during the ignition::gui::Render event.
@scpeters scpeters changed the title WIP: agent_info_display plugin using ignition-gui3 agent_info_display plugin using ignition-gui3 Apr 29, 2021
@scpeters scpeters marked this pull request as ready for review April 29, 2021 18:33
@scpeters
Copy link
Contributor Author

this is now working and ready for review

Copy link
Collaborator

@agalbachicar agalbachicar left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

PTAL

/////////////////////////////////////////////////
void AgentInfoDisplay::UpdateAgentLabel(const ignition::msgs::AgentState& _agent, const std::string& _agentName,
std::shared_ptr<AgentInfoText> _agentInfoText) {
double x = 0.0;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we use a pose to hold all these variables?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it's not so convenient to put them in a Pose3 because ignition::msgs::AgentState uses its own RPYAngles representation for orientation so there isn't an easy way to convert to ignition::math::Pose3

I did store the position and linear_velocity in Vector3 objects in 04490de since it was convenient to do so

}

std::stringstream ss;
ss << _agentName << ":\n pos:(x:" << std::setprecision(2) << x << ",y:" << y << ",z:" << z << ",yaw:" << yaw << ")"
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does ignition::math::Pose3d has an operator<< overload? If so, can we use it?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I used the << operator from the Vector3 class for position and linear_velocity. I didn't use it for the orientation because the text label only includes the yaw currently

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

in 04490de

Use an ignition::math::Vector3d for position and linear velocity
when serializing to string since there is an easy conversion
method from ignition::msgs.
agalbachicar
agalbachicar previously approved these changes Apr 30, 2021
Copy link
Collaborator

@agalbachicar agalbachicar left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

Copy link
Collaborator

@francocipollone francocipollone left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, just some nits:

@scpeters scpeters dismissed stale reviews from francocipollone and agalbachicar via e124de3 May 1, 2021 00:23
Copy link
Collaborator

@agalbachicar agalbachicar left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

Copy link
Collaborator

@francocipollone francocipollone left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

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

Successfully merging this pull request may close these issues.

3 participants