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

Allow aircraft to scatter #15395

Merged
merged 1 commit into from Apr 22, 2019

Conversation

@BGluth
Copy link
Contributor

commented Jul 27, 2018

Made aircraft able to scatter

From what I can tell there isn't an easy way to determine if a cell is occupied like there is for ground units, so I thought reasonably the best thing to do is to pick a random nearby cell and move to it. However, this is not perfect and aircraft that are at the same altitude will collide and repulse off one another as they try to move past each other. If the group you want to scatter is large enough you will often get aircraft into a sort of deadlock. But this is maybe a separate issue altogether?

Let me know if anyone has any ideas for a better implementation.

Resolves #13137 and #14927.
Tested in TD, RA and TS.

@BGluth BGluth force-pushed the BGluth:14927_Aircraft_Scatter branch 2 times, most recently from a70aa83 to 12b2883 Jul 27, 2018

@BGluth BGluth force-pushed the BGluth:14927_Aircraft_Scatter branch from 12b2883 to 90ca11a Aug 21, 2018

@BGluth

This comment has been minimized.

Copy link
Contributor Author

commented Aug 21, 2018

@GraionDilach
Not sure how I let that through... I was playing around and I ended up getting rid of that check I guess. Anyways I re-added a check for that.

Side question: one thing that I noticed is that all of the MoveTo.../MoveInto... methods in Aircraft use HeliFly instead of HeliFlyAndLandWhenIdle even though Aircraft.ResolveOrder uses HeliFlyAndLandWhenIdle instead of HelyFly for move orders. Should the MoveTo.../MoveInto... methods also use HeliFlyAndLandWhenIdle instead of HeliFly?

@Mailaender
Copy link
Member

left a comment

This works nicely in-game.

@pchote
Copy link
Member

left a comment

I really like this, great idea!

I just have a couple of code changes to request, then we can merge this:

OpenRA.Mods.Common/Traits/Air/Aircraft.cs Outdated Show resolved Hide resolved
OpenRA.Mods.Common/Traits/Air/Aircraft.cs Outdated Show resolved Hide resolved

@BGluth BGluth force-pushed the BGluth:14927_Aircraft_Scatter branch from 90ca11a to b6f38d7 Oct 1, 2018

@BGluth

This comment has been minimized.

Copy link
Contributor Author

commented Oct 1, 2018

@pchote Applied the requested changes.

@BGluth BGluth force-pushed the BGluth:14927_Aircraft_Scatter branch from b6f38d7 to e4599cb Oct 2, 2018

@BGluth

This comment has been minimized.

Copy link
Contributor Author

commented Oct 2, 2018

@pchote Made the changes.

@pchote

This comment has been minimized.

Copy link
Member

commented Oct 2, 2018

The code looks good, but i'm sorry to say that I found a deal-breaker bug (not in this code!) that means we will need to hold off merging this until a fix is found: this provides a controlled way to push your aircraft outside of the map (wait until a yak or mig is flying towards the edge, then keep pressing the scatter key), and once outside the map they become immune to enemy fire (#13451).

The aircraft movement code needs to be changed to know about the map edge, and prevent them from going within their turning circle's radius of the edge.

@pchote pchote dismissed their stale review Oct 2, 2018

Comments addressed

@BGluth

This comment has been minimized.

Copy link
Contributor Author

commented Oct 8, 2018

@pchote

The aircraft movement code needs to be changed to know about the map edge, and prevent them from going within their turning circle's radius of the edge.

Is the plan to also still have repulsion forces for units that do make it outside the edge of the map?

@pchote

This comment has been minimized.

Copy link
Member

commented Nov 3, 2018

In hindsight I think it was a mistake to try and use the repulsion logic to push aircraft back inside the map. Instead we should keep a check in the repulsion code to make sure that it won't push units outside, but otherwise handle returning inside bounds using a proper activity that can be queued on idle or from the movement activities.

We can look into this once @reaperrr's current set of aircraft merging PRs are complete.

@pchote

This comment has been minimized.

Copy link
Member

commented Mar 24, 2019

@reaperrr how far do you think we are from the fly code being stable enough to start considering this?

@reaperrr

This comment has been minimized.

Copy link
Contributor

commented Mar 24, 2019

Hard to say, but if my open PRs related to this get reviewed often enough (and I can keep up updating them), maybe some 3 months, plus or minus 1 depending on how well things go?
So with some luck, in time for next playtest, I guess.

@pchote

This comment has been minimized.

Copy link
Member

commented Mar 24, 2019

Ok. I'd really like to include a fix for the out-of-bounds issues and this Scatter PR alongside the other aircraft fixes for Next if at all possible.

@reaperrr

This comment has been minimized.

Copy link
Contributor

commented Apr 22, 2019

Adding to milestone to make sure we don't forget this.

@reaperrr reaperrr added this to the Next Release milestone Apr 22, 2019

@pchote pchote force-pushed the BGluth:14927_Aircraft_Scatter branch from e4599cb to 04262aa Apr 22, 2019

@pchote

This comment has been minimized.

Copy link
Member

commented Apr 22, 2019

I've rebased this onto latest bleed and implemented a workaround to disable nudging for aircraft outside the map bounds. I think this should reduce the impact of the exploit to the point where we can live with it.

@pchote
pchote approved these changes Apr 22, 2019

@reaperrr reaperrr merged commit 55aa346 into OpenRA:bleed Apr 22, 2019

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
5 participants
You can’t perform that action at this time.