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

Make MINS working with pcl 1.12 and Eigen 3.4 #3

Merged
merged 20 commits into from
Oct 11, 2023

Conversation

lnexenl
Copy link
Contributor

@lnexenl lnexenl commented Oct 10, 2023

This is the initial pr message:

Destructors don't need to be called explicitly here. The smart ptr will do this when program exits.

If we print "SystemManager Destructor called!" in destructor of SystemManager, we can see that the destructor is called twice:
image

This also leads to error "corrupted double-linked list"

@lnexenl lnexenl changed the title [fix]: remove expicilitly explicitly called destructors Make MINS runs with pcl 1.12 and Eigen 3.4 Oct 10, 2023
@lnexenl
Copy link
Contributor Author

lnexenl commented Oct 10, 2023

I intend to fix a minor error here, but I decide to improve MINS to run with pcl 1.12 and Eigen 3.4. For my PC runs ubuntu 22.04 and ROS Noetic, MINS cannot be compiled natively on my PC.

I make multiple commits to make MINS compiles and runs on Ubuntu 18.04+ROS Melodic, Ubuntu 20.04+ROS Noetic and Ubuntu 22.04+ROS Noetic.

I test all these conditions with my PC and docker container by roslaunch mins simulation.launch. Each combination compiles and runs successfully.

To make MINS compiles with Eigen 3.4, I upgrade libpointmatcher to newest version. To make MINS compiles with pcl 1.12, I add two lines to ROSPublisher::publish_lidar_map()

I also add some dockerfiles and introduce a CI/CD progress to MINS. The latest CI/CD progress in my repo can be seen here

Hope this pr works.

@lnexenl lnexenl changed the title Make MINS runs with pcl 1.12 and Eigen 3.4 Make MINS working with pcl 1.12 and Eigen 3.4 Oct 10, 2023
sys->~SystemManager();
pub->~ROSPublisher();
op->~Options();
save->~State_Logger();
Copy link
Member

Choose a reason for hiding this comment

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

Hi! Thanks for looking at this problem!
I also actually looked into this and ended up having these explicit destructors.
Previously, when the system was terminated successfully, I got the following error message:

terminate called after throwing an instance of 'class_loader::LibraryUnloadException'
  what():  Attempt to unload library that class_loader is unaware of.

This error also shows up in OpenVINS, and I tried to resolve this issue ended up having those explicit destructors - SystemManager destructor seems to remove the error message.
I was aware of the double destruction issue, but at least I had a clean system termination on my terminal so left it as it was.
Will this double destruction be an issue?

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 see. I also find that removing explicit destructors will get these messages:
image

I think this error might be from image_transport. And the error really doesn't show on screen by calling destructors explicitly. But I think the error doesn't disappear, it might be hidden by error "corrupted double-linked list", which I think is more severe than 'class_loader::LibraryUnloadException'.

Copy link
Member

Choose a reason for hiding this comment

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

Okay, thank you for sharing your opinion. If you are finished with improving the compatibility of MINS, I will go through the test and merge your PR if all is good.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, I have done some tests and it seems that everything runs correctly and natively on my PC. And CI/CD also passes: action

RUN /bin/bash -c "chmod +x /opt/ros/noetic/setup.sh && source /opt/ros/noetic/setup.sh && catkin build --workspace /catkin_ws"

WORKDIR /catkin_ws
ENTRYPOINT ["/bin/bash"]
Copy link
Member

Choose a reason for hiding this comment

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

Both docker run checked.

@WoosikLee2510 WoosikLee2510 merged commit 4e09a0f into rpng:master Oct 11, 2023
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