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

Fixed a crash when trying to capture husks that have been destroyed. #5801

Merged
merged 4 commits into from
Jul 4, 2014

Conversation

Mailaender
Copy link
Member

as desribed in #5793 and #5638.

@Mailaender Mailaender changed the title Fixed a crash when in BuildingInfluence when trying to recover husks Fixed a crash in BuildingInfluence when trying to recover husks Jul 2, 2014
@@ -12,6 +12,7 @@

namespace OpenRA.Mods.RA.Buildings
{
[Desc("A dictionary of buildings placed on the map. Attach this to the World: actor.")]
Copy link
Member

Choose a reason for hiding this comment

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

World: is syntax for a yaml trait.
Maybe Attach this to the world actor would be better?

Copy link
Member Author

Choose a reason for hiding this comment

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

Okay.

@chrisforbes
Copy link
Member

Two problems here:

  1. IsDead() isn't the correct check since we certainly do want to remove "dead" buildings from BuildingInfluence if they take this path -- you want Destroyed, which is what matters for whether a trait lookup is going to explode on you.

  2. This is papering over a logic bug in husk capturing. Units shouldn't have their traits destroyed prior to being removed from the world.

@Mailaender
Copy link
Member Author

Updated. I hope this fixes the problem as a whole. Looks the removal from the world and immediate adding back during owner change was causing breakage everywhere when the actor has already been destroyed. Looks like BuildingInfluence was just the first one being notified and stumbling over it then.

@Mailaender Mailaender changed the title Fixed a crash in BuildingInfluence when trying to recover husks Fixed a crash in when trying to capture husks that have been destroyed. Jul 3, 2014
@Mailaender Mailaender changed the title Fixed a crash in when trying to capture husks that have been destroyed. Fixed a crash when trying to capture husks that have been destroyed. Jul 3, 2014
@pchote
Copy link
Member

pchote commented Jul 3, 2014

I think @chrisforbes's point 2) is the key thing here.

This crash will be occurring when someone captures a husk in the same tick that it is killed and removed from the map. The current checks will prevent the crash, but it will also cause the engineer/mechanic to be consumed with no tank to show for it.

A much better fix for this crash would be to add an IsDead check to CaptureActor.cs:L42.

@Mailaender
Copy link
Member Author

I added additional checks. Now every AddFrameEndTask should be safe.

pchote added a commit that referenced this pull request Jul 4, 2014
Fixed a crash when trying to capture husks that have been destroyed.
@pchote pchote merged commit 15d8fad into OpenRA:bleed Jul 4, 2014
@pchote
Copy link
Member

pchote commented Jul 4, 2014

Confirmed that this fixes the crash and that engineers/mechanics aren't consumed when this case is hit. Thanks.

@Mailaender Mailaender deleted the building-influence-crash branch July 5, 2014 06:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants