Skip to content

Conversation

@rajat2004
Copy link
Contributor

@rajat2004 rajat2004 commented Sep 1, 2019

This PR restructures the Car vehicle code to make it similar to Multirotor in terms of adding support for external firmware control of the car vehicle
An example of how to add external firmware control can be seen here - a5dce95 (ArduPilot's Rover vehicle, the entire branch is master...rajat2004:pr-ardurover2)
(Didn't open PR for the entire thing since would be more difficult to review)

This PR has been tested to be working with manual control from the keyboard on Ubuntu 16.04 and Windows 10, with the below Python APIs on Ubuntu (Unreal Engine)

Things tested to be working -

  • Movement APIs - hello_car.py
  • getCarState - car_monitor.py
  • Pause - pause_continue_car.py
  • Multi-vehicle - multi_agent_car.py
  • ClockSpeed

Unity - Compilation only till now, real testing soon to follow

There are a few more things which can be cleaned up more, such as removing the CarState and CarControls from CarApiBase
Feedback on the approach by the other devs is very much needed!

@rajat2004
Copy link
Contributor Author

Review requested @madratman @sytelus

@rajat2004
Copy link
Contributor Author

@madratman @sytelus It would be great to have a review of this since any changes needed will need to be tested on both Linux and Windows along with the APIs so as to ensure nothing breaks
PR on the ArduPilot side is open (ArduPilot/ardupilot#12179). There is a blocking problem on the ArduPilot side but It would be good to have atleast this part go in

@sytelus
Copy link
Contributor

sytelus commented Sep 13, 2019

@madratman - this looks good to me. Please feel free to merge if you are happy as well!

@rajat2004
Copy link
Contributor Author

rajat2004 commented Sep 17, 2019

Rebased on master, tested on Windows and Ubuntu

@rajat2004
Copy link
Contributor Author

Will need to make changes to the Unity code also, so would be great to have a review!
I'll start on the Unity ones, mostly shouldn't be much different

@rajat2004
Copy link
Contributor Author

rajat2004 commented Sep 18, 2019

Preliminary Unity changes added, tested till compilation only on Windows and Ubuntu 16.04
Will soon test it on Unity as well

@rajat2004 rajat2004 force-pushed the pr-car-restructure2 branch 2 times, most recently from ba2c7c9 to aa11319 Compare September 21, 2019 06:16
@Jaeyoung-Lim
Copy link
Contributor

Pretty cool! Any plans on getting this merged?

@madratman
Copy link
Contributor

Sorry, been busy with other things. Reviewing and testing rn.

@bmalhigh
Copy link
Contributor

bmalhigh commented Oct 2, 2019

@rajat2004 -- Good work !! Couple of clarifying questions for my understanding --

  1. Is it correct that before this change, carcontrols coming via API were directly being applied to Unreal UWheeledVehicleMovementComponent without waiting for tick/update and now with your changes, the controls coming via API are also applied only during tick/update. correct? I don't see any issue with that but just wanted to understand the change clearly.
  2. Do we still need CarPawnApi class object or can that work be done by CarPawn class? The way you have done it now is definitely more evolving/less-disrupting... but trying to understand if CarPawnAPI still serves a very distinct role or not (or if this is going to more confusion w.r.t. number of objects/types)

@rajat2004 rajat2004 force-pushed the pr-car-restructure2 branch from aa11319 to 74518e3 Compare October 3, 2019 06:41
@rajat2004
Copy link
Contributor Author

rajat2004 commented Oct 3, 2019

Sorry for the delayed response. was travelling. And thanks for the review!

@ironclownfish I have added a separate commit - 74518e3 for the changes since I'm not very sure if I've done it in the correct way, once any additional changes needed are done, will squash it and make the same changes on the Unity side

Is it correct that before this change, carcontrols coming via API were directly being applied to Unreal UWheeledVehicleMovementComponent without waiting for tick/update and now with your changes, the controls coming via API are also applied only during tick/update. correct? I don't see any issue with that but just wanted to understand the change clearly.

@bmalhigh Not exactly, the behaviour is the same as in the previous code, the main thing which I've done is the removal of CarPawnApi as the child of CarApiBase and added a separate firmware API interface (PhysXCarApi) with which the physics would interface.
Earlier, the CarControls coming via API were being applied only on RenderTicks (rather than on Physics Ticks - explained by @sytelus in #2102 (comment))
It's the same with these changes other than that the controls are fetched from the firmware API interface and then applied to the MovememntComponent

Do we still need CarPawnApi class object or can that work be done by CarPawn class? The way you have done it now is definitely more evolving/less-disrupting... but trying to understand if CarPawnAPI still serves a very distinct role or not (or if this is going to more confusion w.r.t. number of objects/types)

This PR does reduce the work done by CarPawnApi class quite a lot, but I feel it's cleaner to keep it separate from CarPawn class, gives an easy-to-read interface
(Incidentally, I haven't even looked at the CarPawn class much, quite a lot of Unreal code in that)
Just my opinion, I'll leave it up to the maintainers to decide whether to keep it or not

Hope this explains it a bit clearly

@rajat2004
Copy link
Contributor Author

I've added some Unity changes also, will be squashed after everything is okay

@rajat2004
Copy link
Contributor Author

Been quite some time since I updated this, life got in between, with exams, sickness, system replacement, etc..
Will try to get this done as soon as possible, Unity wasn't working last I tried, would be great to have some pointers

@rajat2004
Copy link
Contributor Author

I think this is ready to be merged now, both Unreal and Unity are working, commits have been squashed

@rajat2004
Copy link
Contributor Author

rajat2004 commented Jan 2, 2020

Ping!
It would be great to have this merged, if possible, before the next release. There's a follow-up PR on this to add ArduRover support (to have that squeezed in as well would be awesome!)

@rajat2004
Copy link
Contributor Author

Opened #2383 which has the additional commits for ArduRover support

@rajat2004 rajat2004 requested a review from madratman January 17, 2020 16:26
Split CarApiBase implementation from CarPawnApi and add CarApiFactory to create appropriate vehicle API based on settings
@rajat2004 rajat2004 requested review from ironclownfish and sytelus and removed request for ironclownfish February 15, 2020 04:50
@rajat2004
Copy link
Contributor Author

@madratman I had opened #2383 also which includes these commits so as to make it easier for other people to use the feature and get review on it
That one has the same commits as this, would you prefer to have the changes made in this PR or the other one? This one can be closed if merging the other PR will be okay for you, in that case will make the updates there itself directly

@madratman
Copy link
Contributor

Sure let's move to 2383. I'm fine with just a rename to pawn api wrt review. The other inconsistencies are wrt airsim codebase in general.

@rajat2004
Copy link
Contributor Author

Ok great, then closing this PR and will update the other one

@rajat2004 rajat2004 closed this Mar 27, 2020
@rajat2004 rajat2004 deleted the pr-car-restructure2 branch March 29, 2020 12:37
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.

6 participants