-
Notifications
You must be signed in to change notification settings - Fork 4.6k
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
Conversation
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 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
Hi @rajat2004, Thanks for your interest in this PR. My answers are below:
About the corrections, @zimmy87 is having a look at the code, so if he considers that, I can correct the code with your suggestions. |
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? |
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.
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.
Our Azure pipeline check is failing with the following build error:
Are you seeing this in local builds? |
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? |
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.
Additional comment regarding a build failure
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? |
d0c708b
to
bca2944
Compare
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. |
Hi @zimmy87, I implemented all the requested changes and updated my repo to the latest version of your master. 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 |
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 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.
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. |
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.
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!
Hi @zimmy87 and @rajat2004 , I resolved all the changes requested, hope everything is correct now. |
Does the "PostInitializeComponents" thing solve this? |
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. |
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