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

Replace some hardcoded Aircraft assumptions with bool fields #13865

Merged
merged 6 commits into from Aug 31, 2017

Conversation

Projects
None yet
3 participants
@reaperrr
Contributor

reaperrr commented Aug 19, 2017

This lays some groundwork for easier activity merging (Fly & HeliFly etc.) and easier hybrid behavior implementation.

Additionally removed single-use FlyCircleTimed activity.

@reaperrr reaperrr added this to the Next + 1 milestone Aug 19, 2017

@pchote

This comment has been minimized.

Show comment
Hide comment
@pchote

pchote Aug 19, 2017

Member

I'd really prefer it if we can use multiple flags for each feature (CanHover, VerticalTakeoff, etc) instead of having an enum with fixed characteristics.

Member

pchote commented Aug 19, 2017

I'd really prefer it if we can use multiple flags for each feature (CanHover, VerticalTakeoff, etc) instead of having an enum with fixed characteristics.

@reaperrr reaperrr removed this from the Next + 1 milestone Aug 20, 2017

@reaperrr reaperrr changed the title from Replace Aircraft.IsPlane hack with AircraftType enum to Replace some hardcoded Aircraft assumptions with bool fields Aug 20, 2017

@reaperrr

This comment has been minimized.

Show comment
Hide comment
@reaperrr

reaperrr Aug 20, 2017

Contributor

Merged #13638 into this and updated.

Contributor

reaperrr commented Aug 20, 2017

Merged #13638 into this and updated.

@reaperrr

This comment has been minimized.

Show comment
Hide comment
@reaperrr

reaperrr Aug 20, 2017

Contributor

I used VTOL since nearly everybody knows what it means, and for those few who don't we have the description. I know we generally avoid abbreviations if possible, but that would make it quite long and a pain to write in this case.

Contributor

reaperrr commented Aug 20, 2017

I used VTOL since nearly everybody knows what it means, and for those few who don't we have the description. I know we generally avoid abbreviations if possible, but that would make it quite long and a pain to write in this case.

@reaperrr

This comment has been minimized.

Show comment
Hide comment
@reaperrr

reaperrr Aug 21, 2017

Contributor

Updated.

In case you wonder about it, I removed the commit that set CanHover: true on TS Banshee & Orca Bomber, it was bogus (I falsely assumed CanHover was triggering the Hovers effect).

Contributor

reaperrr commented Aug 21, 2017

Updated.

In case you wonder about it, I removed the commit that set CanHover: true on TS Banshee & Orca Bomber, it was bogus (I falsely assumed CanHover was triggering the Hovers effect).

@pchote

This comment has been minimized.

Show comment
Hide comment
@pchote

pchote Aug 26, 2017

Member

You can still add VTOL: true to those to give them proper takeoff/landing.

Member

pchote commented Aug 26, 2017

You can still add VTOL: true to those to give them proper takeoff/landing.

@pchote

LGTM otherwise 👍

reaperrr added some commits Jul 14, 2017

Add TakeOffOnCreation and TakeOffOnResupply to Aircraft
Before this, it was impossible to replicate the behavior of the original games (staying on pad/airfield after reload) without hacking around in Mods.Common.
This allows modders to disable these without meddling with code.
Remove FlyCircleTimed activity
A FlyCircle overload is sufficient.
Introduce Aircraft VTOL boolean
Rather than hard-linking vertical take-off/land to the CanHover = Helicopter assumption.
Remove IsPlane hack
The new VTOL boolean together with CanHover is enough to replace this.
@reaperrr

This comment has been minimized.

Show comment
Hide comment
@reaperrr

reaperrr Aug 27, 2017

Contributor

You can still add VTOL: true to those to give them proper takeoff/landing.

I could, but the combination of VTOL: true and CanHover: false doesn't work very well yet. On take-off, it will ignore VTOL: true and just do a plane-like take-off, and when ordered to land, it will ignore CanHover: false and do a helicopter-like "move straight towards target while visually turning".
Reason is that TakeOff just queues MoveTo which doesn't know when it's a take-off, and the only ways to fix that without regressions are either a) some very hacky temporary hacks, which I'd have to write first, or b) fixing it properly by merging the Fly and Land activities, for which I have a 95% finished local branch.
So I'd rather leave VTOL disabled on them for now and enable it with the next PR where it can serve as test-case for the merged activities.

Contributor

reaperrr commented Aug 27, 2017

You can still add VTOL: true to those to give them proper takeoff/landing.

I could, but the combination of VTOL: true and CanHover: false doesn't work very well yet. On take-off, it will ignore VTOL: true and just do a plane-like take-off, and when ordered to land, it will ignore CanHover: false and do a helicopter-like "move straight towards target while visually turning".
Reason is that TakeOff just queues MoveTo which doesn't know when it's a take-off, and the only ways to fix that without regressions are either a) some very hacky temporary hacks, which I'd have to write first, or b) fixing it properly by merging the Fly and Land activities, for which I have a 95% finished local branch.
So I'd rather leave VTOL disabled on them for now and enable it with the next PR where it can serve as test-case for the merged activities.

@reaperrr

This comment has been minimized.

Show comment
Hide comment
@reaperrr

reaperrr Aug 27, 2017

Contributor

Updated.

Contributor

reaperrr commented Aug 27, 2017

Updated.

@Mailaender

Code looks sane 👍 and I can't spot regressions in-game.

@Mailaender Mailaender merged commit 2f66f83 into OpenRA:bleed Aug 31, 2017

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@Mailaender

This comment has been minimized.

Show comment
Hide comment
@Mailaender
Member

Mailaender commented Aug 31, 2017

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