UAbilitySystemComponent::HandleGameplayEvent issues

Development Programming & Scripting
, ,
1

Issue 1: HandleGameplayEvent can activate the same ability more than once with a certain configuration.

If an Ability activates by GameplayEvents as:

A.B.C

A.B

Whenever the event A.B.C triggers the ActivateAbility will trigger twice, whereas there was only one event. This should be prevented by only activating the ability once per gameplay event.

Issue 2 :Another issue is that the handling of the events is not accounting for deletes while “broadcasting” because the current implementation is executing from a copy of the MulticastDelegate. This causes client code to receive callbacks when it already was removed from the multicast delegate.

Also the return type of the function could return the array of activated abilities so that client code can react accordingly to it (by registering to end ability, for instance)

Have you had any related issues with this? Could these changes be integrated in the engine in the future?

I have added this code in the header. Attached is the cpp alternative implementation

`public:
///
/** Removes delegates with bound object for a specific tag container. Returns removed count /
int32 RemoveAllGameplayEventTagContainerDelegates(const FGameplayTagContainer& TagFilter, const UObject BoundObject);
/** Removes delegates with bound object for any tag container. Returns removed count /
int32 RemoveAllGameplayEventTagContainerDelegates(const UObject BoundObject);

/** Adds a new delegate to call when a certain gameplay event happens. It will only be called if it exactly matches the tag provided */
FDelegateHandle AddGameplayEventDelegate(const FGameplayTag& Tag, const FGameplayEventDelegate& Delegate);
FDelegateHandle AddGameplayEventDelegate(const FGameplayTag& Tag, FGameplayEventDelegate&& Delegate);

/** Removes previously registered delegate /
bool RemoveGameplayEventDelegate(const FGameplayTag& Tag, FDelegateHandle DelegateHandle);
/* Removes delegates with bound object for a specific tag. Returns removed count /
int32 RemoveAllGameplayEventDelegates(const FGameplayTag& Tag, const UObject BoundObject);
/** Removes delegates with bound object for any tag. Returns removed count /
int32 RemoveAllGameplayEventDelegates(const UObject BoundObject);

protected:
// Pointers to copies in stack of currently broadcasting GenericGameplayEventCallbacks. To support deletes while broadcasting
TArray<FGameplayEventMulticastDelegate*> BroadcastingGenericGameplayEventCallbacks;
// Pointers to copies in stack of currently broadcasting GameplayEventTagContainerDelegates. To support deletes while broadcasting
TArray<FGameplayEventTagMulticastDelegate*> BroadcastingGameplayEventTagContainerDelegates;`

2

This doesn’t sound like a bug. Event activation is not “Exact Tag” matching, and that makes sense, cause i may want to send GameplayEvent.Projectile and not care if GameplayEvent.Projectile.Rocket is the one to respond. I may want multiple GA’s to actually activate. You can disable them from triggering if they are already active in ShouldAbilityRespondToEvent or using blocking tags to block the activation (or instanced per actor with retrigger unticked).

3

So if I have an ability configured with these trigger events:

GameplayEvent.Projectile

GameplayEvent.Projectile.Rocket

If I receive an event GameplayEvent.Projectile the ability will try to activate once, which is correct. But if I receive an event GameplayEvent.Projectile.Rocket the same event will be trying to activate the same ability twice because the code is trying to match with all parent tags in a loop.

GameplayEvent.Projectile.Rocket *match

GameplayEvent.Projectile *match

GameplayEvent *not match

Note that the event sent in the payload is always the same GameplayEvent.Projectile.Rocket.

What is the valid use case for that?

The fix is preventing to try to activate again the same ability with the same tag. To me it makes sense, as only one event arrived.

We really need the retrigger to be ticked because we acctually retrigger the ability, but we need to make sure that the event that happened is real, not redundant.

4

Issue 1: As Daniel mentioned, this is currently the intended behavior of the system. That said, I do see potential value in enabling either abilities or events to specify whether an “Exact Match” should be required. I’ll be discussing this with the team to explore options.

Issue 2: This one I’ll have to look into as I could see arguments for either side.

Regarding the suggestion to change the return type, I’m interested in understanding the motivation behind it. Would using something like ‘TryActivateAbility()’ instead of event activation not cover the intended use case? Activating abilities from events is generally expected to be fire-and-forget.

While we’re not encountering these issues directly it’s likely they are being worked around and that certainly doesn’t mean it can’t be improved.

5

Issue 1: It is not about requiring an exact match (which could also be convenient). What I am trying to say is that for one single event, whichever it is, one ability should never be activated more than once, under no circumstances. Or at least I can’t find any valid case for it. I think it is a bug.

Issue 2: Here is a code snippet that makes use of the return type to register to the “on ability ended” only for the abilities that were actually activated by this event

TArray<FGameplayAbilitySpecHandle> activatedHandles = asc->HandleGameplayEventEx(GEventTags.GameplayEventMontageByTagPlay, &payloadData); Algo::Transform(activatedHandles, ActivatedAbilities, [&](const FGameplayAbilitySpecHandle& newHandle) { FActivatedAbilityHandleWithASC result{ .AbilitySystemComp = asc, .SpecHandle = newHandle }; ensureAlways(result.GetInstance()); result.AbilityEndedHandle = result.GetInstance()->OnGameplayAbilityEnded.AddUObject(this, &ThisClass::OnMontageAbilityEnded); return result; });And related to the broadcasting of the delegates… Imagine that you have an ability that activates with a certain input event GameplayEvent.Input.Defense.Toggle

