Skip to content

Enabling multi-drone control#706

Closed
saihv wants to merge 3 commits intomicrosoft:masterfrom
saihv:master
Closed

Enabling multi-drone control#706
saihv wants to merge 3 commits intomicrosoft:masterfrom
saihv:master

Conversation

@saihv
Copy link
Contributor

@saihv saihv commented Dec 25, 2017

This PR attempts to fix the issue with multi-vehicle control for multirotors (P0 issue on https://trello.com/b/1t2qCeaA/todo) by inserting each VehicleConnector pointer into an array, thereby converting fpv_vehicle_connector_ into a TArray. This lets us map individual API ports to each drone and control them independently, and with a few more extensions, allows for image capture from multiple drones.

As to the port numbers themselves, the code in SimModeWorldMultiRotor.cpp keeps track of the number of drones in the world and adds sequential IDs to the vehicle config name (i.e. SimpleFlight_0, SimpleFlight_1 etc.) and then MultiRotorParamsFactory.hpp parses settings.json as usual and assigns the corresponding port numbers. The settings.json list also needs to reflect the same names: i.e., VEHICLETYPE_ID. This change was added because the VehicleConfig property in Unreal blueprints doesn't exist anymore. Happy to hear any suggestions to make the change cleaner/more effective!

@sytelus
Copy link
Contributor

sytelus commented Dec 29, 2017

This looks good. I'll have to test it and then merge.

Copy link

@zergler zergler left a comment

Choose a reason for hiding this comment

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

You have an bug in SimModeWorldMultiRotor.cpp line 52/55.
fpv_vehicle_connector_ = nullptr;
TArray does not overload operator=.

@zergler
Copy link

zergler commented Jan 10, 2018

Also, the createConfig() function in MultiRotorParamsFactory.hpp needs to account for the new vehicle name in the if chain, otherwise you'll get a crash.

@saihv
Copy link
Contributor Author

saihv commented Jan 11, 2018

@zergler Thanks for the review! I will fix the fpv_vehicle_connector_ bug and push my changes. But in MultiRotorParamsFactory.hpp, isn't the if condition only looking for the firmware name (line 25)? As this is a child tag inside the vehicle name block, it should be unaffected by changes in the vehicle name, unless I am missing something.

@zergler
Copy link

zergler commented Jan 11, 2018

I'll have to look at it more in depth. I am trying to get multi-vehicle mode working and was using this pull request as a starting point. So thanks for contributing this!

I think it is only unaffected if that setting is present. I should have made that more clear in my comment. Basically inside of SimModeWorldMultiRotor.cpp on line line 133 the vehicle_config_name for the specific spawned vehicle is set to default_vehicle_config plus the added suffix. The default_vehicle_config is set to "SimpleFlight" if using "Multirotor" sim mode or "PhysXCar" if using "Car" sim mode as seen in SimModeBase.cpp on line 92. So you end up getting something like "SimpleFlight_1" for your vehicle_config_name. This is then passed into the createConfig function in MultiRotorParamsFactor.hpp which tries to use the vehicle_name to find a value for firmware_name inside settings.json file. If no such setting is found, firmware_name is set to vehicle_name and you reach line 36 and get a crash because of the added suffix.

I think this is the wrong way of approaching this. A previous version of AirSim had the VehicleConfigName property which could be set inside the UE4Editor. You should make things consistent with the doc otherwise the doc needs to be changed to account for your change. I personally like the approach taken by the doc to set up multiple vehicles because it gives you the ability to assign meaningful names to each actor in the environment. However, that would involve adding it to the blueprint if it's not already there (I couldn't find it). Sorry I can't be more help on that end, still learning UE4 programming myself.

@zergler
Copy link

zergler commented Jan 11, 2018

I see, I just fully read your original post and now understand. So that property no longer exists? Would anyone know how to bring it back?

As a workaround you can use the actor's name directly by replacing line 133 with the following.

std::string vehicle_name(TCHAR_TO_UTF8(*vehicle_pawn->GetName()));
vehicle_name = vehicle_name.substr(0, vehicle_name.find("_"));
wrapper->getConfig().vehicle_config_name = vehicle_name;

@saihv
Copy link
Contributor Author

saihv commented Jan 12, 2018

@zergler What does vehicle_pawn->GetName() return? Does it return 'BP_FlyingPawn' or something similar? Also, according to your description, I am assuming line 25 in MultiRotorParamsFactory.hpp is the one that sets firmware_name to vehicle_name if the firmware name tag doesn't exist?

I am not an expert in UE programming either :) which is why I created this workaround instead of fiddling around with the blueprints. I am not sure why the previous setting was removed, perhaps because it was clashing with the car-related code..

@saihv
Copy link
Contributor Author

saihv commented Jan 12, 2018

Another possible alternative we can consider to avoid problems in MultiRotorParamsFactory.hpp: if the tag for a specific vehicle does not exist in settings.json (An exception being thrown in this case was always an issue: for some reason, a backup configuration doesn't exist) we can take the vehicle name, truncate the vehicle ID and the underscore, and then use it as the firmware name.

@zergler
Copy link

zergler commented Jan 12, 2018

It returns 'BP_FlyingPawn_12' or some other number if you haven't changed the name. I'm not sure why the numbers are appended to the end of each vehicle name string, but they are unique for each actor. For my setup I've renamed all 'BP_FlyingPawn' actors to have real names, and then am able to reference them in the settings.json config file.

image

I would agree with that solution for if the setting doesn't exist. It should prevent frustrating crashes. It wouldn't work though if you are using the Actor's name as the vehicle_name.

@sytelus
Copy link
Contributor

sytelus commented Jan 19, 2018

Hi @saihv, I've merged these changes with bit of more enhancements. Specifically, there is no need to append "_id" to vehicle names (actually that produces error). Instead, if you want to instantiate multiple drones with different configs then just pass that config to VehiclePawnWrapper::initialize.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants