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

3D fuse models #354

Open
wants to merge 123 commits into
base: rolling
Choose a base branch
from
Open

Conversation

giafranchini
Copy link

We implemented the following sensor models:

  • odometry_3d
  • imu_3d

And the following motion models:

  • omnidirectional_3d
  • omnidirectional_3d_ignition

With the associated sensor processing, constraints and tests

andreaostuni and others added 30 commits July 21, 2023 10:48
Co-authored-by: Patrick Roncagliolo <[email protected]>
Co-authored-by: Patrick Roncagliolo <[email protected]>
Copy link

@henrygerardmoore henrygerardmoore left a comment

Choose a reason for hiding this comment

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

Found a couple leftovers in omnidirectional_3d.hpp.
Thanks so much for making this PR! I hope soon to contribute a basic tutorial using these in a similar vein to the existing tutorials to help people get up and running with the 3D models, and noticed these while I was working on that.

fuse_models/include/fuse_models/omnidirectional_3d.hpp Outdated Show resolved Hide resolved
fuse_models/include/fuse_models/omnidirectional_3d.hpp Outdated Show resolved Hide resolved
fuse_models/include/fuse_models/omnidirectional_3d.hpp Outdated Show resolved Hide resolved
fuse_models/include/fuse_models/omnidirectional_3d.hpp Outdated Show resolved Hide resolved
@giafranchini
Copy link
Author

Found a couple leftovers in omnidirectional_3d.hpp. Thanks so much for making this PR! I hope soon to contribute a basic tutorial using these in a similar vein to the existing tutorials to help people get up and running with the 3D models, and noticed these while I was working on that.

Yes, I left these behind during some copy-paste. Thank you for the findings and for the help with the tutorials!!

Copy link

@henrygerardmoore henrygerardmoore left a comment

Choose a reason for hiding this comment

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

Found one other thing while working on the 3D tutorial.

fuse_models/include/fuse_models/common/sensor_proc.hpp Outdated Show resolved Hide resolved
Copy link

@henrygerardmoore henrygerardmoore left a comment

Choose a reason for hiding this comment

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

I think I found an error (found because Eigen will complain when you hit this condition).

fuse_core/include/fuse_core/util.hpp Outdated Show resolved Hide resolved
Copy link
Contributor

@DavidLocus DavidLocus 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 now been through all of the code in this PR. There's a couple of comments from Stephen that I think are still outstanding, but from what I see, structurally and mathematically, this all looks good.

fuse_optimizers/src/fixed_lag_smoother.cpp Outdated Show resolved Hide resolved
svwilliams and others added 4 commits October 18, 2024 12:25
…robotics#366)

* Support gcc12 and ceres 2.1.0
* Add support for the Manifold class when using Ceres Solver version 2.1.0 and above
* General clean up for Ceres 2.2.0 support
* Updated serialization support to be backwards compatible with previously serialized files
* Formatting changes required for ROS 2 Rolling / Ubuntu Noble
Co-authored-by: Henry Moore <[email protected]>
Comment on lines +441 to +464
state_history_.emplace(beginning_stamp, std::move(state1));
state_history_.emplace(ending_stamp, std::move(state2));

// Scale process noise covariance pose by the norm of the current state twist
auto process_noise_covariance = process_noise_covariance_;
if (scale_process_noise_) {
common::scaleProcessNoiseCovariance(
process_noise_covariance, state1.vel_linear, state1.vel_angular,
velocity_linear_norm_min_, velocity_angular_norm_min_);
}

// Validate
process_noise_covariance *= dt;

if (!disable_checks_) {
try {
validateMotionModel(state1, state2, process_noise_covariance);
} catch (const std::runtime_error & ex) {
RCLCPP_ERROR_STREAM_THROTTLE(
logger_, *clock_, 10.0 * 1000,
"Invalid '" << name_ << "' motion model: " << ex.what());
return;
}
}
Copy link

@henrygerardmoore henrygerardmoore Oct 29, 2024

Choose a reason for hiding this comment

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

