Add ability to record videos of dynamic scenarios #22
Conversation
There was a problem hiding this comment.
Thanks for preparing this PR! It looks good except for a few minor issues -- see comments below. The main things are to add documentation for the new features into the Sphinx documentation, and to clean up the scenic.simulators.carla.recording entry point (I'm not sure if you meant to include it, since it's in the CARLA folder but the -r option is set up so that multiple simulators could support it).
| recOptions = parser.add_argument_group('simulation recording options') | ||
| recOptions.add_argument('-r', '--record', help='enable recording of simulations', | ||
| action='store_true') | ||
| recOptions.add_argument('--sensors', help='path to sensor configuration file') |
There was a problem hiding this comment.
Can you add a section in docs/options.rst for "Recording Options" and an entry there with a link to the documentation about the format of the sensor configuration file?
| maxIterations=args.max_sims_per_scene) | ||
| maxIterations=args.max_sims_per_scene, | ||
| save_dir=args.recording_dir, | ||
| sensor_config=args.sensors) |
There was a problem hiding this comment.
Is there any reason to have both the Simulator.toggle_recording_sensors method and these extra arguments to Simulator.simulate? Why not just pass everything into simulate?
Also, since the --sensors option is required when using -r, why not just have -r take the path to the configuration file? Unless the configuration file is optional (e.g. if there is a reasonable default set of sensors), there's no point having two different command-line option.
| @@ -0,0 +1,180 @@ | |||
| # Scenic Data Generation Platform | |||
There was a problem hiding this comment.
This README will be buried inside the source code, so people won't be able to find it; we should move it into the Sphinx documentation. If it's really CARLA-specific, then you can have it as the documentation of the scenic.simulators.carla.recording module (moving it to the docstring of "src/scenic/simulators/carla/recording/init.py"; or, if it would be annoying to convert the Markdown to ReStructuredText, you could link to the .md file from there and use the "recommonmark" Sphinx extension).
|
|
||
| ## Setup | ||
|
|
||
| ### Installing CARLA |
There was a problem hiding this comment.
Some of this seems redundant with the CARLA instructions in docs/simulators.rst -- let's get rid of the redundant parts so that they don't get out of sync. Anything that is needed for recording that isn't needed for Scenic in general (e.g. the Additional Maps archive, if that really is needed) you can still describe here, of course.
|
|
||
| To generate a synthetic dataset using Scenic, you need two things: a scenario configuration file and a sensor configuration file. | ||
|
|
||
| ### Scenario Configuration |
There was a problem hiding this comment.
This stuff isn't used by the -r option, right? This is for the alternate scenic.simulators.carla.recording entry point. If you want to keep that entry point, the documentation needs to clarify the difference between using the main Scenic entry point with -r and using python -m scenic.simulators.carla.recording.
| Now, to actually generate data using the configurations above, simply run: | ||
|
|
||
| ``` | ||
| python -m scenic.simulators.carla.recording --scenarios /path/to/scenario/config --sensors /path/to/sensor/config |
There was a problem hiding this comment.
If you're going to keep this entry point, there should also be an example in the main documentation of how to use scenic -r, since it works differently (you pass a single scenario as usual, rather than using --scenarios to pass a config file).
| ### API Documentation | ||
|
|
||
| #### class DataAPI | ||
| * def get_simulations(self) |
There was a problem hiding this comment.
Not necessary to do now, but eventually it would be good to switch this to use autogenerated documentation from the docstrings of these classes and methods, for consistency with the rest of Scenic.
|
|
||
| parser = argparse.ArgumentParser(prog='scenic', add_help=False, | ||
| usage='scenic [-h | --help] [options] FILE [options]', | ||
| description='Sample from a Scenic scenario, optionally ' |
There was a problem hiding this comment.
This is misleading, since this entry point doesn't support most of Scenic's options. Please change prog, usage, and description to make clear this is a helper script for running multiple scenarios and collecting recordings.
(If you decide to keep this entry point. You could just get rid of it, or add the --scenarios functionality to the main Scenic entry point, which would allow you to drop the subprocess stuff.)
|
|
||
| def save_recordings(self, save_dir): | ||
| if not self.record_sensors: | ||
| print('No recordings saved; turn on recordings for simulator to enable') |
There was a problem hiding this comment.
If you want this to not be a fatal error, use warnings.warn instead; otherwise raise an exception.
| print('No recordings saved; turn on recordings for simulator to enable') | ||
| return | ||
|
|
||
| print('Started saving recorded data') |
There was a problem hiding this comment.
Use verbosePrint so that the --verbosity option can suppress this message.
…ts-vacuum-experiments implement scripts to collect data from vacuum example
No description provided.