Friendly fire fixes and sam sites #7544

Merged
merged 1 commit into from Mar 8, 2015

Conversation

Projects
None yet
4 participants
@pevers
Contributor

pevers commented Feb 24, 2015

This PR will fix the following issues:

#7442

^ is solved by clearing the turret activities when not being able to attack (during low power).

#7403

^ is being solved by clearing the targets when the owner is changed.

var canAttack = false;
foreach (var t in turrets)
if (t.FaceTarget(self, target))
- canAttack = true;
+ return true;
return canAttack;

This comment has been minimized.

@RoosterDragon

RoosterDragon Feb 24, 2015

Member

Since you return true in the loop, you can return false here and do away with the local variable.

@RoosterDragon

RoosterDragon Feb 24, 2015

Member

Since you return true in the loop, you can return false here and do away with the local variable.

This comment has been minimized.

@pevers

pevers Feb 24, 2015

Contributor

Thanks!

@pevers

pevers Feb 24, 2015

Contributor

Thanks!

@RoosterDragon

This comment has been minimized.

Show comment
Hide comment
@RoosterDragon

RoosterDragon Feb 27, 2015

Member

Tested and fixes both issues. I'm not a buff on activities but these changes look good to me.

Could you squash the two optimization commits into one? 👍 from me once that is done.

Member

RoosterDragon commented Feb 27, 2015

Tested and fixes both issues. I'm not a buff on activities but these changes look good to me.

Could you squash the two optimization commits into one? 👍 from me once that is done.

@pevers

This comment has been minimized.

Show comment
Hide comment
@pevers

pevers Feb 28, 2015

Contributor

Thanks, squashed and rebased!

Contributor

pevers commented Feb 28, 2015

Thanks, squashed and rebased!

@obrakmann

This comment has been minimized.

Show comment
Hide comment
@obrakmann

obrakmann Mar 1, 2015

Contributor

I can confirm that this fixes #7403. I cannot reproduce #7442 at all, so can't confim the fix there. I did notice a huge regression with AttackMove and subsequent Move orders due to that fix, though.

  • With AttackMove, the unit's movement isn't fluid, they stop at least once per second and then resume along their path immediately, so they take ages to get anywhere.
  • After an AttackMove has finished, turreted units will ignore Move commands. They'll react to another AttackMove command, though. Not even giving the Stop order helps.

My initial reaction would be to just throw the fix for #7442 out for now. I'm not sure if it even needs fixing anymore.

Contributor

obrakmann commented Mar 1, 2015

I can confirm that this fixes #7403. I cannot reproduce #7442 at all, so can't confim the fix there. I did notice a huge regression with AttackMove and subsequent Move orders due to that fix, though.

  • With AttackMove, the unit's movement isn't fluid, they stop at least once per second and then resume along their path immediately, so they take ages to get anywhere.
  • After an AttackMove has finished, turreted units will ignore Move commands. They'll react to another AttackMove command, though. Not even giving the Stop order helps.

My initial reaction would be to just throw the fix for #7442 out for now. I'm not sure if it even needs fixing anymore.

@pevers

This comment has been minimized.

Show comment
Hide comment
@pevers

pevers Mar 1, 2015

Contributor

@obrakmann Thanks for the reponse, I didn't notice this. Tanks are impossible to control in this 'fix', i'll throw out this change.

Contributor

pevers commented Mar 1, 2015

@obrakmann Thanks for the reponse, I didn't notice this. Tanks are impossible to control in this 'fix', i'll throw out this change.

@RoosterDragon

This comment has been minimized.

Show comment
Hide comment
@RoosterDragon

RoosterDragon Mar 1, 2015

Member

Perhaps you can keep the fix for #7442 if you make the check more specific: i.e. cancel the activity if it cannot attack specifically due to low power, which hopefully prevents it cancelling other important activities (just thinking aloud here, not sure if that is feasible).

Member

RoosterDragon commented Mar 1, 2015

Perhaps you can keep the fix for #7442 if you make the check more specific: i.e. cancel the activity if it cannot attack specifically due to low power, which hopefully prevents it cancelling other important activities (just thinking aloud here, not sure if that is feasible).

@pevers

This comment has been minimized.

Show comment
Hide comment
@pevers

pevers Mar 1, 2015

Contributor

Thanks @RoosterDragon

I may have found out the real issue and why it isn't always reproducible in #7442 (see the last comment).

fc44345 felt a little bit hacky so I fixed it by giving the reverse animation the option to execute the next activity unqueued. This is now only the case when the user is selling something and not when an MVC is deploying/packing. Maybe I miss something here, but it seems to work now (at least for me).

Contributor

pevers commented Mar 1, 2015

Thanks @RoosterDragon

I may have found out the real issue and why it isn't always reproducible in #7442 (see the last comment).

fc44345 felt a little bit hacky so I fixed it by giving the reverse animation the option to execute the next activity unqueued. This is now only the case when the user is selling something and not when an MVC is deploying/packing. Maybe I miss something here, but it seems to work now (at least for me).

@pevers

This comment has been minimized.

Show comment
Hide comment
@pevers

pevers Mar 1, 2015

Contributor

Well, pretty impressive number of commits for such a small issue. But I think this will improve the following:

  • taking over a turret will clear the current target
  • disabled buildings will not queue new attack activities (improves efficiency)
  • this will result in fixing the sell activity on a powered down building
  • I also changed WithMakeAnimation.Reverse to be able to execute the activity after the animation on top of the queue.
