Skip to content

Conversation

Fellfalla
Copy link

Before this MR, doTransform(my_poly, my_poly, transform) did not work as it does for other messages, e.g. pose.

If this is the desired behavior should be discussed.

Copy link
Contributor

@ahcorde ahcorde left a comment

Choose a reason for hiding this comment

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

Copy link
Contributor

@CursedRock17 CursedRock17 left a comment

Choose a reason for hiding this comment

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

I have left a change that should resolve the bug fix, while implementing this feature, without adding extra bloat. Branch reference for a quick test of the code that works for me.

Comment on lines +466 to +470
std::vector<geometry_msgs::msg::Point32> points_transformed(poly_in.points.size(), {});
for (size_t i=0; i < poly_in.points.size(); ++i) {
doTransform(poly_in.points[i], points_transformed[i], transform);
}
poly_out.points = points_transformed;
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
std::vector<geometry_msgs::msg::Point32> points_transformed(poly_in.points.size(), {});
for (size_t i=0; i < poly_in.points.size(); ++i) {
doTransform(poly_in.points[i], points_transformed[i], transform);
}
poly_out.points = points_transformed;
poly_out.points.resize(poly_in.points.size());
for (size_t i = 0; i < poly_in.points.size(); ++i) {
doTransform(poly_in.points[i], poly_out.points[i], transform);
}

The tf2::Polygon cannot handle the = assignment cleanly, so the corresponding test fails. But if we want, an easy fix is to just call reserve on poly_out before we run the doTransform loop, then it will ensure there's space for the referenced point to be moved into the function call. This is beneficial since we're not copying any values.

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