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

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

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

vorixo
Copy link
Contributor

@vorixo vorixo commented Sep 20, 2021

  • Now we are using the avatar as the Target for a better consistency. See: InvokeGameplayCueEvent
  • These static functions also call Add/RemoveLooseGameplayTag on the target's ASC, which might be convenient for gameplay

…static functions

- Now we are using the avatar as the Target for a better consistency. See: InvokeGameplayCueEvent
- These static functions also call `Add/RemoveLooseGameplayTag` on the target's ASC, which might be convenient for gameplay
@tranek
Copy link
Owner

tranek commented Dec 3, 2021

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
Copy link
Contributor Author

vorixo commented Dec 3, 2021

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
Copy link
Owner

tranek commented Dec 3, 2021

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
Copy link
Contributor Author

vorixo commented Dec 4, 2021

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
Copy link
Owner

tranek commented Dec 7, 2021

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
Copy link
Contributor Author

vorixo commented Dec 7, 2021

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

  • A GameplayCueLocal handle in the ASC that uses the avatar as the target actor like InvokeGameplayCueEvent, for consistency.
  • A couple of functions in the GAS BPFL to handle remote cueing in which we do pass-in the TargetActor as a parameter.

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

@SalahAdDin
Copy link

@vorixo Did you open the PR?

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.

3 participants