Skip to content
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 API for list of vehicle names #2936

Merged
merged 3 commits into from
Jun 1, 2021

Conversation

rajat2004
Copy link
Contributor

@rajat2004 rajat2004 commented Aug 14, 2020

Settings -

{
    "SettingsVersion": 1.2,
    "SimMode": "Car",

    "Vehicles": {
        "Car1": {
          "VehicleType": "PhysXCar"
        },
        "Car2": {
          "VehicleType": "PhysXCar",
          "X": 0, "Y": 10, "Z": 0
        }
    }
}

API -

>>> client.listVehicles()
['Car1', 'Car2']
>>> 

Came to mind in #2921, just wrote it since was straightforward to implement

The keys contain an empty string, coming from it being used to refer to the default vehicle, see ApiProvider.hpp being called from SimModeBase.cpp
A simple way is to just remove the empty string ourselves, which is being done right now.
However, the API could also mean - return a list of valid vehicle names which can be passed to APIs, in which case empty string is valid. But it could also introduce more complexity to the client code, such as removing the empty string if data of each vehicle is required only once, and having an empty string will duplicate calls for one vehicle.
I'm leaning towards removing the empty string, and will add that behaviour in the PR. But do comment if the second behaviour makes more sense.

Could be particularly useful if #2390 goes in (which I hope to update in the next few days)
Not much of an use-case otherwise, just allows the user to avoid parsing the settings for getting the vehicle names

@rajat2004
Copy link
Contributor Author

Rebased on master, conflict in every file (ouch), didn't go well with the Wind PR
But seems like both Travis and Azure Pipelines have stopped, would be good to have that looked at, don't want to go back to when master didn't compile on different platforms (which I myself caused a few times 😅 )
Ping @saihv @madratman

@rajat2004 rajat2004 force-pushed the vehicle-names-api branch 2 times, most recently from 0da3682 to bfcbfcf Compare December 23, 2020 00:34
@rajat2004 rajat2004 mentioned this pull request Feb 8, 2021
@rajat2004 rajat2004 force-pushed the vehicle-names-api branch 2 times, most recently from f7d9f51 to b9a6242 Compare March 12, 2021 18:11
@rajat2004 rajat2004 force-pushed the vehicle-names-api branch 2 times, most recently from 1e9d1a2 to 65a7867 Compare April 21, 2021 04:01
@zimmy87
Copy link
Contributor

zimmy87 commented May 27, 2021

Hey @rajat2004, can you resolve the current conflicts to get this PR ready for merging?

@zimmy87
Copy link
Contributor

zimmy87 commented Jun 1, 2021

Tested Unreal & Unity builds, API worked as expected in both builds, so I am moving ahead with merging this.

@zimmy87 zimmy87 merged commit 4bf6587 into microsoft:master Jun 1, 2021
@rajat2004 rajat2004 deleted the vehicle-names-api branch June 1, 2021 17:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants