Camera replaced by Cinematographic Camera (CinemAirSim)

[ppueyor]

Fixes: #1489
Fixes: #2303
Fixes: #2311
-Integrates the CineCameraComponent into AirSim
-New PR from #3779

About

An AddOn for AirSim that includes all the tools to integrate a cinematographic camera and the needed tools to control/access to it.
More info available in repo: https://github.com/ppueyor/CinemAirSim
Or conference paper: https://arxiv.org/abs/2003.07664

How Has This Been Tested?

Ubuntu 20 and UE4 4.26
Different demos and API calls
Results available in repo

Screenshots:

https://www.youtube.com/watch?v=OJGNaitWZVA

[rajat2004]

The video and paper looks pretty awesome, and it's pretty nice that you want this to get merged in the project also
Just had a brief look, and had some concerns hope you might be able to answer, and might interest the maintainers also

• What's the performance diff if any between the current camera and the new one? Some FPS info would be good, Add image benchmark script #3889 might be useful. Also, does this support all the image types?
• Wondering whether replacing the current camera entirely might be a good idea or not. This adds a lot of camera APIs, and there's enough already. In case of problems with first point, whether having this as a separate camera might be a better option. A separate set of APIs (client) for camera might be something with looking into, it'll improve the current code also by avoiding mixing so many APIs

[ppueyor]

Hi @rajat2004,

Thanks for your interest in this PR. My answers are below:

• I used that script and the average FPS is 19.90, don't know how much it is in the regular camera. It does support all kinds of image types, and it is possible to get images and subwindows the same way and updates as the regular camera.

• Well, I think that it's something that the maintainers should answer, if that could be interesting or not. I can talk about my own experience. It opened me to a whole world of possibilities. If you want to use the drone with filming objectives (that it is wide use of the UAVs), this PR is more than helpful because u could not modify the intrinsics of the camera otherwise.

About the corrections, @zimmy87 is having a look at the code, so if he considers that, I can correct the code with your suggestions.

[rajat2004]

By the FPS, I meant some results to compare on your system itself, for the Cinematic and normal cameras, for different resolutions like 256x144, 720p, 1080p and maybe 4k also?
Makes sense, UAVs for filming is probably the most common usecase, other applications are also closely vision-related, and improvements in camera to model the real world would definitely be useful

[zimmy87]

Thanks for submitting this PR, this is a cool feature! Most of my feedback is style-related. I second @rajat2004's comment about replacing the default camera with the new UCineCameraComponent. I think the default user experience is different enough that there should be an option for switching to the new type of camera. It'd also be nice to see the rest of @rajat2004's review items addressed before merging this.

[zimmy87]

Our Azure pipeline check is failing with the following build error:

In file included from /__w/13/s/Unreal/Environments/Blocks/Plugins/AirSim/Intermediate/Build/Linux/B4D820EA/UE4/Development/AirSim/Module.AirSim.2_of_2.cpp:2:
/__w/13/s/Unreal/Environments/Blocks/Plugins/AirSim/Source/TextureShuffleActor.cpp:24:49: error: incomplete type 'UMaterialInstanceDynamic' named in nested name specifier
        DynamicMaterialInstances[material_id] = UMaterialInstanceDynamic::Create(DynamicMaterial, this);
                                                ^~~~~~~~~~~~~~~~~~~~~~~~~~
/home/ue4/ue-4.25.1-linux-debugeditor/Engine/Source/Runtime/Engine/Classes/Engine/BlendableInterface.h:10:7: note: forward declaration of 'UMaterialInstanceDynamic'
class UMaterialInstanceDynamic;
      ^
In file included from /__w/13/s/Unreal/Environments/Blocks/Plugins/AirSim/Intermediate/Build/Linux/B4D820EA/UE4/Development/AirSim/Module.AirSim.2_of_2.cpp:2:
/__w/13/s/Unreal/Environments/Blocks/Plugins/AirSim/Source/TextureShuffleActor.cpp:27:33: error: member access into incomplete type 'UStaticMeshComponent'
        components[component_id]->SetMaterial(material_id, DynamicMaterialInstances[material_id]);
                                ^
/home/ue4/ue-4.25.1-linux-debugeditor/Engine/Source/Runtime/CoreUObject/Public/Templates/Casts.h:22:7: note: forward declaration of 'UStaticMeshComponent'
class UStaticMeshComponent;
      ^
