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 attack orders to preempt move completion for turreted units. #16110

Merged
merged 2 commits into from Jan 27, 2019

Conversation

Projects
None yet
4 participants
@pchote
Copy link
Member

pchote commented Jan 25, 2019

Fixes #14824.

This PR (the last commit, after the changes from #16067) adds an OnQueueAttackActivity notification that is called when an attack order is added to the activity queue.

AttackFollow uses this notification to set the target if the order is not queued. This allows turrets to start tracking immediately, instead of waiting for an existing move order to complete and the attack order to tick for the first time.

This sits in the middle of the other targeting changes, so makes sense to include in the playtest. The improvement in perceived responsiveness should help soften the blow from turrets no longer tracking targets that are outside of range.

@pchote pchote added this to the Next Release milestone Jan 25, 2019

@pchote pchote force-pushed the pchote:target-turrets-immediately branch from 4e8f8e9 to f26b425 Jan 25, 2019

@matjaeck

This comment has been minimized.

Copy link
Contributor

matjaeck commented Jan 25, 2019

Move command -> Attack order -> Move command will make the unit's turret point at the target and moving the unit to the location of the last move command but it wont fire at the target in range.

GIF

bildschirmaufnahme2019012600

@pchote pchote force-pushed the pchote:target-turrets-immediately branch from f26b425 to f760020 Jan 26, 2019

@pchote

This comment has been minimized.

Copy link
Member Author

pchote commented Jan 26, 2019

Fixed.

@matjaeck

This comment has been minimized.

Copy link
Contributor

matjaeck commented Jan 26, 2019

Units sometimes won't attack structures when targeted under fog. This is easily reproducible for defence structures and happens for other structures, too, but I don't know how to reproduce exactly. Something is causing inconsistent behavior.

GIF not attacking defences

1

GIF some tanks attack, some do not

5

@matjaeck

This comment has been minimized.

Copy link
Contributor

matjaeck commented Jan 26, 2019

I can still reproduce this on #16112 for buildings: build a medium tank with client one and send it out of your base. Build a power plant of client 2 next to where the medium tank is. Move the medium tank away from the power plant so that it becomes hidden under fog. Issue an attack order and following move command on it /next to its position - the medium tank will pass the power plant without attacking.

@pchote

This comment has been minimized.

Copy link
Member Author

pchote commented Jan 26, 2019

GIF not attacking defences

Are you sure that you testing the latest version of the PR? I can't reproduce this.

GIF some tanks attack, some do not

This looks the same as the first issue you raised - works for tanks that are ordered to attack a visible structure, doesn't work for tanks that are ordered to attack a frozen one. This should already be fixed!

@teinarss

This comment has been minimized.

Copy link
Contributor

teinarss commented Jan 26, 2019

GIF not attacking defences

Are you sure that you testing the latest version of the PR? I can't reproduce this.

GIF some tanks attack, some do not

This looks the same as the first issue you raised - works for tanks that are ordered to attack a visible structure, doesn't work for tanks that are ordered to attack a frozen one. This should already be fixed!

I can reproduce this also.

@pchote

This comment has been minimized.

Copy link
Member Author

pchote commented Jan 26, 2019

I finally discovered the repro case here: it only happens if you build the pillbox after building the tank.

This happens because of an inconsistency in the way FrozenUnderFog handles visibility updates, which means you can catch actors in the middle of a tick where the game considers neither the FrozenActor nor the real actor to be visible. This must be a really ancient bug that was then made worse by #15866.

I'm not yet sure whether it is best to try and include a fix here, in #16067, or in its own PR.

@pchote

This comment has been minimized.

Copy link
Member Author

pchote commented Jan 26, 2019

I added the fix here, even though this isn't strictly related to this PR, because this provides the simplest repro case for testing.

@teinarss

This comment has been minimized.

Copy link
Contributor

teinarss commented Jan 26, 2019

Works now.

@matjaeck
Copy link
Contributor

matjaeck left a comment

Works for me too and looks ingame-wise good to me now.

@pchote pchote force-pushed the pchote:target-turrets-immediately branch from a1f6262 to 3393da3 Jan 26, 2019

@pchote

This comment has been minimized.

Copy link
Member Author

pchote commented Jan 26, 2019

Rebased now that the dependency has been merged. The code changes should be significantly more readable now.

@reaperrr
Copy link
Contributor

reaperrr left a comment

Code looks good to me, LGTM once that one comment is adressed

pchote added some commits Jan 27, 2019

Fix FrozenUnderFog / FrozenActor visibility consistency.
This fixes cases where both objects return visible / not
when queried at the wrong time during a tick.

@pchote pchote force-pushed the pchote:target-turrets-immediately branch from 3393da3 to 9b91c21 Jan 27, 2019

@pchote

This comment has been minimized.

Copy link
Member Author

pchote commented Jan 27, 2019

Fixed.

@reaperrr reaperrr merged commit d6d1f3a into OpenRA:bleed Jan 27, 2019

2 checks passed

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

@pchote pchote deleted the pchote:target-turrets-immediately branch Jan 27, 2019

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