tranek / GASDocumentation

My understanding of Unreal Engine 5's GameplayAbilitySystem plugin with a simple multiplayer sample project.
MIT License
4.27k stars 789 forks source link

[UPD] GameplayCueLocal handlers refreshed to use UGameplayCueManager static functions #66

Open vorixo opened 2 years ago

vorixo commented 2 years ago
tranek commented 2 years ago

Those _NonReplicated functions were added in UE5. GASDocumentation supports 4.27 at the moment. UAbilitySystemGlobals::Get().GetGameplayCueManager()->HandleGameplayCue() is the correct way to do it in 4.27.

I'm not entirely convinced on the AvatarActor yet, but that's not saying that it's wrong. I'm trying to think of a case where you could have an OwnerActor but not an AvatarActor where you would still want GCs to fire. I believe that there are guards in GAS code for the scenario of having an OwnerActor but no AvatarActor so Epic is expecting that to happen.

I'm imagining if you're in a spectator where the ASC is on the PlayerState but you don't set the AvatarActor as the SpectatorPawn (we might want to set that as the AvatarActor anyway). We'd still want to trigger GCs locally. This scenario breaks down if we're using an ASC proxy, but that's non-standard GAS. It really depends on how the game is architected. The OwnerActor seems to me to be a more general catch-all case. That said, I see the benefits of using the AvatarActor for some of the internal debug code there - printing out a character's name versus a PlayerState's name is more useful.

vorixo commented 2 years ago

Those _NonReplicated functions were added in UE5. GASDocumentation supports 4.27 at the moment. UAbilitySystemGlobals::Get().GetGameplayCueManager()->HandleGameplayCue() is the correct way to do it in 4.27.

I'm not entirely convinced on the AvatarActor yet, but that's not saying that it's wrong. I'm trying to think of a case where you could have an OwnerActor but not an AvatarActor where you would still want GCs to fire. I believe that there are guards in GAS code for the scenario of having an OwnerActor but no AvatarActor so Epic is expecting that to happen.

I'm imagining if you're in a spectator where the ASC is on the PlayerState but you don't set the AvatarActor as the SpectatorPawn (we might want to set that as the AvatarActor anyway). We'd still want to trigger GCs locally. This scenario breaks down if we're using an ASC proxy, but that's non-standard GAS. It really depends on how the game is architected. The OwnerActor seems to me to be a more general catch-all case. That said, I see the benefits of using the AvatarActor for some of the internal debug code there - printing out a character's name versus a PlayerState's name is more useful.

Hey Dan! I agree, UAbilitySystemGlobals::Get().GetGameplayCueManager()->HandleGameplayCue() should be the way to go in 4.27. Let's file this part of the PR for the UE5 version If you agree.

The AvatarActor part concept is introduced in InvokeGameplayCueEvent engine function. Take a look in there, I basically did the same for consistency with the rest of the codebase. I think the reasoning behind this is that avatars are the ones supposed to play animations and effects in any present use case. An ASC without avatar will not play cues regarding to these snippets. So this change goes towards standardization more than convenience.

tranek commented 2 years ago

Let's file this part of the PR for the UE5 version If you agree.

Yep, let's circle back to it in UE5. Thanks!

The AvatarActor part concept is introduced in InvokeGameplayCueEvent engine function. Take a look in there, I basically did the same for consistency with the rest of the codebase. I think the reasoning behind this is that avatars are the ones supposed to play animations and effects in any present use case. An ASC without avatar will not play cues regarding to these snippets. So this change goes towards standardization more than convenience.

I believe that UAbilitySystemGlobals::Get().GetGameplayCueManager()->HandleGameplayCue() will work just fine without an AvatarActor. The routing looks for a named function on the TargetActor and it falls back to the global GameplayCue set otherwise with the TargetActor passed through as the parameter into the GC.

I've done some things where I execute a local GC but don't need the local player's AvatarActor. For example, when a replicated Actor in the world is destroyed, I might hook into its EndPlay() to execute a local explosion GC by grabbing the local player's ASC and passing in the destroyed Actor through the GC parameters. While I've been able to get away with that out of sheer laziness, it's probably better to just put a GC interface on those Actors and execute it on them directly (no ASC needed).

Anyway, I think it comes down to engineer/designer choice if you want the MyTarget parameter in the GC to be the OwnerActor or AvatarActor and how you want to handle the scenario if the AvatarActor is nullptr. I opted for OwnerActor to handle AvatarActor's being invalid. I'm going to leave it that way for now in the code snippet, but I think parts of this conversation would make for good commentary next to it so that other people can make their own choice.

vorixo commented 2 years ago

I agree, with my latest commit I provide a more generic solution, which can be used in any actor that implements the receiving interface.

In addition, only those actors with an ASC can benefit from the loose tag system :)

Might also want to remove the local bSuppressGameplayCues, since this can be used for "remote cueing" on actors with no ASC. What do you think?

tranek commented 2 years ago

If they are going to pass in a TargetActor as a parameter, then they might as well be static functions in a BlueprintFunctionLibrary. Because at that point they are not coupled at all to the ASC where the functions live. I can't comment on those functions though since I'm not looking at UE5 for GASDocumentation until it's released.

Might also want to remove the local bSuppressGameplayCues, since this can be used for "remote cueing" on actors with no ASC. What do you think?

I'm not actually familiar with that bool in regards to remote cueing.

If those _NonReplicated functions are what you want to expose to BP, then just make a static wrapper function in BlueprintFunctionLibrary class and let them handle internally whether or not suppression should happen.

vorixo commented 2 years ago

If they are going to pass in a TargetActor as a parameter, then they might as well be static functions in a BlueprintFunctionLibrary. Because at that point they are not coupled at all to the ASC where the functions live.

Indeed, I agree. In fact this conversation we had about "remote cueing", covering the examples you exposed should be all dealt differently, as you comment, in a BPFL function (for example).

While at the same time, if you want to keep such functionality in the ASC without passing in a TargetActor, and still complying with the standardized approach, then we might fall back to the implementation of InvokeGameplayCueEvent, however in this case, using the non_replicated calls. By doing that, we won't have parts of the code in which the owner is passed over and parts of the code in which the avatar is passed over. That's why my first proposal used the AvatarActor.

If those _NonReplicated functions are what you want to expose to BP, then just make a static wrapper function in BlueprintFunctionLibrary class and let them handle internally whether or not suppression should happen.

Right, revising your comments I can draw a conclusion.

To sum up, my next proposal would consist of

I'll do a PR later on addressing this :)

SalahAdDin commented 1 year ago

@vorixo Did you open the PR?