-
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
Extend Recording to multiple vehicles #2861
Conversation
b8f983d
to
d9593dc
Compare
/azp run microsoft.AirSim |
Azure Pipelines successfully started running 1 pipeline(s). |
d9593dc
to
ca06159
Compare
The last commit fixes a bug in the PR which causes it to not save any images when VehicleName wasn't specified.
And here, if we change Edit: So images are not corrupted, I'm able to save them by using For compressed images, UE is doing the conversion to PNG format, and then we're just writing it as a binary file. For uncompressed, writing array values as binary doesn't work. I can see 2 options, either go for an external library such as |
recording_setting.requests.clear(); | ||
// Add Scene image for each vehicle | ||
for (const auto& vehicle : vehicles) { | ||
recording_setting.requests[vehicle.first].push_back(ImageCaptureBase::ImageRequest( |
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 strange part which I'll take a look at later, vehicles
map here doesn't contain a single vehicle when no vehicles are specified, but instead 3 (one for each simmode), and why I added getDefaultVehicleName
, rather than using the first vehicle in the map.
Not sure why it's being done like this, shouldn't a single default vehicle be created in initializeVehicleSettings()
instead of 3?
Can be seen by adding a line here, like Utils::log(vehicle.first)
.
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.
Due to how ASimModeBase::setupVehiclesAndCamera() is defined, only vehicles that match the current simmode get instantiated. The only other place where the vehicles map gets read is in AirSimSettings::getVehicleSetting(), which only gets called in VehicleSimApiBase, and VehicleSimApiBase objects have a 1:1 relationship with the vehicles that get instantiated. As a result, I am confident the change to initializeVehicleSettings() to only initialize a single default vehicle is a safe change to make.
a1ea180
to
faee309
Compare
faee309
to
6b75538
Compare
6b75538
to
90cec34
Compare
c7ff43a
to
ed78d6f
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.
Thanks for submitting this PR @rajat2004! I have a few small comments mostly dealing with naming and style conventions that I'd like to see addressed before merging.
5ce5c9b
to
0c5abba
Compare
Hey @rajat2004, the 2 failing checks are due to a build break caused by a different PR. The fix for this build break was committed yesterday, can you rebase this PR to pick up the fix and rerun all the checks? |
0c5abba
to
e90f9a0
Compare
Add `vehicles_specified` var to track whether Vehicles element was present in settings Also change all occurrences of simmode strings to fixed variables
Starts recording automatically when the simulation starts
Write uncompressed image as PPM file while Recording
Plus some cleanup
273e997
to
a1acd18
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.
tested this locally and it looks good to me; will discuss with my team about merging
recording_setting.requests.clear(); | ||
// Add Scene image for each vehicle | ||
for (const auto& vehicle : vehicles) { | ||
recording_setting.requests[vehicle.first].push_back(ImageCaptureBase::ImageRequest( |
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.
Due to how ASimModeBase::setupVehiclesAndCamera() is defined, only vehicles that match the current simmode get instantiated. The only other place where the vehicles map gets read is in AirSimSettings::getVehicleSetting(), which only gets called in VehicleSimApiBase, and VehicleSimApiBase objects have a 1:1 relationship with the vehicles that get instantiated. As a result, I am confident the change to initializeVehicleSettings() to only initialize a single default vehicle is a safe change to make.
Thank you for the contribution @rajat2004! |
Glad to help! Contributing to AirSim has definitely helped me improve my skills, and hope that this will be useful to others as well :) |
Bug report Example settings.json Versions: What's the issue you encountered? "pngcheck -v uncompressed_corrupt_img_0_0_1616710947452891700.png" returns: |
@Ryton This PR hasn't been included in the 1.4 release, you'll have to use the latest master |
Ah ok, thanks! i was not aware of this. |
Fixed now, using the latest master so NOT:
But rather, within CameraSettings:
|
Replaces #2847 (branch got messed up)
Closes #3029
Closes #3123
Closes #3214
Other related issues - #2307
New Additions -
VehicleName
),VehicleName
parameter inCameras
field to specify specific vehicle to use the camera fromFolder
field in settings to change the directory to store data inEnabled
parameter for starting recording when AirSim starts without needing to press R or call API (How to start recording when game begins? #3029) (Any naming suggestions are welcome)Also fixes a bug when pressing Esc and Play multiple times causes requests to add in the vector and save multiple images (#1414 (comment))
Example settings -
Recording - 2020-07-11-11-48-51.zip
Testing -
TODO:
Further possible additions -
Documents
)- I suspect this might be a race condition for the first write, since there are no images, no rendering and saving of images is required, so writing of pose data is happening almost instantaneously, but the file to write the data in hasn't still been created.(Haven't confirmed this though, just a thought)
- Fixed (set
is_ready
flag at the end)Any testing (especially of
Folder
on Windows) or suggestions would be great!Came to mind when working on #2841
Related older issue #1346, Also includes the fix proposed in #1414