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

Expose and use the proposed open_karto parameter, minimum time interval #11

Closed

Conversation

spmaniato
Copy link

@spmaniato spmaniato commented Aug 16, 2016

This PR is tied to ros-perception/open_karto#11 ⚠️

This is a follow-up to the open_karto PR mentioned above. It adds a new ROS param, minimum_time_interval, which is used to set the proposed open_karto param MinimumTimeInterval. It also passes the laser scan message's time stamp to open_karto enabling it to calculate the time interval between two scans.

Copy link
Contributor

@lucbettaieb lucbettaieb left a comment

Choose a reason for hiding this comment

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

Seems legit. I'd like to see better style in this file, so if you could, would you lint and add {}s?

Thanks!

bool use_scan_barycenter;
if(private_nh_.getParam("use_scan_barycenter", use_scan_barycenter))
mapper_->setParamUseScanBarycenter(use_scan_barycenter);

double minimum_time_interval;
if(private_nh_.getParam("minimum_time_interval", minimum_time_interval))
mapper_->setParamMinimumTimeInterval(minimum_time_interval);
Copy link
Contributor

Choose a reason for hiding this comment

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

if you would, @spmaniato , please add {}s to the if/else's in this file and do a full lint.

@lucbettaieb lucbettaieb assigned spmaniato and unassigned lucbettaieb Dec 25, 2017
@lucbettaieb
Copy link
Contributor

@spmaniato Would you like to check this out?

@spmaniato
Copy link
Author

@lucbettaieb Hey Luc, thanks for following up. I haven't worked on the Karto codebases in over a year. And unfortunately I don't have dev and sim environments set up to make and test any changes. And I don't feel comfortable pushing untested changes 😬

Moreover, if we were to do a full lint of this codebase, I bet it would come up with hundreds of issues. Fixing just this one file wouldn't help much. And we would be mixing my minimal functional changes with dozens of lines of linting-related changes, thus making the PR harder to review. May I recommend a separate PR that focuses entirely on linting and non-functional changes?

Sorry I cannot be more helpful at this time.

@lucbettaieb
Copy link
Contributor

@spmaniato I didn't intend to ask you to do a full lint of the codebase.. lol that would be ridiculous.
If you want to get this PR in, let me know when you can test it, etc, and remove all of the extra spaces and stuff that your commits introduce into the code. The changes are pretty minimal, so it shouldn't be much effort.

@spmaniato
Copy link
Author

@lucbettaieb

  1. As I said, I do not have the dev or sim setup to re-test this PR, I'm sorry 😕 I had tested it back when I first opened it. See the corresponding (merged) PR description in open_karto for the results: Process scan if enough time has elapsed since the previous one open_karto#11
  2. My PR doesn't introduce extra spaces. It removes whitespace at the end of lines, which is pretty a standard thing that most editors do automatically these days. I don't see how that's harmful.

If you do not want to merge it in its current state, that's understandable. But I cannot work on it now or in the foreseeable future. Feel free to close it if you prefer and I'll re-open when and if I can work on it.

@lucbettaieb
Copy link
Contributor

Oops - you're right about the spaces. My bad! Haven't looked at this in a while. 😅

Whenever you have time to test this, please let me know. No rush. Just wanted to ping you to see if you wanted to do anything with this.

@lucbettaieb
Copy link
Contributor

@spmaniato Are you still interested in merging this?

@spmaniato spmaniato closed this Oct 15, 2019
svwilliams added a commit to locusrobotics/slam_karto that referenced this pull request Oct 26, 2022
…tor-mapping-launch

RST-244 Wait for map->local transform
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