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

Sensors from JSON and more #2821

Closed
wants to merge 8 commits into from
Closed

Conversation

ACLeighner
Copy link

Proposing a few changes here:
-Added ability to load parameters for JSON for Barometer, Distance Sensor, IMU, GPS, Magnetometer
-Small change to python types doc, fixes a change I proposed before with better datatypes (float vs quaternion) for distance sensor
-Small change to documentation on linux build, sometimes deleting the 'binaries' and 'intermediate' folders in your unreal project after rebuilding airsim can fix errors where unreal tries to open content that doesnt exist anymore and causes the 'try rebuilding from source' error
-small change to move on path, replaces the position PID to use the velocity PID when traveling at constant altitude, elimnates shake problems

TODO:
-did not update documentation to show the new fields available for settings...

@ghost
Copy link

ghost commented Jul 1, 2020

CLA assistant check
All CLA requirements met.

Copy link
Contributor

@rajat2004 rajat2004 left a comment

Choose a reason for hiding this comment

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

Great PR! Adding settings for all the sensors will be a great addition. Just a few comments -

  • Would be great if the indentation would be consistent, tabs and spaces are intermixed, will be better to use 4 spaces throughout
  • There's an open PR Distance sensor Upgrades #2807 which exposes the settings for Distance sensor, along with adding an option for drawing debug point like Lidar. It might be better to do all the Distance sensor changes in that PR since will make it easier to review and avoid merge conflicts

"Version" : "1.3.1",
"VersionName": "1.3.1",
"Version" : "1.2.0",
"VersionName": "1.2.0",
Copy link
Contributor

Choose a reason for hiding this comment

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

Probably unintentional change, should be reversed

Copy link
Contributor

Choose a reason for hiding this comment

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

agreed

@@ -95,6 +95,7 @@ Alternatively, you can use [APIs](apis.md) for programmatic control or use the s
```
* If this is the case then look for *.gch file(s) that follows after that message, delete them and try again. Here's [relevant thread](https://answers.unrealengine.com/questions/412349/linux-ue4-build-precompiled-header-fatal-error.html) on Unreal Engine forums.
* If you see other compile errors in console then open up those source files and see if it is due to changes you made. If not, then report it as issue on GitHub.
* Additionally, if you are having trouble rebuilding the plugin in UE4 after making changes to the AirSim library, you may need to manually delete the binaries and intermediate folders in both your Unreal Project folder, and in the UnrealProject/Plugin/AirSim folder. This will force clang to recompile both the UE4 project and plugin and may resolve issues of "clang could not find file x"

Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe add a line here regarding clean.sh which does all this

@@ -24,6 +24,8 @@ struct ImuSimpleParams {
https://www.invensense.com/wp-content/uploads/2015/02/MPU-6000-Datasheet1.pdf
For Allan Variance/Deviation plots see http://www.invensense.com/wp-content/uploads/2015/02/MPU-3300-Datasheet.pdf
*/
Pose relative_pose;
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't see this being used anywhere?

Copy link
Contributor

Choose a reason for hiding this comment

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

yes, although relative pose is being consumed in the initialization of settings, it is not incorporated into the sensor model.

Copy link
Contributor

Choose a reason for hiding this comment

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

@ACLeighner can you either remove this setting or make the corresponding changes in the sensor model. (My guess would be you were in the middle of doing the same?)

@@ -19,9 +19,39 @@ struct GpsSimpleParams {
real_T update_frequency = 50; //Hz
real_T startup_delay = 1; //sec

Pose relative_pose;
Copy link
Contributor

Choose a reason for hiding this comment

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

Unused

@rajat2004
Copy link
Contributor

Also, I'm not very sure about exposing each setting of the sensors such as noise, but if a user is modifying those, then they probably know what they're doing, so shouldn't be a problem. Though some explanation for all of them will need to be added

@@ -24,6 +24,8 @@ struct ImuSimpleParams {
https://www.invensense.com/wp-content/uploads/2015/02/MPU-6000-Datasheet1.pdf
For Allan Variance/Deviation plots see http://www.invensense.com/wp-content/uploads/2015/02/MPU-3300-Datasheet.pdf
*/
Pose relative_pose;
Copy link
Contributor

Choose a reason for hiding this comment

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

yes, although relative pose is being consumed in the initialization of settings, it is not incorporated into the sensor model.

@@ -24,6 +24,8 @@ struct ImuSimpleParams {
https://www.invensense.com/wp-content/uploads/2015/02/MPU-6000-Datasheet1.pdf
For Allan Variance/Deviation plots see http://www.invensense.com/wp-content/uploads/2015/02/MPU-3300-Datasheet.pdf
*/
Pose relative_pose;
Copy link
Contributor

Choose a reason for hiding this comment

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

@ACLeighner can you either remove this setting or make the corresponding changes in the sensor model. (My guess would be you were in the middle of doing the same?)

max_distance = Quaternionr()
distance = np.float32(0)
min_distance = np.float32(0)
max_distance = np.float32(0)
Copy link
Contributor

Choose a reason for hiding this comment

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

this can be removed as #2807 is in

if (std::abs(cur.z() - dest.z()) <= getDistanceAccuracy()) //for paths in XY plan current code leaves z untouched, so we can compare with strict equality
moveByVelocityZInternal(velocity_vect.x(), velocity_vect.y(), dest.z(), yaw_mode);
moveByVelocityInternal(velocity_vect.x(), velocity_vect.y(), 0, yaw_mode);
Copy link
Contributor

Choose a reason for hiding this comment

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

can you move the moveOnPath changes to a separate PR?
Can you share a test script to showcase this fixed the shaking issue?

"Version" : "1.3.1",
"VersionName": "1.3.1",
"Version" : "1.2.0",
"VersionName": "1.2.0",
Copy link
Contributor

Choose a reason for hiding this comment

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

agreed

print("make sure we are hovering at 7 meters...")
client.moveToZAsync(z, 1).join()
client.moveToZAsync(z, 5).join()
Copy link
Contributor

Choose a reason for hiding this comment

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

why is this change needed, can you please justify it?
seems like a leftover from your testing?

@madratman
Copy link
Contributor

Also conflicts need to be resolved.

@rajat2004
Copy link
Contributor

@ACLeighner This branch now contains commits not related to the PR, cause its made from the master branch. It'll be best to create a new PR from the original commits using a separate branch if still interested in getting this feature in

@jonyMarino
Copy link
Collaborator

Hi @ACLeighner! Thanks for your contribution! I think it is time to merge it. Can you rebase your PR and solve the conflicts? Thanks!

evroon added a commit to evroon/AirSim that referenced this pull request Mar 22, 2021
@evroon
Copy link
Contributor

evroon commented May 19, 2021

This issue can be closed, its features have already been merged into master with commit 006d4e2.

@jonyMarino
Copy link
Collaborator

Thanks, @evroon for highlighting it

@jonyMarino jonyMarino closed this May 26, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants