-
Notifications
You must be signed in to change notification settings - Fork 89
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
Ground speed sensor #161
Ground speed sensor #161
Conversation
@smnschfr would you be able to test this? If you haven't setup unreal engine, I can provide you with a packaged version of the game if that helps you. Let me know what you need 👍 |
Hi @SijmenHuizenga , yes we'll gladly test this. A packaged version would be appreciated. |
Thanks @smnschfr !! This is the packaged download link. This link expires on 26 juli. |
case SensorBase::SensorType::GSS: | ||
// at this moment the codebase requires every sensor to have settings. | ||
// However, gss does not have settings... so we just place ImuSettings in here for now. | ||
sensor_setting = std::unique_ptr<SensorSetting>(new ImuSetting()); |
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.
how does this work? Can you give an example of a settings.json that has an IMU and a GSS?
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.
The new ImuSetting()
doesn't do anything with the imu settings in the json, it just creates an empty settings object.
The problem is that sensor_setting
needs to have a valid object and I don't want to create the GSSSettings
class because gss doesn't have settings. The good solution would be to change the abstraction structure to allow sensors without settings. But I don't want to do that because lotsofwork and notthatusefullreally.
Hi @SijmenHuizenga If it's not too much effort, I'd suggest using the same data representation as in the real sensor. |
@smnschfr Yes, good idea 👍 See above commit with the implementation. Now, driving around it looks like this: different scale to see the z changing: |
@smnschfr Is there anything else you would like to see changed before we merge it? |
@SijmenHuizenga I'm wondering why the curve doesn't look completely discrete (stair shape) at some points. Is that just the image resolution? Otherwise looks fine to me. And thanks for implementing that so quickly! |
Are we adding noise to this sensor? |
At this moment I added no noise and am not planning to do so. My rational being that the kistler is this precise as well. When smoothing the kistler 250hz into the 100hz it is basically ground truth. I wrote a bit on this here and in the docs in this pr. |
Well spotted! This is because the ros bridge requests the information at a higher frequency then the physics simulation updates the vehicle. I'm not sure we can fix this, but i'm trying in #166 The architecture of this project (dictated by airsim) requires data pulling from the simulation in unreal so there is no way (yet) to synchronize it. So for now we have to live with this staircase datathing. |
It seems like everything has been resolved, so let's merge! |
closes #92