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

Programmable camera distortion #3039

Merged
merged 15 commits into from
Dec 17, 2020
Merged

Conversation

saihv
Copy link
Contributor

@saihv saihv commented Sep 25, 2020

Introduces programmable camera distortion through a distortion material and a material parameter collection hosting the Brown distortion coefficients (3 radial, 2 tangential). Coefficients can be changed dynamically through the API.

Currently only works for the scene (RGB) camera.

@m1baldwin
Copy link

@saihv Will there be a separate PR to also allow the ROS wrapper for airsim to retrieve the distortion parameters of the camera and publish them in the 'D' field of the sensor_msgs/Image message that it publishes per-camera?

Copy link
Contributor

@ironclownfish ironclownfish left a comment

Choose a reason for hiding this comment

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

This looks good to me! There are a couple unnecessary line deletes/adds here and there, but in my opinion we don't need to worry so much about a small amount of such things.
Cool feature!

@saihv
Copy link
Contributor Author

saihv commented Oct 3, 2020

@m1baldwin I just added a getDistortionParams() API to retrieve the current distortion params of a camera. Perhaps you can reference this function from the ROS side as part of your PR to populate the distortion related fields?

@saihv saihv removed the request for review from madratman October 3, 2020 07:25
@@ -201,7 +200,7 @@ void ASimModeBase::initializeTimeOfDay()
sky_sphere_ = sky_spheres[0];
static const FName sun_prop_name(TEXT("Directional light actor"));
auto* p = sky_sphere_class_->FindPropertyByName(sun_prop_name);
UObjectProperty* sun_prop = Cast<UObjectProperty>(p);
FObjectProperty* sun_prop = Cast<UObjectProperty>(p);
Copy link
Contributor

Choose a reason for hiding this comment

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

This change breaks UE 4.24, which is okay IMO, just need to update docs to 4.25
It still produces warnings on 4.25, so it should be fixed completely like
FObjectProperty* sun_prop = CastField<FObjectProperty>(p)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good catch! I will revert this, because officially we still wish to support older UE versions.

Copy link
Contributor

Choose a reason for hiding this comment

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

Sure, that's also good, however I think the latest PRs like the segmentation one and even this has asset modifications which were done on 4.25 I assume, and wouldn't work in 4.24. Need to test all this though
While trying to maintain compatibility with 4.24, it might be good to update the docs to recommend 4.25, since some features could stop working on 4.24 and also raise support questions. Currently docs say 4.22 is supported, which I'm pretty sure is not the case

@rajat2004
Copy link
Contributor

Also, this would normally be caught by Travis, but needs some non-implemented methods in Unity/PawnSimApi so as to not break Unity compilation
https://travis-ci.com/github/rajat2004/AirSim/builds/187945054

@m1baldwin
Copy link

@m1baldwin I just added a getDistortionParams() API to retrieve the current distortion params of a camera. Perhaps you can reference this function from the ROS side as part of your PR to populate the distortion related fields?

@saihv sure, I think this is a better approach to use the new api rather than how I have it hard coded currently. I need to update the PR.

@m1baldwin
Copy link

Is it fine to use “plumb bob” model as the distortion model string here?

@saihv
Copy link
Contributor Author

saihv commented Oct 5, 2020

@m1baldwin Yes, plumb bob is appropriate.

@m1baldwin
Copy link

m1baldwin commented Oct 6, 2020

@saihv What is the difference between the simGetDistortion and getDistortion methods you have added? And, would the ros wrapper need to select between the two?

Update on this; it seems I am getting a seg fault when I try this in the ros wrapper. I am using simGetDistortion. When I try with the python API, I get "AttributeError: 'MultirotorClient' object has no attribute 'simGetDistortionParams'"

@saihv
Copy link
Contributor Author

saihv commented Oct 7, 2020

@m1baldwin Can you doublecheck if you're using the right Python client? (i.e., the one in this branch and not another pre-installed version)

