Skip to content

Lyrical compatibility #163

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.

Sign up for GitHub

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

Open
mxgrey wants to merge 2 commits into
base: main
Choose a base branch
from
Open

Lyrical compatibility #163

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
215737a
Remove line that was causing UB segfault in gz_sim
mxgrey May 20, 2026
d2860aa
Return to removing AxisAlignedBox component
mxgrey May 20, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
14 changes: 13 additions & 1 deletion rmf_building_sim_gz_plugins/src/lift.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -160,6 +160,7 @@ class LiftPlugin

void read_aabbs(EntityComponentManager& ecm)
{
std::vector<Entity> remove_from;
ecm.Each<components::AxisAlignedBox,
components::Pose>([&](const Entity& entity,
const components::AxisAlignedBox* aabb,
Expand All @@ -174,10 +175,21 @@ class LiftPlugin
// TODO(luca) this could be a component instead of a hash map
_initial_aabbs[entity] = aabb->Data();
_initial_poses[entity] = pose->Data();
enableComponent<components::AxisAlignedBox>(ecm, entity, false);
Copy link
Copy Markdown
Member

@luca-della-vedova luca-della-vedova May 20, 2026

Choose a reason for hiding this comment

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

This was added because of the comment above (RTF dropping as soon as lift starts moving).
Does this still happen? If it does we should still remove the components but not inside the Each call (i.e. we collect them into a vector then iterate and remove outside the loop).
If it doesn't happen anymore we should just remove the optimization comment above since it's not relevant anymore

Copy link
Copy Markdown
Collaborator Author

@mxgrey mxgrey May 20, 2026

Choose a reason for hiding this comment

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

Thanks for calling this out, I wasn't aware of the reason the component was being removed.

I haven't looked at whether there's a frame drop, but if it's a possible concern then I don't mind collecting the entities and removing the component after the Each.

Copy link
Copy Markdown
Collaborator Author

@mxgrey mxgrey May 20, 2026

Choose a reason for hiding this comment

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

Restored: d2860aa

remove_from.push_back(entity);
}
return true;
});

for (const auto entity : remove_from)
{
// We remove this component from all the entities because we've observed
// a significant drop in RTF while lifts are moving if they contain this
// component.
//
// TODO(mxgrey): Identify why the RTF would drop, and see if there's a way
// to avoid removing this component.
enableComponent<components::AxisAlignedBox>(ecm, entity, false);
}
}

std::vector<std::string> get_available_floors(const LiftData& lift) const
Expand Down
Loading