Contributor

pevers commented Mar 1, 2015

Well, pretty impressive number of commits for such a small issue. But I think this will improve the following:

  • taking over a turret will clear the current target
  • disabled buildings will not queue new attack activities (improves efficiency)
  • this will result in fixing the sell activity on a powered down building
  • I also changed WithMakeAnimation.Reverse to be able to execute the activity after the animation on top of the queue.
OpenRA.Mods.Common/Traits/Sellable.cs
@@ -53,9 +53,9 @@ public void Sell(Actor self)
var makeAnimation = self.TraitOrDefault<WithMakeAnimation>();
if (makeAnimation != null)
- makeAnimation.Reverse(self, new Sell());
+ makeAnimation.Reverse(self, new Sell(), true);

This comment has been minimized.

@obrakmann

obrakmann Mar 2, 2015

Contributor

true is already the default value for this optional parameter, so you don't need to explicitly specify it. Or did you mean for this to be false?

@obrakmann

obrakmann Mar 2, 2015

Contributor

true is already the default value for this optional parameter, so you don't need to explicitly specify it. Or did you mean for this to be false?

OpenRA.Mods.Common/Traits/Sellable.cs
else
- self.QueueActivity(new Sell());
+ self.QueueActivity(true, new Sell());

This comment has been minimized.

@obrakmann

obrakmann Mar 2, 2015

Contributor

Same here as in the comment above, really: QueueActivity(true, new SomeActivity()) is the same as just QueueActivity(new SomeActivity()). It only behaves differently if the queued paramter is false.

So I don't quite get what you want to achieve with this change. The behaviour with this patch is exactly the same as without it. I mean, I get that it allows for a different behaviour, but you don't use it, so why add it?

@obrakmann

obrakmann Mar 2, 2015

Contributor

Same here as in the comment above, really: QueueActivity(true, new SomeActivity()) is the same as just QueueActivity(new SomeActivity()). It only behaves differently if the queued paramter is false.

So I don't quite get what you want to achieve with this change. The behaviour with this patch is exactly the same as without it. I mean, I get that it allows for a different behaviour, but you don't use it, so why add it?

@pevers

This comment has been minimized.

Show comment
Hide comment
@pevers

pevers Mar 2, 2015

Contributor

@obrakmann thanks for te review!

That's a mistake, that must be false (I was trying if it would still work, and it did). I added the parameter animation method to be able to execute the next activity right away just as with QueueActivity.

In this change I prevented an AttackBase activity to be scheduled if the actor is disabled. This will fix #7442 but I was still worried that maybe some other activity would jump in between the sell-animation and the actually sell. So I added the option to the QueueActivty and Render.

Contributor

pevers commented Mar 2, 2015

@obrakmann thanks for te review!

That's a mistake, that must be false (I was trying if it would still work, and it did). I added the parameter animation method to be able to execute the next activity right away just as with QueueActivity.

In this change I prevented an AttackBase activity to be scheduled if the actor is disabled. This will fix #7442 but I was still worried that maybe some other activity would jump in between the sell-animation and the actually sell. So I added the option to the QueueActivty and Render.

@obrakmann

This comment has been minimized.

Show comment
Hide comment
@obrakmann

obrakmann Mar 4, 2015

Contributor

Looks good now (although I still can't reproduce #7442). Please squash the commits, though.

Contributor

obrakmann commented Mar 4, 2015

Looks good now (although I still can't reproduce #7442). Please squash the commits, though.

fixed taking over sam/pillbox/etc. to stop firing
fixed issue with powered down samsites

more optimization

very small optimization

undo changes to powerdown samsite fixes

refixed powered down sam sites

removed debug line

redid the fix in another approach by queueing a sell activity on the top of the queue. This was already present but didn't always work

fixed line I removed

prevented attack activity to be queued when actor is disabled

another style fail of me

reverse and queueactivity activities are now executed on top of the queue for a sell action
@pevers

This comment has been minimized.

Show comment
Hide comment
@pevers

pevers Mar 4, 2015

Contributor

I have squashed the commits.

Contributor

pevers commented Mar 4, 2015

I have squashed the commits.

@pevers

This comment has been minimized.

Show comment
Hide comment
@pevers

pevers Mar 8, 2015

Contributor

Can the 'Changes Requested' label be removed?

Contributor

pevers commented Mar 8, 2015

Can the 'Changes Requested' label be removed?

@obrakmann

This comment has been minimized.

Show comment
Hide comment
@obrakmann

obrakmann Mar 8, 2015

Contributor

So, I've finally been able to reproduce that bug, and can now confirm that this is fixed with this change. 👍

Contributor

obrakmann commented Mar 8, 2015

So, I've finally been able to reproduce that bug, and can now confirm that this is fixed with this change. 👍

obrakmann added a commit that referenced this pull request Mar 8, 2015

@obrakmann obrakmann merged commit 190d197 into OpenRA:bleed Mar 8, 2015

2 checks passed

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

This comment has been minimized.

Show comment
Hide comment
@obrakmann

obrakmann Mar 8, 2015

Contributor

Thanks!

Changelog

Contributor

obrakmann commented Mar 8, 2015

Thanks!

Changelog

@jabbink jabbink deleted the delftswa2014:bugfix/sam_site_friendlyfire branch Apr 9, 2015

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