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

Stop ceres problem from freeing loss function #519

Merged
merged 1 commit into from
Jul 18, 2022
Merged

Stop ceres problem from freeing loss function #519

merged 1 commit into from
Jul 18, 2022

Conversation

john-maidbot
Copy link
Contributor

@john-maidbot john-maidbot commented Jul 18, 2022


Basic Info

Info Please fill out this column
Ticket(s) this addresses N/A
Primary OS tested on Ubuntu
Robotic platform tested on gazebo simulation

Description of contribution in a few bullet points

  • Stop ceres problem from freeing the loss function when we reset the solver. This is important because otherwise calling solver_->Reset() invalidates the loss_function_ pointer held by the solver_ (whereas I would expect the behavior of solver_->Reset() to be that the loss function is left unchanged). Open to alternative ways to address this problem, but this worked for my use case.
  • It also seemed like the blocks_ object should be freed in the ~CeresSolver() function (since I don't think the problem takes ownership of this).
  • I ran into this issue because in my use case, I need to cleanly reset the slam toolbox state multiple times without killing the process (funny enough you don't run into major issues until the second time you call solver_->Reset()). I also think this would affect people if they needed to call deserialize pose graph multiple times.

Description of documentation updates required from your changes

None that I know of.


Future work that may be required in bullet points

  • I hope to make a future PR with the reset function if it makes sense for general use cases. atm it seems unique to my use case.

Copy link
Owner

@SteveMacenski SteveMacenski left a comment

Choose a reason for hiding this comment

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

LGTM, thanks!

@SteveMacenski SteveMacenski merged commit 348b3a4 into SteveMacenski:ros2 Jul 18, 2022
malban pushed a commit to malban/slam_toolbox that referenced this pull request Jul 19, 2022
SteveMacenski pushed a commit that referenced this pull request Jul 19, 2022
SteveMacenski added a commit that referenced this pull request Aug 24, 2022
* Publish pose with covariance in localization mode. (#511)

* Visualize localization nodes and edges with different colors in the pose graph marker message. (#513)

* Fix edge marker id to not conflict with the localization edge marker. (#515)

* Stop ceres problem from freeing loss function (#519)

* Add map_and_localization node that supports toggling between mapping and localization modes with a service. (#512)

* resolve API change from galactic (#524)

* Fix segfault after loading posegraph (#527)

* Assign mapper to closure_assistant after loading posgraph

* Fix linting

* Midigate spamming for 50Hz lidar (#526)

* Increase scan filter queue_size

* Add scan_queue_size as parameter

Co-authored-by: Marc Alban <[email protected]>
Co-authored-by: john-maidbot <[email protected]>
Co-authored-by: Samuel Lindgren <[email protected]>
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.

2 participants