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

Revise AttackMoveActivity #16105

Merged
merged 3 commits into from Mar 22, 2019

Conversation

@tovl
Copy link
Contributor

commented Jan 23, 2019

Since this seems to be the underlying cause of many glitches, I decided to tackle this issue.

Improvements:

  • The AttackMove behavior is now completely contained inside AttackMoveActivity.
  • Reliance on INotifyIdle.IsIdle is eliminated, thus avoiding a major source of glitchiness.
  • AttackMove now also respects any orders queued after it.

Fixes #16092
Fixes #14822
Fixes #11439
Fixes #9908
Fixes #7454
Fixes #15854
Fixes #13136
Fixes #16100

The only sacrifice that has to be made is that the inner activity can now no longer be an arbitrary activity. The reason is that the inner activity has to be freshly initialized inside AttackMoveActivity. You cannot restart an activity that is already cancelled, which is what the old implementation tried to do. Therefore, the caller to MoveTo is now hardcoded inside AttackMoveActivity. I am not very experienced with C# so perhaps there is a way to achieve this anyway that I do not now of.

Regardless, there is only one reference to AttackMoveActivity that needs anything other than MoveTo to initialize the inner activity. Therefore, I made a separate activity for Guard, which needs MoveFollow to initialize it's inner activity.

@pchote pchote added this to the Next + 1 milestone Jan 23, 2019

@pchote

This comment has been minimized.

Copy link
Member

commented Jan 23, 2019

The basic approach here looks good, but I don't expect to look at this in detail until after the playtests are out of the way. This will make a good headline feature for Next + 1.

@tovl tovl changed the title revise AttackMoveActivity Revise AttackMoveActivity Jan 24, 2019

@obrakmann
Copy link
Contributor

left a comment

Haven't tried this ingame yet, but it looks very promising. Just a few comments to start with.

@tovl tovl force-pushed the tovl:fix-AttackMove branch 2 times, most recently from bf426ff to cbd9698 Jan 26, 2019

@tovl tovl force-pushed the tovl:fix-AttackMove branch from cbd9698 to d6e5306 Jan 26, 2019

@abcdefg30

This comment has been minimized.

Copy link
Member

commented Jan 27, 2019

Needs a rebase now.

@tovl

This comment has been minimized.

Copy link
Contributor Author

commented Jan 27, 2019

rebased

@tovl tovl force-pushed the tovl:fix-AttackMove branch from 054d0ba to 6795b22 Jan 27, 2019

@pchote pchote referenced this pull request Jan 27, 2019
@matjaeck matjaeck referenced this pull request Jan 28, 2019

@tovl tovl force-pushed the tovl:fix-AttackMove branch from 6795b22 to 3594999 Feb 10, 2019

@tovl tovl force-pushed the tovl:fix-AttackMove branch 2 times, most recently from 40172a1 to e7b8101 Feb 26, 2019

@pchote

This comment has been minimized.

Copy link
Member

commented Mar 2, 2019

Do we want this to depend on #16246?

@tovl

This comment has been minimized.

Copy link
Contributor Author

commented Mar 2, 2019

I already had an alternative implementation based purely on ChildActivities ready to go but that didn't work due to the use of SequenceActivities in the various attack activities. So that would need to depend on #16217 and future PRs that fix the other attack variants as well. Since the current approach works and depending on how long it takes to get to fixing those other activties, I'd suggest keeping this as a standalone and making a new PR reimplementing it using ChildActivity at a later date.

@pchote

This comment has been minimized.

Copy link
Member

commented Mar 9, 2019

It looks like you accidentally commited a pair of .fuse_hidden files. Can you please unstage those?

Activate(self, order.OrderString == "AssaultMove");
var targetLocation = move.NearestMoveableCell(cell);
self.SetTargetLine(Target.FromCell(self.World, targetLocation), Color.Red);
assaultMoving = order.OrderString == "AssaultMove";

This comment has been minimized.

Copy link
@pchote

pchote Mar 9, 2019

Member

I suspect this will activate the AssaultMoveScanCondition prematurely if you queue an assault move after an existing attack move? It would be better to pass this into the activity, and then pull it out with ((AttackMoveActivity)activity).IsAssaultMove in Tick.

This comment has been minimized.

Copy link
@pchote

pchote Mar 9, 2019

Member

An option for the future, once we are sure that activities that have started are guaranteed to have a chance to clean themselves up, is to move the condition granting/revoking into AttackMoveActivity.OnFirstRun and AttackMoveActivity.OnLastRun.

I don't think we're quite there yet?

This comment has been minimized.

Copy link
@tovl

tovl Mar 9, 2019

Author Contributor

Okay. I've passed it into the activity.

I don't think we're quite there yet?

I'd rather make sure that all attack variations properly work with childactivities first before doing such a thing. I will look into this when converting AttackMove to use childactivities later.

@tovl

This comment has been minimized.

Copy link
Contributor Author

commented Mar 9, 2019

It looks like you accidentally commited a pair of .fuse_hidden files.

This happens a lot and is quite annoying. I think it would be a good idea to put this in the .gitignore file.

@tovl tovl force-pushed the tovl:fix-AttackMove branch from b6f0e03 to e6e80de Mar 9, 2019

