Bug - Instanced Abilities do not get OnRemoveAbility called on them during server travel

Development Programming & Scripting
, ,
1

Found a bug where OnRemoveAbility on clients are called on the CDO of an ability instead of the primary instance when performing a server travel. This is because we call DestroyActiveState on the ability system component when we are performing a seamless server travel, which happens before the client receives the replicated removal of the abilities. I think I have a fix for this, but wanted to run it by y’all before going forward with this change.

`// UAbilitySystemComponent::ClearAllAbilities(), line 423
#if ENGINE_CHANGES
if (!IsOwnerActorAuthoritative() && !bDestroyActiveStateInitiated)
#else
if (!IsOwnerActorAuthoritative())
#endif
{
ABILITY_LOG(Error, TEXT(“Attempted to call ClearAllAbilities() without authority.”));

return;
}

// UAbilitySystemComponent::ClearAllAbilities(), line 450
#if WITH_ENGINE_CHANGES
if (IsOwnerActorAutoritative())
{
ActivatableAbilities.Items.Empty(ActivatableAbilities.Items.Num());
ActivatableAbilities.MarkArrayDirty();
}
#else
ActivatableAbilities.Items.Empty(ActivatableAbilities.Items.Num());
ActivatableAbilities.MarkArrayDirty();
#endif

// UAbilitySystemComponent::DestroyActiveState(), line 1380
#if ENGINE_CHANGES
ClearAllAbilities();
#else
if (IsOwnerActorAuthoritative())
{
// We should now ClearAllAbilities because not all abilities CanBeCanceled().
// This will gracefully call EndAbility and clean-up all instances of the abilities.
ClearAllAbilities();
}
else
{
// If we’re a client, ClearAllAbilities won’t execute and we should clean up these instances manually.
// CancelAbilities() will only MarkPending kill InstancedPerExecution abilities.
// TODO: Is it correct to simply mark these as Garbage rather than EndAbility? I suspect not, but this
// is ingrained behavior (circa 2015). Perhaps better to allow ClearAllAbilities on client if bDestroyActiveStateInitiated (Nov 2023).
for (FGameplayAbilitySpec& Spec : ActivatableAbilities.Items)
{
TArray<UGameplayAbility*> AbilitiesToCancel = Spec.GetAbilityInstances();
for (UGameplayAbility* InstanceAbility : AbilitiesToCancel)
{
if (InstanceAbility)
{
InstanceAbility->MarkAsGarbage();
}
}

Spec.ReplicatedInstances.Empty();
Spec.NonReplicatedInstances.Empty();
}
}
#endif`

2

Steps to Reproduce
Cannot seem to reproduce this in Lyra.

3

Update - Fix initially suggested did not work as clients did not remove the old specs in their ActivatableAbilities array, causing OnRemoveAbility to get called initially for the clients’ instantiated ability, and then again for the ability CDO when the server replicated the removal of the ability.

Running this change instead for the past week has proven to be more successful:

`// UAbilitySystemComponent::DestroyActiveState(), line 1380
if (IsOwnerActorAuthoritative())
{
// We should now ClearAllAbilities because not all abilities CanBeCanceled().
// This will gracefully call EndAbility and clean-up all instances of the abilities.
ClearAllAbilities();
}
else
{
// If we’re a client, ClearAllAbilities won’t execute and we should clean up these instances manually.
// CancelAbilities() will only MarkPending kill InstancedPerExecution abilities.
// TODO: Is it correct to simply mark these as Garbage rather than EndAbility? I suspect not, but this
// is ingrained behavior (circa 2015). Perhaps better to allow ClearAllAbilities on client if bDestroyActiveStateInitiated (Nov 2023).
for (FGameplayAbilitySpec& Spec : ActivatableAbilities.Items)
{
#if WITH_ENGINE_CHANGES
if (Spec.Ability->GetInstancingPolicy() == EGameplayAbilityInstancingPolicy::InstancedPerExecution)
{
TArray<UGameplayAbility*> AbilitiesToCancel = Spec.GetAbilityInstances();
for (UGameplayAbility* InstanceAbility : AbilitiesToCancel)
{
if (InstanceAbility)
{
InstanceAbility->MarkAsGarbage();
}
}
}

Spec.ReplicatedInstances.Empty();
Spec.NonReplicatedInstances.Empty();
#else
TArray<UGameplayAbility*> AbilitiesToCancel = Spec.GetAbilityInstances();
for (UGameplayAbility* InstanceAbility : AbilitiesToCancel)
{
if (InstanceAbility)
{
InstanceAbility->MarkAsGarbage();
}
}

Spec.ReplicatedInstances.Empty();
Spec.NonReplicatedInstances.Empty();
#endif
}
}`

I’m not sure if this works for replicated instanced abilities quite yet, but for our non-replicated instanced abilities we seem to have been getting reliable OnAbilityRemoved calls

4

Thanks for pointing this out! Given the TODO this definitely warrants a deeper look, there’s been some changes in other areas of ability cancelation as well.

> This is because we call DestroyActiveState on the ability system component when we are performing a seamless server travel

Do you mean you’re calling DestroyActiveState() manually or that it is being called automatically as part of the travel process?

5

It is getting called automatically as part of the travel process. Not sure why it gets called before the abilities are removed in our project, but the abilities are removed before DestroyActiveState is called in Lyra, but might be related to seamless travel.

6

Awesome, I’ll do some investigation and get back to you with what I find. Thanks again!