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

Proper way of exiting when using ROS #54

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

jahaniam
Copy link

Since this library is going to be used with ROS and withoutROS, it should be compatible for both.
In the current code, when you close pangolin you are calling exit(0). In the case of using ROS for an online camera, the code after "ros::spin" will never execute unless you call ros::shutdown instead of exit(0) in Pangolin wrapper.

So unlike running without ROS, in DSO_ROS code, these lines will never execute (line 228-238) when you are using ROS:

for(IOWrap::Output3DWrapper* ow : fullSystem->outputWrapper)
 {
 ow->join();
 delete ow;
 }

 delete undistorter;
 delete fullSystem;

#define EIGEN_ALIGN32 EIGEN_ALIGN_TO_BOUNDARY(32)
to the coarseinitializer.h

tested for eigen 3.2 and 3.3
including ROS in Pangolin for ros::shutdown if user has ROS installed
@jahaniam
Copy link
Author

jahaniam commented Apr 17, 2017

you can neglect the last second commit ("going back to before eigen fix pull request. Fixing issue just by adding ... ") if you want to, but then you still need to fix the eigen error for DSO_ROS.

@NikolausDemmel
Copy link
Collaborator

What happens when you compile dso while ROS is installed and available, but you want to use it without dso_ros? With your change, wouldn't it fail to shutdown in that case?

Rather then detecting ROS at compile time, maybe there could be a way to make the "exit" function configurable, defaulting to exit() and being overwritten in the dso_ros case.

Why is this PR suggesting to revert the eigen alignment changes?

@jahaniam
Copy link
Author

With your change, wouldn't it fail to shutdown in that case? No, it will still be possible to run it without ROS even if you installed ROS.
For your second comment yes maybe it is possible but I didnt know how to do it without overwriting it.
Why is this PR suggesting to revert the eigen alignment changes? The reason is when I did that change it was before eigen alignement. My changes doesn't include eigen stuff . If you can run the code after eigen alignment that is OK you dont need to go back but I think after they did the eigen alignment they changed some function interfaces but they didn't change the interface in DSO_ROS code. I dont know why they did a lot of change for eigen alignment when it could be fixed only by adding one line.
All these changes are for saving the keyframes in a file after you close the pangolin when you are using ROS and using your own bagfiles. If you don't need this,you dont need to merge.

@NikolausDemmel
Copy link
Collaborator

With your change, wouldn't it fail to shutdown in that case?

No, it will still be possible to run it without ROS even if you installed ROS.

But if you call ros::shutdown(), it won't exit the non-ROS Pangolin GUI application if you are not running a ros::spin() loop, or would it?

Why is this PR suggesting to revert the eigen alignment changes?

The reason is when I did that change it was before eigen alignement. My changes doesn't include eigen stuff . If you can run the code after eigen alignment that is OK you dont need to go back but I think after they did the eigen alignment they changed some function interfaces but they didn't change the interface in DSO_ROS code.

I think it would be great if your fix for the exit() when using dso with dso_ros could be merged (otherwise, what is the point of a pull request?). But as is, with reverting other unrelated changes, I guess it cannot be merged.

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.

None yet

2 participants