fix(component): move AddImpulse + SetActorLocation declarations to public - #9
Closed
reznok wants to merge 6 commits into
Closed
fix(component): move AddImpulse + SetActorLocation declarations to public#9reznok wants to merge 6 commits into
reznok wants to merge 6 commits into
Conversation
WalkthroughAdds block-all ability gating and chain-window lifecycle handling, rewrites granted-tag preservation across multiple effects, extends the ability component with custom events, Niagara user params, and camera shake APIs, and updates prediction diagnostics plus server batch-ack processing. ChangesAbility system updates
Ability system component APIs
Prediction diagnostics and server ack handling
Possibly related PRs
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 OpenGrep (1.23.0)Source/GMCAbilitySystem/Private/Effects/GMCAbilityEffect.cpp┌──────────────┐ �[32m✔�[39m �[1mOpengrep OSS�[0m [00.13][ERROR]: unable to find a config; path Source/GMCAbilitySystem/Private/Tests/GMAS_ChainSpec.cpp┌──────────────┐ �[32m✔�[39m �[1mOpengrep OSS�[0m [00.13][ERROR]: unable to find a config; path Source/GMCAbilitySystem/Private/Ability/GMCAbility.cpp┌──────────────┐ �[32m✔�[39m �[1mOpengrep OSS�[0m [00.25][ERROR]: unable to find a config; path
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
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
Comment on lines
+455
to
464
| 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.
Suggested change
| 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.
Comment on lines
+18
to
+28
| /** | ||
| * 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.
Comment on lines
+56
to
+83
| /** 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.
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>
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>
reznok
force-pushed
the
fix/synced-event-public-visibility
branch
from
June 29, 2026 00:26
b0fd188 to
24ded49
Compare
UE 5.8 relocated the StructUtils headers into CoreUObject under a StructUtils/ prefix; the old root-level "InstancedStruct.h" include path no longer resolves. Update the five headers that included it directly. GMASSyncedEvent.h already guards the path by engine version, so it is left untouched. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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/Public/Components/GMCAbilityComponent.h`:
- Around line 973-974: `FireCustomEvent` is exposed in Blueprint as if it were
usable by all callers, but it is server-only and already no-ops for
non-authority. Update the `UFUNCTION` declaration on `FireCustomEvent` in
`GMCAbilityComponent` to include `BlueprintAuthorityOnly` alongside
`BlueprintCallable` so Blueprint usage matches the authority check and avoids
client-side warnings.
- Around line 1297-1302: The SpawnParticleSystemAtLocation API in
GMCAbilityComponent is breaking existing C++ call sites by inserting UserParams
before the boolean flags; preserve the old parameterless signature by keeping a
forwarding overload that matches
SpawnParticleSystemAtLocation(FFXSystemSpawnParameters, bool, bool) and have it
delegate to the new implementation. Also add
meta=(AutoCreateRefTerm="UserParams") on the Blueprint-facing declaration so
UserParams remains optional for Blueprints while existing native code continues
to compile unchanged.
🪄 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: 167fdb79-5dc6-4fc4-85ca-d741ed9b4405
📒 Files selected for processing (13)
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/Ability/Tasks/GMCAbilityTaskBase.hSource/GMCAbilitySystem/Public/Ability/Tasks/WaitForInputKeyPress.hSource/GMCAbilitySystem/Public/Ability/Tasks/WaitForInputKeyPressParameterized.hSource/GMCAbilitySystem/Public/Components/GMCAbilityComponent.hSource/GMCAbilitySystem/Public/Utility/GMASBoundQueueV2_Operations.hSource/GMCAbilitySystem/Public/Utility/GMASNiagaraParams.h
✅ Files skipped from review due to trivial changes (4)
- Source/GMCAbilitySystem/Public/Ability/Tasks/WaitForInputKeyPressParameterized.h
- Source/GMCAbilitySystem/Public/Ability/Tasks/GMCAbilityTaskBase.h
- Source/GMCAbilitySystem/Public/Utility/GMASBoundQueueV2_Operations.h
- Source/GMCAbilitySystem/Public/Ability/Tasks/WaitForInputKeyPress.h
🚧 Files skipped from review as they are similar to previous changes (7)
- Source/GMCAbilitySystem/Public/Ability/GMCAbility.h
- Source/GMCAbilitySystem/Private/Effects/GMCAbilityEffect.cpp
- Source/GMCAbilitySystem/Private/Tests/GMAS_BugFixSpec.cpp
- Source/GMCAbilitySystem/Private/Tests/GMAS_ActivationSpec.cpp
- Source/GMCAbilitySystem/Private/Ability/GMCAbility.cpp
- Source/GMCAbilitySystem/Public/Utility/GMASNiagaraParams.h
- Source/GMCAbilitySystem/Private/Components/GMCAbilityComponent.cpp
Comment on lines
+973
to
+974
| UFUNCTION(BlueprintCallable, Category = "GMCAbilitySystem") | ||
| void FireCustomEvent(FGameplayTag EventTag, FInstancedStruct Payload); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify the header metadata matches the implementation authority guard.
rg -n -C3 'FireCustomEvent|BlueprintAuthorityOnly' \
Source/GMCAbilitySystem/Public/Components/GMCAbilityComponent.h \
Source/GMCAbilitySystem/Private/Components/GMCAbilityComponent.cppRepository: DeepWorldsSA/DeepWorlds_GMCAbilitySystem
Length of output: 3802
Mark FireCustomEvent as authority-only in Blueprint. The implementation already returns early for non-authority callers, so the current BlueprintCallable node exposes a client-side no-op that only warns at runtime. Add BlueprintAuthorityOnly to match the server-only contract.
🤖 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/Components/GMCAbilityComponent.h` around lines
973 - 974, `FireCustomEvent` is exposed in Blueprint as if it were usable by all
callers, but it is server-only and already no-ops for non-authority. Update the
`UFUNCTION` declaration on `FireCustomEvent` in `GMCAbilityComponent` to include
`BlueprintAuthorityOnly` alongside `BlueprintCallable` so Blueprint usage
matches the authority check and avoids client-side warnings.
Comment on lines
1297
to
+1302
| UFUNCTION(BlueprintCallable, Category="GMAS|FX") | ||
| UNiagaraComponent* SpawnParticleSystemAtLocation(FFXSystemSpawnParameters SpawnParams, bool bIsClientPredicted = false, bool bDelayByGMCSmoothing = false); | ||
| UNiagaraComponent* SpawnParticleSystemAtLocation( | ||
| FFXSystemSpawnParameters SpawnParams, | ||
| const TArray<FGMASNiagaraUserParam>& UserParams, | ||
| bool bIsClientPredicted = false, | ||
| bool bDelayByGMCSmoothing = false); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check call sites that may still use the previous signature.
rg -n -C3 '\bSpawnParticleSystemAtLocation\s*\(' Source/GMCAbilitySystemRepository: DeepWorldsSA/DeepWorlds_GMCAbilitySystem
Length of output: 3095
Preserve the unparameterized spawn API. Adding UserParams before the booleans breaks existing C++ calls like SpawnParticleSystemAtLocation(Params, true, false). Add meta=(AutoCreateRefTerm="UserParams") for Blueprint and keep a forwarding overload for the old signature.
🤖 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/Components/GMCAbilityComponent.h` around lines
1297 - 1302, The SpawnParticleSystemAtLocation API in GMCAbilityComponent is
breaking existing C++ call sites by inserting UserParams before the boolean
flags; preserve the old parameterless signature by keeping a forwarding overload
that matches SpawnParticleSystemAtLocation(FFXSystemSpawnParameters, bool, bool)
and have it delegate to the new implementation. Also add
meta=(AutoCreateRefTerm="UserParams") on the Blueprint-facing declaration so
UserParams remains optional for Blueprints while existing native code continues
to compile unchanged.
Author
|
This PR kind of ran away. Closing for now, I'll make areal one in the future. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
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