In file included from /__w/13/s/Unreal/Environments/Blocks/Plugins/AirSim/Intermediate/Build/Linux/B4D820EA/UE4/Development/AirSim/Module.AirSim.2_of_2.cpp:2:
/__w/13/s/Unreal/Environments/Blocks/Plugins/AirSim/Source/TextureShuffleActor.cpp:30:42: error: member access into incomplete type 'UMaterialInstanceDynamic'
    DynamicMaterialInstances[material_id]->SetTextureParameterValue("TextureParameter", SwappableTextures[tex_id]);
                                         ^
/home/ue4/ue-4.25.1-linux-debugeditor/Engine/Source/Runtime/Engine/Classes/Engine/BlendableInterface.h:10:7: note: forward declaration of 'UMaterialInstanceDynamic'
class UMaterialInstanceDynamic;
      ^
4 errors generated.

Are you seeing this in local builds?

[ppueyor]

Hi @zimmy87, I answered your suggestions/bug reports. I will fix everything once we decide what to do to integrate it.

No, I haven't seen that error of compilation in local builds. I'm using UE 4.26.1 in Ubuntu 20. Can this be the difference?

What are the next steps we should do? Should I wait for your instructions?

[zimmy87]

Additional comment regarding a build failure

[zimmy87]

Hi @ppueyor, apologies for the delay in getting back to you; I've responded to the remaining feedback items; please let me know if you have any other questions/concerns. Regarding the build failure in TextureShuffleActor.cpp, I'm still seeing this with your latest revision, so this looks like a legitimate build failure. My guess is you're missing an include somewhere (usually the "incomplete type" error occurs when only a forward declaration is present for a given class and the full header for that class is needed to access one of its members). We're currently using 4.25.1 on our build machines, and we'd like to maintain compatibility with 4.25, so using 4.26.1 could be a potential cause here. Would you be able to test locally with a 4.25.1 build?

[ppueyor]

Hi @zimmy87,

Thanks for the review. Yes, I will test it in Unreal 4.25.1 once we decide what to do with the cinematic flag in Settings.hpp.
I fixed the suggested changes. I will commit everything when it is all decided and ready.

[ppueyor]

Hi @zimmy87,

I implemented all the requested changes and updated my repo to the latest version of your master.
I talked to Jonny and we decided to set the focal length to match with the regular camera.

I tested it with ubuntu 20 and Unreal 4.25 and works perfectly.

Please, tell me if you need anything else.

P.S. : I needed many commits to bug simple bugs, if you could merge them in one, it will be cleaner, sorry for this

[zimmy87]

The new focal length looks good when I manually override the right camera, but unfortunately I'm not seeing this focal length applied to the default camera, so I still see the same narrow field of view with the latest iteration.

[zimmy87]

Hi @ppueyor, thanks for the update, I tested locally and noticed a few things that I think should get addressed before merging. I've gone through all previous review feedback and resolved everything that looks to have been addressed. I see that there's one remaining item of feedback from @rajat2004 on PythonClient/airsim/client.py that hasn't been addressed yet (it looks like this may not require a code change, feel free to resolve the comment yourself if that's the case). There also looks like there's one remaining item of feedback from my previous review about the names of SrcPPSettings, DstPPSettings, and DstWeightedBlendables in Unreal/Plugins/AirSim/Source/PIPCamera.cpp. These variables should be renamed to src_pp_settings, dst_pp_settings, and dst_weighted_blendables.

[rajat2004]

Some code improvement comments. Additionally, would be great if you could add some example scripts to test the various capabilities, and also some documentation.
Thanks!

[ppueyor]

Hi @zimmy87 and @rajat2004 , I resolved all the changes requested, hope everything is correct now.
There is some docu/demo work to do, but I don't have much time to do it right now. I will do it asap.

[ppueyor]

The new focal length looks good when I manually override the right camera, but unfortunately I'm not seeing this focal length applied to the default camera, so I still see the same narrow field of view with the latest iteration.

Does the "PostInitializeComponents" thing solve this?

[zimmy87]

Tested the latest iteration and it works well for me, so I am moving ahead with merging. It would be nice to add some extra documentation and examples, but I feel that can be handled in a separate PR.