The Defense ability will listen to this same gameplay event to know when it should end itself. The issue with the current implementation is that the same event that activates the ability will be immediately broadcasted (after activating the Defense ability) and the ability will end right away, within the same call to HandleGameplayEvent, because the delegate that was added in ActivateAbility will be already in the list of delegates. The fix that I am proposing is trying to mimic the behavior of MulticastDelegate::Broadcast in order to be more consistent overall with multiple delegate execution.

6

There is an easy fix for that, which is to create a new function and call the new one from the old one.

int32 UAbilitySystemComponent::HandleGameplayEvent(FGameplayTag EventTag, const FGameplayEventData* Payload) { return HandleGameplayEventEx(EventTag, Payload).Num(); }Regarding the activation triggers. If an ability has configured in its data A.B and A.B.C it is true that it is a redundant configuration, because A.B will be enough to trigger the ability. But it happened to us multiple times that an ability has many (10+) activation events and some of them might become redundant by accident. This should not be reason enough to trigger the ability multiple times with the same event. The code should be robust to any data configuration without leading to undefined behavior.

Regarding the delegate processing I understand that it is a change in the API, but having the map

TMap<FGameplayTag, FGameplayEventMulticastDelegate> GenericGameplayEventCallbacks;

totally accessible as public is really bad. It should have never been public in the first place.

It is prone to errors and caused many bugs in the past to us. For instance you call GenericGameplayEventCallbacks.Remove(tag) //this is TMap::Remove

instead of

GenericGameplayEventCallbacks.FindOrAdd(tag).Remove(handle) // this is MulticastDelegate::Remove

and you are removing all delegates of a certain tag, which you should not be allowed to (because you don’t know if anyone else might be registered to that same tag.

Also it is inefficient as the map keeps growing and growing with tags containing empty delegates. There is no cleanup code that deletes empty entries.

I would at least consider deprecating the public access to GenericGameplayEventCallbacks in an intermediate version and then make it protected in a newer version.

7

I would stick to protected and also make the functions virtual so that a child class can extend without the need of patching the engine.

Regarding the cleanup what I mean is that you need to remove the entries of the map that contain gameplay tags with multicast delegates that are no longer bound, otherwise the map keeps growing forever and ends up with lots of tags with empty multicast delegates. The client code of course is always responsible of unbinding.

`bool UAbilitySystemComponent::RemoveGameplayEventDelegate(const FGameplayTag& Tag, FDelegateHandle DelegateHandle)
{
bool bRemoved = false;

if (FGameplayEventMulticastDelegate* MulticastDelegate = GenericGameplayEventCallbacks.Find(Tag))
{
bRemoved = MulticastDelegate->Remove(DelegateHandle);
// if nobody is interested in this tag anymore then remove it from the map
if (bRemoved && !MulticastDelegate->IsBound())
{
GenericGameplayEventCallbacks.Remove(Tag);
}
}

// If we are currently broadcasting then also remove from the copies in stack
for (FGameplayEventMulticastDelegate* BroadcastingMulti : BroadcastingGenericGameplayEventCallbacks)
{
BroadcastingMulti->Remove(DelegateHandle);
}

return bRemoved;
}`

We already have data validation in our abilities, simply we didn’t consider this config as an error. I thought the fix could be valid and useful for other projects.

I am more concerned about issue 2 because we are forced to patch the engine. For the HandleGameplayEvent we are already using our function HandleGameplayEventEx with the new signature and new behavior, but we cannot fix the delegate broadcasting without making GenericGameplayEventCallbacks protected and adding the new functions.

Thanks for taking the time to look at this.

8

No more questions. Thank you for your time.

9

After speaking with the team, unfortunately we can’t change the function signature currently due to it already having a return type that is in use in multiple projects, same for the logic behind the current delegate processing.

I am curious about your first point though, the activation triggers. While I can’t think of a use case where I would want an ability to trigger twice from a single event I also can’t think of an instance where I would want an ability to trigger from A.B.C and A.B both, would you be able to share why your ability needs to listen for both? It seems like the ability should activate on A.B and then internally it can do its own processing if A.B.C was the full trigger. I believe the intention with how it was designed was so that each ability and delegate decides how and if it should actually respond to the event without an extra layer of event rules in-between.

10

I do agree GenericGameplayEventCallbacks should be protected heck even private, cause the functions should be the only way to access such map.

Regarding clean up, that should be the job of whatever binds to it. This is common across C++ and most languages, that you should be responsible for cleaning up anything you have registered to. It would be highly inefficient for the ASC to go through all the map and check if a delegate is still valid. Epic traditionally had a bit of a history, expecially with GameplayAbilities with things being public that really shouldn’t. Actually common across most of the code base. They have done a good job so far moving stuff to protected/private where needed. I am really strict on exposing things raw in the public interface without good reason.

But yeah i just wanted to point out the cleanup issue is not really an issue the system should be handling. Sure it *could*, but why should it?. We have very strict rules in place to bind and unbind things properly instead of relying on the engine/other code to “possibly” do it.

One thing we do is run an audit over our gameplay abilities (and gameplay effects) to monitor and check for things that are not correct. You *COULD* also run validations if such an instance happens to catch these errors at your code level.

Simply overriding IsDataValid should help you alleviate such issues. We have such validations in our speciailized version of GameplayAbility (and our specialized version of GameplayEffect, purely for additional validation only).

11

I absolutely agree about making GenericGameplayEventCallbacks private and forcing the use of virtual functions for modification. I will make a task to tackle that first. The other changes can’t be done at the moment, but they are being taken into consideration as we make more modifications to GAS.

I really appreciate you taking the time to discuss and explain so thoroughly :smiley:

With that I will consider this resolved unless you have any other questions?

12

I will submit a PR with some more changes to GAS so keep an eye in the next week with for the PR. It fixes some stuff we saw our team do cause it was exposed in the public domain instead of being locked out.