@abcdefg30

This comment has been minimized.

Copy link
Member

commented Mar 16, 2019

Something is wrong with target lines: When you attack-move a group of units to a location,and they encounter enemies on their way, some units will have target lines pointing at the location while they are attacking and other units have target lines pointing at the location of dead enemies while attacking another enemy. After all enemies have been killed, some target lines point at the target location, other point at dead enemies.

This is already present on bleed iirc.

@matjaeck

This comment has been minimized.

Copy link
Contributor

commented Mar 16, 2019

For target lines:

bleed

bleed

fix-AttackMove

fix

@pchote

This comment has been minimized.

Copy link
Member

commented Mar 16, 2019

I plan to resurrect #14384 to fix the target line issues that are currently on bleed, but this can't happen while all this other work is still in progress.

@tovl

This comment has been minimized.

Copy link
Contributor Author

commented Mar 16, 2019

I was under the impression as well that #14384 would fix this and I just didn't worry that much about it.

@matjaeck

This comment has been minimized.

Copy link
Contributor

commented Mar 16, 2019

Yaks will return to base after reaching the location where they have been sent with a move command. They should circle at the location. It looks like they are trying to attack move but can't fire.

This does not happen when the yaks are in attack anything stance.

Another notice: The return to base behavior of yaks is different from bleed:

RTB GIF Let two yaks of one airfield force fire somewhere, then issue RTB with deploy key. The first yak will fly to the player spawn / conyard after it has rearmed while on bleed it stays at the airfield's rally point. Returning to the player spawn / conyard is something that used to happen when planes have been attack moving ( #16100) but now this behavior has extended to any RTB. When you look closely (it's more obvious ingame at higher framerate) you'll also notice that in the moment when the plane reaches the rally point after take off, it will get stuck in the air for a split-second and then fly over to the player spawn

rtb

@tovl tovl force-pushed the tovl:fix-AttackMove branch from ff49a86 to ab60e1c Mar 16, 2019

@tovl tovl force-pushed the tovl:fix-AttackMove branch from ab60e1c to 170eb93 Mar 16, 2019

@tovl

This comment has been minimized.

Copy link
Contributor Author

commented Mar 16, 2019

OK, I think I've solved the aircraft issues now. The problem apparently was that OnBecomingIdle was not accessible as part of the interface and I've mistakenly removed the interface when removing the hack. Earlier problems seem to have been related to the AttackMove hack hijacking the OnBecomingIdle method for its own uses. Hopefully this fixes #16100 as well.

@matjaeck

This comment has been minimized.

Copy link
Contributor

commented Mar 16, 2019

Nice, aircraft issues are fixed and this fixes #16100 too as far as I can tell. My only remaining comment is that activities like guard or attack move are canceled when infantry evades a crush. On bleed infantry won't cancle the attack move in this situation.

Code changed.

@matjaeck

This comment has been minimized.

Copy link
Contributor

commented Mar 16, 2019

Found another issue that seems to be new: when you attack move with yaks and they run out of ammo, they reject another attack move order when landed. Might be related to the part of code that should prevent them from continuing their attack move (#13474). See https://imgur.com/a/bdTMvyK.

@tovl

This comment has been minimized.

Copy link
Contributor Author

commented Mar 16, 2019

This turns out to be caused by the hacky fix to #15055, just like in #16241. So, I've followed @reaperrr 's lead and removed it here as well.

@tovl

This comment has been minimized.

Copy link
Contributor Author

commented Mar 16, 2019

Regarding infantry evading crushes: I believe this is the case for regular move as well and that this is also true on bleed (for regular move). The hacky attack-move implementation sort of fixed this accidentally, but a proper fix would require a complete rewrite of the nudging logic. I'd say that that is out-of-scope for this PR.

@obrakmann

This comment has been minimized.

Copy link
Contributor

commented Mar 17, 2019

I cannot reproduce that infinite loop anymore. Can anyone else, or has it been fixed in the meantime?

@matjaeck

This comment has been minimized.

Copy link
Contributor

commented Mar 17, 2019

The issue that yaks reject attack-move orders when landed is fixed and I agree that nudging is out-of-scope for this PR. There is one more issue I noticed but that one shouldn't block my 👍 for this, as everything else looks good to me.

Just for completeness: When you have 3 yaks on one airfield and let them attack-move at some tanks so that they run out of ammo before destroying them, the yaks will continue their attack-move after reloading. However, if you have 3 airfields for the 3 yaks, they won't continue their attack-move after reloading. It looks like that freeing the airfield for waiting planes triggers their attack-move to continue.

@matjaeck
Copy link
Contributor

left a comment

The last issue I mentioned is also present on bleed, so 👍 after playtesting in RA.

@tovl

This comment has been minimized.

Copy link
Contributor Author

commented Mar 17, 2019

I've not been able to reproduce the infinite loop either.

@pchote

This comment has been minimized.

Copy link
Member

commented Mar 21, 2019

Shall we get this merged then?

@obrakmann obrakmann merged commit 16f1750 into OpenRA:bleed Mar 22, 2019

2 checks passed

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

This comment has been minimized.

Copy link
Contributor

commented Mar 22, 2019

Yes, we shall. Congrats, @tovl, well done!

Changelog

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.