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 GrantConditionOnPrerequisite not refreshing when the actor's owner changes #12981

Merged
merged 2 commits into from Mar 19, 2017

Conversation

Projects
None yet
5 participants
@abcdefg30
Member

abcdefg30 commented Mar 17, 2017

Also moved GrantConditionOnPrerequisite into the Conditions folder, as the other GrantConditionOn* traits are in there as well.

@GraionDilach

👍

@abcdefg30

This comment has been minimized.

Show comment
Hide comment
@abcdefg30

abcdefg30 Mar 19, 2017

Member

Rebased and updated.

Member

abcdefg30 commented Mar 19, 2017

Rebased and updated.

@atlimit8

This does not fully handle updating the condition on ownership change.

@@ -62,6 +61,11 @@ void INotifyRemovedFromWorld.RemovedFromWorld(Actor self)
globalManager.Unregister(self, this, info.Prerequisites);
}
void INotifyOwnerChanged.OnOwnerChanged(Actor self, Player oldOwner, Player newOwner)
{
globalManager = newOwner.PlayerActor.Trait<GrantConditionOnPrerequisiteManager>();

This comment has been minimized.

@atlimit8

atlimit8 Mar 19, 2017

Member

You are missing condition revocation.

 			if (conditionToken != ConditionManager.InvalidConditionToken)
 				conditionToken = conditionManager.RevokeCondition(self, conditionToken);

			globalManager = newOwner.PlayerActor.Trait<GrantConditionOnPrerequisiteManager>();

Without that, this could be in the wrong state after ownership change.

@atlimit8

atlimit8 Mar 19, 2017

Member

You are missing condition revocation.

 			if (conditionToken != ConditionManager.InvalidConditionToken)
 				conditionToken = conditionManager.RevokeCondition(self, conditionToken);

			globalManager = newOwner.PlayerActor.Trait<GrantConditionOnPrerequisiteManager>();

Without that, this could be in the wrong state after ownership change.

This comment has been minimized.

@abcdefg30

abcdefg30 Mar 19, 2017

Member

This is done by INotifyRemovedFromWorld.RemovedFromWorld (capturing an actor removes it from the world, then changes the owner, and then adds it back to the world).

@abcdefg30

abcdefg30 Mar 19, 2017

Member

This is done by INotifyRemovedFromWorld.RemovedFromWorld (capturing an actor removes it from the world, then changes the owner, and then adds it back to the world).

This comment has been minimized.

@atlimit8

atlimit8 Mar 19, 2017

Member

Unlike GrantConditionOnPrerequisiteManager.Register, GrantConditionOnPrerequisiteManager.Unregister does not call PrerequisitesUpdated. I already accounted for INotifyRemovedFromWorld.RemovedFromWorld and INotifyAddedToWorld.AddedToWorld. It is GrantConditionOnPrerequisiteManager.Register that fixes it after all.

@atlimit8

atlimit8 Mar 19, 2017

Member

Unlike GrantConditionOnPrerequisiteManager.Register, GrantConditionOnPrerequisiteManager.Unregister does not call PrerequisitesUpdated. I already accounted for INotifyRemovedFromWorld.RemovedFromWorld and INotifyAddedToWorld.AddedToWorld. It is GrantConditionOnPrerequisiteManager.Register that fixes it after all.

This comment has been minimized.

@abcdefg30

abcdefg30 Mar 19, 2017

Member

Ah, alright then. I had only added loggin inside PrerequisitesUpdated and saw it being canceled there (somewhen). Thanks for having a look anyway. :)

@abcdefg30

abcdefg30 Mar 19, 2017

Member

Ah, alright then. I had only added loggin inside PrerequisitesUpdated and saw it being canceled there (somewhen). Thanks for having a look anyway. :)

GrantConditionOnPrerequisiteManager.Register handles it/

@atlimit8 atlimit8 merged commit ff998fd into OpenRA:bleed Mar 19, 2017

2 checks passed

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

@abcdefg30 abcdefg30 deleted the abcdefg30:grantOwner branch Mar 19, 2017

@atlimit8

This comment has been minimized.

Show comment
Hide comment
@atlimit8
Member

atlimit8 commented Mar 19, 2017

@reaperrr reaperrr referenced this pull request May 11, 2017

Closed

Ship a bugfix release #13274

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