Suggested change
state_history_.emplace(beginning_stamp, std::move(state1));
state_history_.emplace(ending_stamp, std::move(state2));
// Scale process noise covariance pose by the norm of the current state twist
auto process_noise_covariance = process_noise_covariance_;
if (scale_process_noise_) {
common::scaleProcessNoiseCovariance(
process_noise_covariance, state1.vel_linear, state1.vel_angular,
velocity_linear_norm_min_, velocity_angular_norm_min_);
}
// Validate
process_noise_covariance *= dt;
if (!disable_checks_) {
try {
validateMotionModel(state1, state2, process_noise_covariance);
} catch (const std::runtime_error & ex) {
RCLCPP_ERROR_STREAM_THROTTLE(
logger_, *clock_, 10.0 * 1000,
"Invalid '" << name_ << "' motion model: " << ex.what());
return;
}
}
// Scale process noise covariance pose by the norm of the current state twist
auto process_noise_covariance = process_noise_covariance_;
if (scale_process_noise_) {
common::scaleProcessNoiseCovariance(
process_noise_covariance, state1.vel_linear, state1.vel_angular,
velocity_linear_norm_min_, velocity_angular_norm_min_);
}
// Validate
process_noise_covariance *= dt;
if (!disable_checks_) {
try {
validateMotionModel(state1, state2, process_noise_covariance);
} catch (const std::runtime_error & ex) {
RCLCPP_ERROR_STREAM_THROTTLE(
logger_, *clock_, 10.0 * 1000,
"Invalid '" << name_ << "' motion model: " << ex.what());
return;
}
}
state_history_.emplace(beginning_stamp, std::move(state1));
state_history_.emplace(ending_stamp, std::move(state2));

I believe state1 and state2 are being used after move here. Does switching the order still work? Seems like it should since none of the switched stuff after relies on state_history_ as far as I can tell.

Copy link
Author

Choose a reason for hiding this comment

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

Yes, it's true that in this case state1 and state2 are being used after move. I basically copied the implementation of generateMotionModel of the Unicycle2D class, where it's done in the same way.
Anyway, after some testing I found that the two object member values are not modified after a call to std::move(), I believe that this is since state1 and state2 are of StateHistoryElement struct type that:

  • does not implement a custom move constructor
  • has member types that recall to primitive types such as float, double (I'm not entirely sure about UUIDs which are boost::uuids::uuid but they behave in the same way), which remain unchanged after move

For example, I tried adding another std::string member to StateHistoryElement struct, assign a value to it, and I checked that after move it is left in valid and uninitialized state (an empty string in this case).

About your suggested changes, the only thing that I am not sure of is that in this case, if validateMotionModel fails than the two states are not inserted in the state history, while this happens in the current implementation. I don't know the exact consequences of this.
Maybe @svwilliams can you helps us here? Thank you!

@servos
Copy link

servos commented Nov 26, 2024

@svwilliams any update on getting this merged?

jacobians.data(),
jacobian_quat2rpy);

jacobian << J[0], J[1], J[2], J[3], J[4];

Choose a reason for hiding this comment

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

jacobian is a 15x15, right? But isn't this inserting 15 rows and 16 columns (since J[1] is 4 columns)? Sorry I keep coming back with random questions, thank you so much for making this PR!

I ran into this only while using predict while running in debug, where I hit an eigen_assert (Too many coefficients passed to comma initializer (operator<<)) that only happens in debug mode.

Comment on lines +496 to +514
fuse_core::Vector3d acceleration_linear;
if (params_.predict_with_acceleration) {
acceleration_linear << acceleration_output.accel.accel.linear.x,
acceleration_output.accel.accel.linear.y, acceleration_output.accel.accel.linear.z;
}

predict(
position,
orientation,
velocity_linear,
velocity_angular,
acceleration_linear,
dt,
position,
orientation,
velocity_linear,
velocity_angular,
acceleration_linear,
jacobian);
Copy link

@henrygerardmoore henrygerardmoore Dec 13, 2024

Choose a reason for hiding this comment

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

should acceleration_linear be initialized to zero here? otherwise predict will use an uninitialized value if you aren't predicting with acceleration.

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

Successfully merging this pull request may close these issues.

9 participants