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

Camera replaced by Cinematographic Camera (CinemAirSim) #3949

Merged
merged 12 commits into from
Dec 21, 2021

Conversation

ppueyor
Copy link
Contributor

@ppueyor ppueyor commented Aug 10, 2021

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

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.

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

AirLib/include/api/RpcLibClientBase.hpp Outdated Show resolved Hide resolved
AirLib/include/api/RpcLibClientBase.hpp Outdated Show resolved Hide resolved
AirLib/src/api/RpcLibClientBase.cpp Outdated Show resolved Hide resolved
AirLib/src/api/RpcLibServerBase.cpp Outdated Show resolved Hide resolved
PythonClient/airsim/client.py Show resolved Hide resolved
@ppueyor
Copy link
Contributor Author

ppueyor commented Aug 17, 2021

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
Copy link
Contributor

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

PythonClient/airsim/client.py Outdated Show resolved Hide resolved
Unreal/Plugins/AirSim/Source/PIPCamera.cpp Outdated Show resolved Hide resolved
Unreal/Plugins/AirSim/Source/PIPCamera.cpp Outdated Show resolved Hide resolved
Copy link
Contributor

@zimmy87 zimmy87 left a comment

Choose a reason for hiding this comment

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

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.

Unreal/Plugins/AirSim/Source/PIPCamera.cpp Outdated Show resolved Hide resolved
Unreal/Plugins/AirSim/Source/PIPCamera.cpp Show resolved Hide resolved
Unreal/Plugins/AirSim/Source/PIPCamera.cpp Outdated Show resolved Hide resolved
Unreal/Plugins/AirSim/Source/PIPCamera.h Outdated Show resolved Hide resolved
Unreal/Plugins/AirSim/Source/PIPCamera.h Show resolved Hide resolved
@zimmy87
Copy link
Contributor

zimmy87 commented Sep 9, 2021

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
Copy link
Contributor Author

ppueyor commented Sep 22, 2021

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?

Copy link
Contributor

@zimmy87 zimmy87 left a comment

Choose a reason for hiding this comment

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

Additional comment regarding a build failure

Unreal/Plugins/AirSim/Source/PIPCamera.cpp Outdated Show resolved Hide resolved
@zimmy87
Copy link
Contributor

zimmy87 commented Nov 2, 2021

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 ppueyor force-pushed the master branch 2 times, most recently from d0c708b to bca2944 Compare November 5, 2021 15:57
@ppueyor
Copy link
Contributor Author

ppueyor commented Nov 8, 2021

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
Copy link
Contributor Author

ppueyor commented Dec 16, 2021

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

Copy link
Contributor

@zimmy87 zimmy87 left a comment

Choose a reason for hiding this comment

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

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.

Unreal/Plugins/AirSim/Source/PIPCamera.cpp Show resolved Hide resolved
Unreal/Plugins/AirSim/Source/PIPCamera.cpp Outdated Show resolved Hide resolved
@zimmy87
Copy link
Contributor

zimmy87 commented Dec 17, 2021

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.

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.

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!

PythonClient/airsim/client.py Outdated Show resolved Hide resolved
Unreal/Plugins/AirSim/Source/PIPCamera.cpp Outdated Show resolved Hide resolved
Unreal/Plugins/AirSim/Source/PIPCamera.cpp Outdated Show resolved Hide resolved
Unreal/Plugins/AirSim/Source/PIPCamera.cpp Outdated Show resolved Hide resolved
Unreal/Plugins/AirSim/Source/PIPCamera.cpp Outdated Show resolved Hide resolved
Unreal/Plugins/AirSim/Source/PIPCamera.cpp Show resolved Hide resolved
Unreal/Plugins/AirSim/Source/PIPCamera.cpp Outdated Show resolved Hide resolved
Unreal/Plugins/AirSim/Source/PIPCamera.cpp Outdated Show resolved Hide resolved
Unreal/Plugins/AirSim/Source/PIPCamera.cpp Show resolved Hide resolved
Unreal/Plugins/AirSim/Source/PIPCamera.h Outdated Show resolved Hide resolved
@ppueyor
Copy link
Contributor Author

ppueyor commented Dec 20, 2021

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 ppueyor requested a review from zimmy87 December 20, 2021 13:35
@ppueyor
Copy link
Contributor Author

ppueyor commented Dec 20, 2021

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
Copy link
Contributor

zimmy87 commented Dec 21, 2021

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.

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.

CinematicCamera on Drones Cine camera
4 participants