Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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 lidar measurement model class #195
Make lidar measurement model class #195
Changes from 1 commit
e619675
81cf634
0a7a366
7e6e2ad
2b8bff5
26e606b
4ba9695
2982960
59645ad
c7de317
75d130c
4c5350c
7caf1e0
3c87670
2d5d062
d024a99
a28be14
6c4befc
5cd5ff2
9a8a451
0d8990a
f9444b8
b37c72e
de81133
b2e8e71
a038fad
257a144
49572be
239b387
2efa4a7
cca96e3
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this is odd. how come you force to use only float? as it's template, a programmer might give other types, too.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it's very complicated to static_assert templated class.
allowing
pf::ParticleBase<float>
and alsopf::ParticleBase<double>
would be enough for now.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
IMO, this interface class doesn't need to take STATE_TYPE as class templates.
STATE_TYPE is used only for an argument of measure function. So, for me up-casting to pf::ParticleBase by the function argument is sufficient.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Basically, this makes sense for me.
However in this case, since
pf::ParticleBase
is templated, it is not allowed to be used as an argument of virtual function.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If you bind interfaces by pure virtual function, yes. Taking a derived class via class template parameter, and static cast from this pointer to a derived class for each member functions allows you to make common function interfaces that has own template parameters.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Come to think of it, these lidar measurement models are available only for
State6DOF
, not for general pf::ParticleBase.So, just support
State6DOF
would be enough for this.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
sounds reasonable.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
define measurement result type
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
on this PR?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
oh, I've forgotten this. thanks.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
num_points_default_
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
copy before erase
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
move this template to sample().