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 pillbox not uncloaking upon firing #21214

Merged
merged 2 commits into from Feb 7, 2024
Merged

Conversation

PunkPun
Copy link
Member

@PunkPun PunkPun commented Nov 15, 2023

Regression from #20675
Fixes #21093

@PunkPun PunkPun added this to the Next Release milestone Nov 15, 2023
@PunkPun PunkPun force-pushed the fix-camo branch 3 times, most recently from 0c15ddb to a39dcd7 Compare November 16, 2023 11:53
@JovialFeline
Copy link
Contributor

Fixes the tests I had, including the odd multi-passenger pillbox on wheels. 👌

@dnqbob
Copy link
Contributor

dnqbob commented Nov 23, 2023

Call other actor's INotifyAttack will result in messing up traits like GrantConditionOnAttack and crashing.

Exception of type `System.InvalidOperationException`: Attempting to revoke condition with invalid token 2 for hbox 292.
   at OpenRA.Actor.RevokeCondition(Int32 token) in D:\work\Projects\Github\OpenRA\OpenRA.Game\Actor.cs:line 583
   at OpenRA.Mods.Common.Traits.GrantConditionOnAttack.RevokeInstance(Actor self, Boolean revokeAll) in D:\work\Projects\Github\OpenRA\OpenRA.Mods.Common\Traits\Conditions\GrantConditionOnAttack.cs:line 77
   at OpenRA.TraitDictionary.TraitContainer`1.ApplyToAllTimed(Action`2 action, String text) in D:\work\Projects\Github\OpenRA\OpenRA.Game\TraitDictionary.cs:line 312
   at OpenRA.World.Tick() in D:\work\Projects\Github\OpenRA\OpenRA.Game\World.cs:line 458
   at OpenRA.Game.InnerLogicTick(OrderManager orderManager) in D:\work\Projects\Github\OpenRA\OpenRA.Game\Game.cs:line 646
   at OpenRA.Game.LogicTick() in D:\work\Projects\Github\OpenRA\OpenRA.Game\Game.cs:line 661
   at OpenRA.Game.Loop() in D:\work\Projects\Github\OpenRA\OpenRA.Game\Game.cs:line 830
   at OpenRA.Game.Run() in D:\work\Projects\Github\OpenRA\OpenRA.Game\Game.cs:line 883
   at OpenRA.Game.InitializeAndRun(String[] args) in D:\work\Projects\Github\OpenRA\OpenRA.Game\Game.cs:line 313
   at OpenRA.Launcher.Program.Main(String[] args) in D:\work\Projects\Github\OpenRA\OpenRA.Launcher\Program.cs:line 32

Testcase: replace the same file(yaml) in OpenRA-RA
structures.zip

@dnqbob
Copy link
Contributor

dnqbob commented Nov 23, 2023

I really don't recommend use this way to fix the uncloak issue because there can be countless dangerous edge cases on traits like GrantConditionOnAttack (for example you also need to test cases when IsCyclic is enabled and applying proper and hacky fix after that).

@PunkPun
Copy link
Member Author

PunkPun commented Dec 2, 2023

Fixed the crash

@PunkPun
Copy link
Member Author

PunkPun commented Dec 2, 2023

I think it makes perfect sense for infantry to call the pillbox's NotifyAttack. Not having that created weird exceptions and limitations to the AttackGarrison trait.

@dnqbob
Copy link
Contributor

dnqbob commented Dec 3, 2023

Then I recommend a detailed test on all INotifiedAttack related traits. They can have flaws on function but should never crash.

@PunkPun
Copy link
Member Author

PunkPun commented Jan 10, 2024

This fixes traits like ammo and reveal on fire not working in pillboxes

OpenRA.Mods.Common/Traits/Armament.cs Outdated Show resolved Hide resolved
OpenRA.Mods.Common/Traits/Armament.cs Show resolved Hide resolved
OpenRA.Mods.Common/Traits/Armament.cs Outdated Show resolved Hide resolved
Copy link
Member

@abcdefg30 abcdefg30 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code changes lgtm, but untested.

P.S. It feels like AttackGarrisoned shouldn't (re-)use Armaments from the passengers, but that is not for this PR.

@abcdefg30 abcdefg30 merged commit 1a4f366 into OpenRA:bleed Feb 7, 2024
3 checks passed
@abcdefg30
Copy link
Member

Changelog

@PunkPun PunkPun deleted the fix-camo branch February 7, 2024 15:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

AttackedGarrisoned does not trigger uncloak
5 participants