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

Removed unnecessary redefinition of 'readParameters()' #102

Merged
merged 6 commits into from Apr 23, 2019

Conversation

Projects
None yet
3 participants
@MattDom
Copy link
Contributor

commented Apr 17, 2019

Refer to issue #101

@maximilianwulf

This comment has been minimized.

Copy link
Contributor

commented Apr 18, 2019

Thank you very much for the pull request. We will check it.

@maximilianwulf

This comment has been minimized.

Copy link
Contributor

commented Apr 18, 2019

Hi @MattDom, it seems rather that the function readParameters() is missing SensorProcessorBase::readParameters(); from the base class. In this function, the frames for tf are set. Can you try this instead of deleting it?

bool PerfectSensorProcessor::readParameters()
{
  SensorProcessorBase::readParameters();
  return true;
}

The reasoning behind the PerfectSensor class is that it does not need the noise parameters. The other classes such as StereoSensorProcessor assign a default value of zero to the noise parameters. This is why it works in the end.

It would be great if you could test it. It seems that a lot of issues here could be solved with it.

@maximilianwulf
Copy link
Contributor

left a comment

Test changes as explained in the comments to this PR.

Add base class method to readParameters()
Refer to #102 for details on that
@MattDom

This comment has been minimized.

Copy link
Contributor Author

commented Apr 18, 2019

Thanks for the reply. And yes, I admit that this was the quick'n'dirty solution. I changed it according to your suggestion.

Btw: It seems, that I broke the formatting a bit in the include file. It is not worth a new commit, so it should be corrected with the next commit. I'm sorry for that.

@maximilianwulf

This comment has been minimized.

Copy link
Contributor

commented Apr 18, 2019

Thank you for the update. Did it solve your problems from #101?

@MattDom

This comment has been minimized.

Copy link
Contributor Author

commented Apr 18, 2019

Yes, I tested it with two different settings and it works like charm.

@maximilianwulf

This comment has been minimized.

Copy link
Contributor

commented Apr 23, 2019

Thank you very much for the changes and finding the bug. I would be happy if you could take care of the last comment?

@pfankhauser or @goodfaiter could one of you also give the okay?

@pfankhauser
Copy link
Member

left a comment

Thanks, please check if tabs/spaces are clean.

MattDom added some commits Apr 23, 2019

@MattDom

This comment has been minimized.

Copy link
Contributor Author

commented Apr 23, 2019

I fixed some wrong whitespaces and tabs. However, the file should be pretty again.

@maximilianwulf
Copy link
Contributor

left a comment

Thank you very much for the changes, they are almost perfect. Your PR will be merged after you address these.

@maximilianwulf maximilianwulf removed the request for review from goodfaiter Apr 23, 2019

@MattDom

This comment has been minimized.

Copy link
Contributor Author

commented Apr 23, 2019

Okay, I adjusted the white spaces according to the style guide. I'm sorry, there were a few unintended tab spaces spaces left.

@maximilianwulf
Copy link
Contributor

left a comment

Sorry for the strict requests, but this is really the last comment.

@MattDom

This comment has been minimized.

Copy link
Contributor Author

commented Apr 23, 2019

No need to apologize! Okay, here is a sample from what I understand. The points stand for the whitespace:

..virtual bool computeVariances(
......const pcl::PointCloud<pcl::PointXYZRGB>::ConstPtr pointCloud,
......const Eigen::Matrix<double, 6, 6>& robotPoseCovariance,
......Eigen::VectorXf& variances);

At first two and than additionally four for the parameters.

@maximilianwulf

This comment has been minimized.

Copy link
Contributor

commented Apr 23, 2019

Exactly!

Depending on your IDE you can add clang-format to your toolchain. The tool automatically formats your cpp code.

Added missing white spaces
Added missing white spaces to fulfill style requirements
@maximilianwulf
Copy link
Contributor

left a comment

Thank you again for your contribution!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.