simGetDistortionParams is the user-accessible API call, getDistortionParams is its internal corresponding method. The ROS wrapper would use the simGetDistortionParams one.

@rajat2004
Copy link
Contributor

@m1baldwin Which version of UE are you using? I think you'll have to use the version used by @saihv (probably 4.25.3) or later, since earlier versions will most likely fail to load the assets
As for the Python API, you'll have to use the files in this branch, You could create a file in the multirotor/, etc folders and use import setup_path. Another way which I use is to install airsim using pip install -e . from Pythonclient folder, that'll use the local package. Useful if changing branches, etc. since no need to use setup_path for quick tests

@m1baldwin
Copy link

@rajat2004 I had upgraded to UE4.25.3 for the purpose of testing this PR with the ros wrapper. Everything seems to run and build fine. For the python client, might be that I am not using the changes in the local, so I will check that. That will be the first thing I check. I merged this PR into my PR I had been testing to bring them to the same place, I had made minimal changes so what I am testing now seems to be mostly this PR.

Assuming this isn't an issue though for the cpp codes, I wonder why I am getting a seg fault?

Also, I have noticed that if I remove the simGetDistortion API from the wrapper, and then run the wrapper as is, seemingly randomly the simulation is breaking. In VScode, it is throwing a debug breakpoint at line 494 in the enableCapture function in PIPCamera (https://github.com/saihv/AirSim/blob/PR/distortion/Unreal/Plugins/AirSim/Source/PIPCamera.cpp#L494). This breakout only happens when I run something which is continuously calling the simGetImages API (ros_wrapper in this case)

@rajat2004
Copy link
Contributor

@m1baldwin Could you perhaps try out #3064? It might be causing the problem, it reverts the PR #2881, I'm not sure why this didn't appear when I tested with 10-15 min runs on my system, and with other testers as well. But seems to be same as here, so reverted the PR

@m1baldwin
Copy link

@rajat2004 sure, I could test with that next. Does it work with ue4.25.3?

@rajat2004
Copy link
Contributor

Yeah, it only has C++ code change, no asset modifications so works with any UE version

@m1baldwin
Copy link

m1baldwin commented Oct 7, 2020

@rajat2004 okay. It is weird, because I thought I had been using the latest master anyway before this, and I wasn't seeing this failure either. It was only when I tried this PR that the issue popped up. Maybe I was using a version of master from before Sep. 2? (edit, I was using the version from sep.2 with the image capture rate improvement)

@m1baldwin
Copy link

@rajat2004 Just to confirm, PR #3064 does fix the issue for me. The weird thing is I went back and double checked what I had been running previously. All last week, I was running the latest master which included your original PR to improve the image capture rate. For some reason after testing this PR, I started seeing the failure I described above.

I tried resetting everything to what I had last week, but now I see the failure still. Testing the PR you recommended works, but now of course my frame rate is low again. I am going to need to spend some more time testing as I will need the higher fps for my work.

Copy link

@tool3210 tool3210 left a comment

Choose a reason for hiding this comment

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

It looks good.

@jonyMarino jonyMarino closed this Dec 15, 2020
@jonyMarino jonyMarino reopened this Dec 15, 2020
@jonyMarino
Copy link
Collaborator

Hi @saihv. We are getting this from Unity build:

C:\Users\USER\source\repos\AirSim\Unity\AirLibWrapper\AirsimWrapper\Source\PawnSimApi.cpp(231): error C4716: 'PawnSimApi::getDistortionParams': must return a value [C:\Users\USER\source\repos\AirSim\Unity\AirLi
bWrapper\AirsimWrapper\AirsimWrapper.vcxproj]

@saihv
Copy link
Contributor Author

saihv commented Dec 15, 2020

@jonyMarino Fixed

@jonyMarino jonyMarino merged commit 5196910 into microsoft:master Dec 17, 2020
@jonyMarino
Copy link
Collaborator

Thank you, Sai! Great contribution! I had fun testing it.

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.

6 participants