fix(component): move AddImpulse + SetActorLocation declarations to public#9
fix(component): move AddImpulse + SetActorLocation declarations to public#9reznok wants to merge 5 commits into
Conversation
Both functions are UFUNCTION(BlueprintCallable) — Blueprint reflection ignores C++ access modifiers, but C++ callers were locked out by the private-block placement. Iliad's combat hit pipeline (UILCombatLibrary:: ProcessAbilityHit) needs to invoke AddImpulse from C++ for the knockback step; without this fix, C2248 access errors block the build. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
WalkthroughAdds ability chain-window sequencing (natural end grants a timed window tag consumed on the next stage's activation), block-all-other-abilities gating with an allowlist, a per-tag refcount rewrite for ChangesAbility Lifecycle: Chain Windows, Block-All, GrantedTag Refcount
Component API Extensions: FireCustomEvent, Niagara User Params, Camera Shake
Operation Processing Diagnostics and Server Batch-Ack Hoisting
Sequence Diagram(s)sequenceDiagram
rect rgba(173, 216, 230, 0.5)
Note over Caller,UGMC_AbilitySystemComponent: Chain Window Flow
end
participant Caller
participant UGMC_AbilitySystemComponent
participant UGMCAbility_StageA
participant UGMCAbility_StageB
participant ActiveEffects
Caller->>UGMC_AbilitySystemComponent: TryActivateAbility(StageA)
UGMC_AbilitySystemComponent->>UGMCAbility_StageA: BeginAbility() — remove ChainConsumeWindowTags effects
Note over UGMCAbility_StageA: StageA runs...
Caller->>UGMC_AbilitySystemComponent: EndAbilitiesByTag(StageA) [natural end]
UGMCAbility_StageA->>ActiveEffects: Apply ChainWindowTag effect (ChainWindowDuration, unique by tag)
Caller->>UGMC_AbilitySystemComponent: TryActivateAbilitiesByInputTag(InputTag) [first-passing-wins]
UGMC_AbilitySystemComponent->>UGMC_AbilitySystemComponent: IsAbilityTagBlocked(StageA) → blocked (window gate open)
UGMC_AbilitySystemComponent->>UGMC_AbilitySystemComponent: IsAbilityTagBlocked(StageB) → allowed
UGMC_AbilitySystemComponent->>UGMCAbility_StageB: TryActivateAbility(StageB)
UGMCAbility_StageB->>ActiveEffects: BeginAbility — RemoveEffect(ChainConsumeWindowTags)
ActiveEffects-->>UGMCAbility_StageB: ChainWindowTag effect removed
UGMC_AbilitySystemComponent-->>Caller: true (StageB activated, returns immediately)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
⚔️ Resolve merge conflicts
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment Warning |
New UGMCAbility properties: - bBlockAllOtherAbilities: while this ability is active, every activation is denied unless the candidate AbilityTag matches BlockAllAllowedTags (hierarchical MatchesAny). SetBlockAllOtherAbilities() toggles at runtime for phase control. - Enforced in IsAbilityTagBlocked, which PreBeginAbility checks on every activation path (normal, client-auth, server queue replay). New instances are not yet in ActiveAbilities during their own check, so a blocker always activates; re-activation of its own tag family is denied while it runs. - CDO-to-instance copy extended for both properties. - 6 new GMAS.Unit.Activation.BlockAllOtherAbilities specs, all green. NOTE: this commit also carries previously uncommitted working-tree changes to GMCAbilityComponent.cpp/.h from earlier sessions (custom event firing FireCustomEvent/OnCustomEvent, client-auth activation path, Niagara param helpers + GMASNiagaraParams.h) that could not be split out cleanly -- the Iliad module already compiles against them. Pre-existing red specs (GMAS.Unit.Attribute / ModifierMath / Ability.Cooldown) fail with or without this change; tracked separately. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
… input - ChainWindowTag/ChainWindowDuration on UGMCAbility: natural EndAbility applies an internally-built Persistent transient effect granting the window tag for the duration (bUniqueByEffectTag so re-grant refreshes). CancelAbility grants nothing -- interrupted swings do not advance chains. - ChainConsumeWindowTags: removed in BeginAbility after every gate passes; gate-denied presses consume nothing. - TryActivateAbilitiesByInputTag: first-passing-wins. One press activates exactly one ability; CheckActivationTags rejects fall through to the next candidate, which is how N chain stages share one InputTag. - GMAS.Unit.Chain spec (8 tests): grant/no-grant-on-cancel, progression, consume, expiry reset, stage-1 self-block, denied-press window survival, first-passing-wins. Uses public TickActiveEffects seam with manual ActionTimer (GenPredictionTick overwrites it from the move timestamp). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
- UGMASNiagaraParamLibrary::MakeNiagaraFloatParam/MakeNiagaraVectorParam -- one-node param construction (lighter than MakeStruct, reachable from scripted graph authoring). - UGMC_AbilitySystemComponent::SpawnParticleAtPoint: builds FFXSystemSpawnParameters internally (world position, auto-destroy) so the common location-spawn case needs no struct node; delegates to the existing multicast SpawnParticleSystemAtLocation. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…per-tag preserve) Bug (workflow-diagnosed + adversarially verified): ActiveTags is a set-like FGameplayTagContainer with no refcount, so two effects granting the same tag collapse to one entry. RemoveTagsFromOwner gated preserve on a same-EffectTag sibling count (GetActiveEffectsByTag), so when effect A ended while effect B (different/empty EffectTag) still granted shared tag T, T was stripped. Real symptom: a pawn stunned (GE_Stun grants State.Stunned + the two movement/ rotation locks) while LeapStomp self-root (GE_PreventMovementForced, also grants the two locks) is active regained movement when the ~0.9s self-root expired, because that removal yanked the shared State.Movement.Locked out from under the still-active stun. State.Stunned (only GE_Stun grants it) persisted, so VFX showed but the pawn could move. Fix: RemoveTagsFromOwner now preserves PER GrantedTag -- keep a tag if any OTHER live (not bCompleted) effect routed to the same container (matched by bClientAuth) still grants it, regardless of EffectTag/class. Generalizes the old same-EffectTag-only preserve and subsumes same-effect stacking. No new component state, no rollback surface (the rejected refcount-in-AddActiveTag approach would have broken the zombie-effect test and MatchTagToBool). Tests: GMAS.Unit.BugFix Shared GrantedTag refcount (two effects, different/empty EffectTag, both grant T; T survives the short effect expiry, drops only when the long one ends; single granter still clears). Updated the obsolete preserve=true test (no warning; removes only when no other granter). Full GMAS tag/effect/ activation/chain/clientauth suites green. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@Source/GMCAbilitySystem/Private/Effects/GMCAbilityEffect.cpp`:
- Around line 455-464: The preserve scan in the tag-preservation logic currently
considers any non-completed sibling effect as an active granter, but it does not
filter out effects that have not yet been applied or started. In the loop
iterating through ActiveEffectsSnapshot, add an additional condition to the
existing filter (which already checks Other, Other != this, !Other->bCompleted,
client auth match, and tag match) to verify that the sibling effect has actually
been applied before treating it as an active granter. This will prevent delayed
or unstarted effects from artificially keeping tags alive.
In `@Source/GMCAbilitySystem/Public/Utility/GMASNiagaraParams.h`:
- Around line 56-83: Complete the maker API in UGMASNiagaraParamLibrary by
adding three missing BlueprintPure UFUNCTIONs that follow the same pattern as
the existing MakeNiagaraFloatParam and MakeNiagaraVectorParam. Add
MakeNiagaraIntParam (taking an int32 value parameter), MakeNiagaraBoolParam
(taking a bool value parameter), and MakeNiagaraColorParam (taking an
FLinearColor value parameter). Each function should create an
FGMASNiagaraUserParam, set the Name and Type fields appropriately matching the
corresponding EGMASNiagaraUserParamType enum value, populate the correct value
field (IntValue, BoolValue, or ColorValue respectively), and return the
constructed parameter.
- Around line 18-28: The documentation comment for the struct/class containing
the Niagara parameter override incorrectly states that "the spawn helper
prefixes 'User.' for you" when in fact the implementation passes the Param Name
directly to Niagara's SetVariable* functions without prepending "User.". Update
the documentation to clarify that users must provide the complete parameter name
including the "User." prefix (e.g., "User.SizeScale" instead of just
"SizeScale") to match the actual behavior of the implementation in
GMCAbilityComponent.cpp where P.Name is passed directly to SetVariableFloat,
SetVariableInt, and similar Niagara API calls.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: b52f6860-24b8-47ea-8ff6-a006742dc87f
📒 Files selected for processing (9)
Source/GMCAbilitySystem/Private/Ability/GMCAbility.cppSource/GMCAbilitySystem/Private/Components/GMCAbilityComponent.cppSource/GMCAbilitySystem/Private/Effects/GMCAbilityEffect.cppSource/GMCAbilitySystem/Private/Tests/GMAS_ActivationSpec.cppSource/GMCAbilitySystem/Private/Tests/GMAS_BugFixSpec.cppSource/GMCAbilitySystem/Private/Tests/GMAS_ChainSpec.cppSource/GMCAbilitySystem/Public/Ability/GMCAbility.hSource/GMCAbilitySystem/Public/Components/GMCAbilityComponent.hSource/GMCAbilitySystem/Public/Utility/GMASNiagaraParams.h
| if (bPreserveOnMultipleInstances) | ||
| { | ||
| bool bAnotherGranterAlive = false; | ||
| for (const TPair<int, UGMCAbilityEffect*>& Pair : ActiveEffectsSnapshot) | ||
| { | ||
| if (Other && Other != this && !Other->bCompleted) | ||
| const UGMCAbilityEffect* Other = Pair.Value; | ||
| if (Other && Other != this && !Other->bCompleted | ||
| && Other->EffectData.bClientAuth == EffectData.bClientAuth | ||
| && Other->EffectData.GrantedTags.HasTagExact(Tag)) | ||
| { |
There was a problem hiding this comment.
Filter out not-yet-applied sibling effects in tag-preservation.
The preserve scan currently treats any non-completed sibling as an active granter. Delayed/unstarted effects can therefore keep a tag alive even though they have not applied that tag yet, leaving stale owner tags.
Suggested fix
- if (Other && Other != this && !Other->bCompleted
+ if (Other && Other != this && !Other->bCompleted
+ && Other->bHasAppliedEffect
&& Other->EffectData.bClientAuth == EffectData.bClientAuth
&& Other->EffectData.GrantedTags.HasTagExact(Tag))📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if (bPreserveOnMultipleInstances) | |
| { | |
| bool bAnotherGranterAlive = false; | |
| for (const TPair<int, UGMCAbilityEffect*>& Pair : ActiveEffectsSnapshot) | |
| { | |
| if (Other && Other != this && !Other->bCompleted) | |
| const UGMCAbilityEffect* Other = Pair.Value; | |
| if (Other && Other != this && !Other->bCompleted | |
| && Other->EffectData.bClientAuth == EffectData.bClientAuth | |
| && Other->EffectData.GrantedTags.HasTagExact(Tag)) | |
| { | |
| if (bPreserveOnMultipleInstances) | |
| { | |
| bool bAnotherGranterAlive = false; | |
| for (const TPair<int, UGMCAbilityEffect*>& Pair : ActiveEffectsSnapshot) | |
| { | |
| const UGMCAbilityEffect* Other = Pair.Value; | |
| if (Other && Other != this && !Other->bCompleted | |
| && Other->bHasAppliedEffect | |
| && Other->EffectData.bClientAuth == EffectData.bClientAuth | |
| && Other->EffectData.GrantedTags.HasTagExact(Tag)) |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@Source/GMCAbilitySystem/Private/Effects/GMCAbilityEffect.cpp` around lines
455 - 464, The preserve scan in the tag-preservation logic currently considers
any non-completed sibling effect as an active granter, but it does not filter
out effects that have not yet been applied or started. In the loop iterating
through ActiveEffectsSnapshot, add an additional condition to the existing
filter (which already checks Other, Other != this, !Other->bCompleted, client
auth match, and tag match) to verify that the sibling effect has actually been
applied before treating it as an active granter. This will prevent delayed or
unstarted effects from artificially keeping tags alive.
| /** | ||
| * One user-parameter override to apply to a Niagara System after spawn. | ||
| * Carried through GMAS multicast spawn helpers so all receivers (server, | ||
| * owning client, sim proxies) apply identical user params to the local | ||
| * NiagaraComponent they instantiate. Without this, only the caller's | ||
| * NiagaraComponent gets the param set; remotes see defaults. | ||
| * | ||
| * Author specs in BP as an array, drive size/color/etc per spawn. | ||
| * Param Name should be the Niagara user var leaf name (e.g. "SizeScale" | ||
| * — NOT "User.SizeScale"; the spawn helper prefixes "User." for you). | ||
| */ |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Search for how Niagara user parameters are set in the codebase to confirm prefix handling
rg -nP --type=cpp -C3 'SetVariable(Float|Vec3|Int|Bool|LinearColor)\s*\(' --glob '!**/ThirdParty/**'Repository: DeepWorldsSA/DeepWorlds_GMCAbilitySystem
Length of output: 1979
🏁 Script executed:
# Search for actual FGMASNiagaraUserParam usage to see what parameter names are provided
rg -nA5 'FGMASNiagaraUserParam\s*[=\{]' --type=cpp | head -100Repository: DeepWorldsSA/DeepWorlds_GMCAbilitySystem
Length of output: 66
🏁 Script executed:
# Search for blueprint or example code that shows expected parameter naming
rg -n 'SizeScale|User\.' --type=cpp --type=h | grep -i niagara | head -30Repository: DeepWorldsSA/DeepWorlds_GMCAbilitySystem
Length of output: 497
🌐 Web query:
Unreal Engine Niagara SetVariableFloat User parameter namespace prefix
💡 Result:
To set a Niagara User parameter from Blueprints or C++ using functions like SetNiagaraVariableFloat, you must include the User. prefix in the variable name [1][2][3]. When targeting a parameter created in the User namespace of a Niagara System, the variable name argument should be formatted as "User.YourParameterName" [1]. For example, if you have a User parameter named "SpawnRate", you would use "User.SpawnRate" in the Blueprint node or function call [1]. Key details regarding this namespace prefix: - Requirement: The User. prefix is mandatory for external systems (like Blueprints or C++) to correctly identify and override the parameter within the Niagara System's User namespace [1][4][3]. - Usage: This applies to functions such as SetNiagaraVariableFloat [1] or generic SetVariable functions [5]. - Scope: The User namespace acts as the public interface for the Niagara System, allowing external data to be passed into the simulation [1][2][3]. Other namespaces (such as System, Emitter, or Particle) are internal to the simulation and cannot be directly set from outside in the same manner [1]. - Consistency: Ensure the name string matches exactly, including the prefix and the case-sensitive name of the parameter [1].
Citations:
- 1: https://bugnet.io/blog/fix-unreal-niagara-userparam-not-updated-from-blueprint
- 2: https://github.com/ibbles/LearningUnrealEngine/blob/master/Niagara%20user%20exposed%20parameters.md
- 3: https://unrealingens.wordpress.com/2018/06/11/exploring-niagara-pre-release-part-3/
- 4: https://forums.unrealengine.com/t/how-to-change-dynamically-niagara-float-parameter/270979
- 5: https://dev.epicgames.com/documentation/en-us/unreal-engine/API/Plugins/Niagara/FNiagaraSystemInstanceController/SetVariable
Documentation claim about automatic "User." prefixing is incorrect.
The documentation states that "the spawn helper prefixes 'User.' for you" (line 27), but Unreal Engine's Niagara SetVariable* API functions require the "User." prefix to be included in the variable name parameter itself. The implementation at lines 3667–3679 in GMCAbilityComponent.cpp passes P.Name directly to SetVariableFloat, SetVariableInt, etc. without adding any prefix.
Users following this documentation will provide parameter names like "SizeScale", but the Niagara API requires "User.SizeScale". Either:
- The code must be updated to prepend "User." before calling SetVariable* functions, OR
- The documentation must be corrected to state that users must provide the full name including the "User." prefix (e.g.,
"User.SizeScale", not"SizeScale")
Correct the documentation or implementation to align with Niagara's API requirements.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@Source/GMCAbilitySystem/Public/Utility/GMASNiagaraParams.h` around lines 18 -
28, The documentation comment for the struct/class containing the Niagara
parameter override incorrectly states that "the spawn helper prefixes 'User.'
for you" when in fact the implementation passes the Param Name directly to
Niagara's SetVariable* functions without prepending "User.". Update the
documentation to clarify that users must provide the complete parameter name
including the "User." prefix (e.g., "User.SizeScale" instead of just
"SizeScale") to match the actual behavior of the implementation in
GMCAbilityComponent.cpp where P.Name is passed directly to SetVariableFloat,
SetVariableInt, and similar Niagara API calls.
| /** One-node makers for FGMASNiagaraUserParam — lighter than a MakeStruct | ||
| * node in graphs and reachable from scripted graph authoring. */ | ||
| UCLASS() | ||
| class GMCABILITYSYSTEM_API UGMASNiagaraParamLibrary : public UBlueprintFunctionLibrary | ||
| { | ||
| GENERATED_BODY() | ||
|
|
||
| public: | ||
| UFUNCTION(BlueprintPure, Category = "GMAS|FX") | ||
| static FGMASNiagaraUserParam MakeNiagaraFloatParam(FName Name, float Value) | ||
| { | ||
| FGMASNiagaraUserParam P; | ||
| P.Name = Name; | ||
| P.Type = EGMASNiagaraUserParamType::Float; | ||
| P.FloatValue = Value; | ||
| return P; | ||
| } | ||
|
|
||
| UFUNCTION(BlueprintPure, Category = "GMAS|FX") | ||
| static FGMASNiagaraUserParam MakeNiagaraVectorParam(FName Name, FVector Value) | ||
| { | ||
| FGMASNiagaraUserParam P; | ||
| P.Name = Name; | ||
| P.Type = EGMASNiagaraUserParamType::Vector; | ||
| P.VectorValue = Value; | ||
| return P; | ||
| } | ||
| }; |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick win
Complete the maker API for all enum types.
The enum EGMASNiagaraUserParamType defines five types (Float, Int, Bool, Vector, Color), but the library provides makers for only two (Float and Vector). Missing:
MakeNiagaraIntParamMakeNiagaraBoolParamMakeNiagaraColorParam
This asymmetry forces users to either construct FGMASNiagaraUserParam manually (error-prone due to Type/value field pairing) or avoid Int/Bool/Color parameters entirely.
📦 Proposed additions
UFUNCTION(BlueprintPure, Category = "GMAS|FX")
static FGMASNiagaraUserParam MakeNiagaraVectorParam(FName Name, FVector Value)
{
FGMASNiagaraUserParam P;
P.Name = Name;
P.Type = EGMASNiagaraUserParamType::Vector;
P.VectorValue = Value;
return P;
}
+
+ UFUNCTION(BlueprintPure, Category = "GMAS|FX")
+ static FGMASNiagaraUserParam MakeNiagaraIntParam(FName Name, int32 Value)
+ {
+ FGMASNiagaraUserParam P;
+ P.Name = Name;
+ P.Type = EGMASNiagaraUserParamType::Int;
+ P.IntValue = Value;
+ return P;
+ }
+
+ UFUNCTION(BlueprintPure, Category = "GMAS|FX")
+ static FGMASNiagaraUserParam MakeNiagaraBoolParam(FName Name, bool Value)
+ {
+ FGMASNiagaraUserParam P;
+ P.Name = Name;
+ P.Type = EGMASNiagaraUserParamType::Bool;
+ P.BoolValue = Value;
+ return P;
+ }
+
+ UFUNCTION(BlueprintPure, Category = "GMAS|FX")
+ static FGMASNiagaraUserParam MakeNiagaraColorParam(FName Name, FLinearColor Value)
+ {
+ FGMASNiagaraUserParam P;
+ P.Name = Name;
+ P.Type = EGMASNiagaraUserParamType::Color;
+ P.ColorValue = Value;
+ return P;
+ }
};🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@Source/GMCAbilitySystem/Public/Utility/GMASNiagaraParams.h` around lines 56 -
83, Complete the maker API in UGMASNiagaraParamLibrary by adding three missing
BlueprintPure UFUNCTIONs that follow the same pattern as the existing
MakeNiagaraFloatParam and MakeNiagaraVectorParam. Add MakeNiagaraIntParam
(taking an int32 value parameter), MakeNiagaraBoolParam (taking a bool value
parameter), and MakeNiagaraColorParam (taking an FLinearColor value parameter).
Each function should create an FGMASNiagaraUserParam, set the Name and Type
fields appropriately matching the corresponding EGMASNiagaraUserParamType enum
value, populate the correct value field (IntValue, BoolValue, or ColorValue
respectively), and return the constructed parameter.
Summary
Move two
UFUNCTION(BlueprintCallable)declarations onUGMC_AbilitySystemComponentfrom theprivate:section topublic::AddImpulse(FVector Impulse, bool bVelChange = false)SetActorLocation(FVector Location)Motivation
Both functions are
UFUNCTION(BlueprintCallable)— Blueprint reflection bypasses C++ access modifiers, so the BP-side surface works either way. But C++ callers geterror C2248: cannot access private member declared in class 'UGMC_AbilitySystemComponent'.The asymmetry is a code-organization bug: BlueprintCallable methods are by definition part of the component's public API. Anyone consuming GMAS from C++ (e.g., a custom combat library that wants to issue a server-side knockback impulse) currently has to either route through Blueprint, use
UFunctionreflection +ProcessEvent, or patch the plugin header locally.Concrete example consumer: an
ILCombatLibrary::ProcessAbilityHitfunction that takes a knockback impulse + hangtime and applies it viaTargetASC->AddImpulse(FinalImpulse, /*bVelChange=*/ true). Before this fix, the call failed to compile with C2248.Scope
ExecuteSyncedEvent(also in the same private block) is intentionally NOT moved — it's the internal dispatcher driven by the bound queue, not external-caller-facing API.Test plan
AddImpulse/SetActorLocationnodes work identically (no UFUNCTION metadata changed)Files changed
Source/GMCAbilitySystem/Public/Components/GMCAbilityComponent.h— declarations moved from private to public, preserving UFUNCTION + DisplayName + Category + default arg🤖 Generated with Claude Code
Summary by CodeRabbit
Release Notes
New Features
Bug Fixes