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

Fix ProductionAirdrop aircraft not getting removed instantly #16825

Merged
merged 2 commits into from Aug 11, 2019

Conversation

@reaperrr
Copy link
Contributor

commented Jul 21, 2019

...when leaving map before finishing TakeOff.

Fixes #16824, as far as I can tell.

@reaperrr reaperrr added this to the Next Release milestone Jul 21, 2019

@reaperrr reaperrr force-pushed the reaperrr:fix-air-mapleave branch from 610f0f4 to 1b5302d Jul 21, 2019

@reaperrr reaperrr force-pushed the reaperrr:fix-air-mapleave branch from 1b5302d to a14ab00 Jul 21, 2019

@reaperrr

This comment has been minimized.

Copy link
Contributor Author

commented Jul 21, 2019

Updated. Tested both TD and D2k, both still seem to work as intended.

@fruestueck
Copy link
Contributor

left a comment

Works as intended. Can't comment the code.

@reaperrr reaperrr force-pushed the reaperrr:fix-air-mapleave branch from a14ab00 to b4fb300 Jul 25, 2019

@reaperrr

This comment has been minimized.

Copy link
Contributor Author

commented Jul 25, 2019

Updated again.

I intentionally made 2 ctors, one with target and one without, as the former will come in handy if we ever want aircraft to leave in a specific direction instead of just flying forward.

Fix ProductionAirdrop aircraft removal timing
...when leaving map before finishing TakeOff.

@reaperrr reaperrr force-pushed the reaperrr:fix-air-mapleave branch from b4fb300 to 2305498 Aug 3, 2019

@reaperrr

This comment has been minimized.

Copy link
Contributor Author

commented Aug 3, 2019

Updated.

@pchote

This comment has been minimized.

Copy link
Member

commented Aug 6, 2019

The introduced a small visual regression by removing the plane as soon as it hits the map edge - meaning the back half of the aircraft and all of its contrail blink out of existence. Can we please queue another few-cell timed fly between the FlyOffMap and RemoveSelf? The extra flight ticks could be controlled by a parameter on ProductionAirdrop to keep things simple.

@abcdefg30

This comment has been minimized.

Copy link
Member

commented Aug 7, 2019

Shouldn't FlyOffMap already do this itself? (Not stop at the map edge but go a bit further?)

@reaperrr

This comment has been minimized.

Copy link
Contributor Author

commented Aug 9, 2019

Shouldn't FlyOffMap already do this itself? (Not stop at the map edge but go a bit further?)

Well, technically it does move them off map (but only by 1 tick).
But I agree that FlyOffMap should handle this itself, else we'd have to insert that additional FlyTimed everywhere (airstrikes, crate planes and so on).

Updated.

@reaperrr reaperrr force-pushed the reaperrr:fix-air-mapleave branch from 9c8f881 to bd51456 Aug 10, 2019

@reaperrr

This comment has been minimized.

Copy link
Contributor Author

commented Aug 10, 2019

Updated.

@pchote pchote force-pushed the reaperrr:fix-air-mapleave branch from bd51456 to a4c5d17 Aug 10, 2019

@pchote

This comment has been minimized.

Copy link
Member

commented Aug 10, 2019

This wasn't quite right, so I tweaked and pushed a fix. This fixes aircraft still vanishing immediately if the aircraft was already at cruise altitude, and I think it also fixes your circling issue.

@pchote pchote force-pushed the reaperrr:fix-air-mapleave branch from a4c5d17 to 7da43c7 Aug 10, 2019

@pchote
Copy link
Member

left a comment

👍 with my tweaks

Don't exit FlyOffMap immediately on leaving map
Usually they'll get removed afterwards, so they need to be
out of players' sight before ending this activity.

@reaperrr reaperrr force-pushed the reaperrr:fix-air-mapleave branch from 7da43c7 to 7a9d18e Aug 11, 2019

@reaperrr

This comment has been minimized.

Copy link
Contributor Author

commented Aug 11, 2019

Updated.

@abcdefg30 abcdefg30 merged commit b7d966f into OpenRA:bleed Aug 11, 2019

2 checks passed

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

This comment has been minimized.

Copy link
Member

commented Aug 11, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
6 participants
You can’t perform that action at this time.