-
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
Add support for fixed external cameras #3320
Conversation
e6f6069
to
c9b2cc1
Compare
f9e088c
to
7304f21
Compare
59e39f2
to
e6ab29c
Compare
e6ab29c
to
68bb0c8
Compare
9db28c8
to
7fe058e
Compare
7fe058e
to
1f53fa9
Compare
1f53fa9
to
2868340
Compare
It seems that you have a lot of conflicts. This is most likely due to conflicts with autoformatted code in master. Please follow these steps to resolve this:
Thanks for helping us with these style changes! |
@zimmy87 Yeah, there are major conflicts with this one, will be a pretty painful one. I'll try to get this done today, but I think it would be best to get #3472 in first since that API will also need to be adapted. This PR was just intended to see if the feature is feasible and useful. Any review regarding the code and the current API modification would be very much appreciated! |
763ea44
to
e8bc584
Compare
39d82a4
to
5689b05
Compare
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.
a few comments, plus I'm having an issue running the external_camera.py script
5689b05
to
b1f3b61
Compare
@rajat2004 great feature! thank you for the great work. |
Hmm, don't have any real preference either way, pretty straightforward to add that API. The listVehicles API itself becomes more useful with the addVehicle API. |
Also fix missing vehicle, external field
b1f3b61
to
79ee342
Compare
PR looks good to me; moving ahead with merging; thanks for the contribution @rajat2004! |
Fixes: #3208
Fixes: #1896,
Fixes: #1709,
Fixes: #610,
Fixes: #599
About
This PR adds the capability to create a fixed camera separate from the cameras on the vehicle, such as a CCTV camera. I'm not aware of any simple way to go about doing this currently, and therefore thought to try it out. I haven't checked the issues very thoroughly, so please let me know if there are other related issues and ways to go about doing this.
PR has been opened to get ideas and suggestions and whether this is actually required. This will likely become a big PR, so it can be split into parts for easier review also.
Currently have added a boolean field
external
in thegetCameraInfo
API, went for this since there are currently 7 camera related APIs, which could increase later on as well. Having completely separate APIs will duplicate the code in many places, the new API while doesn't look the cleanest felt like the better choice to me. Do comment if splitting into a different API or some other way is more preferred.TODO (Comment if something's missing) -
getImages
set/getDistortionParams
getCameraInfo
setCameraFov
setCameraPose
Also removes the
simSetCameraOrientation
API kept around for backward compatibilityThe last commit to add an internal
CameraDetails
struct is a NFC commit, just to clean up the multiple parameters being passed everywhere. If it's not desirable then the coomit can be dropped easily. Any suggestions on the struct naming are welcome!How Has This Been Tested?
Example Settings
Example API Call -
An example Python script has been added, the FoV change will be reflected in the info only after #3278
Script Output
Subwindws Testing:
Subwindow Settings
The
external_camera.py
can be easily used for testing all the camera APIs, as well as vehicle cameras by setting the flag. Thedetection.py
script has also been tested to be working after these changes.Screenshots (if appropriate):
Old FoV (90):
New Fov (120):
Subwindows:
airsim_cv_mode.zip