Queue Refactor #95

Packetdancer · 2024-09-30T00:43:10Z

This is a fairly significant set of changes, reworking how the ability queue works and how the effect application works.

The core of this is the creation of a templated TGMASBoundOperationQueue where each TGMASBoundOperation represents a single action (add/remove, whatever) to a queue. The queue can be replicated client-to-server via GMC moves (as with the ability queue), server-to-client via RPC (as with the ServerAuth effect queue type), or used as a holding pen (as with the PredictedQueued effect queue type). In addition, each operation has a non-templated form which can be passed via RPC and then restored to a full operation in the queue.

Atop that change, the following additional changes were made.

The ability queue was converted to using a bound operation queue.
Effect application was converted to using bound operation queues.
New effect queue types were defined: Predicted, PredictedQueued, and ServerAuth are exposed to blueprint. ClientAuth and ServerAuthMove exist for the sake of completeness, but are hidden in the UENUM.
New API was added to determine how an effect operation (add/remove) is queued.
Old API was converted to use the new queues; the old API will use Predicted if bOutside is false, and ServerAuth if true.
The new API accepts "effect handles" -- these are effect identifiers which are only relevant/valid locally, but which can be used to refer to an effect immediately after it is queued for application, whether or not a valid effect ID was available yet.

For the sake of testing, there is a matching queue-refactor branch of the GMASExTemplate project which pulls this branch as the GMAS submodule, and where the BP_EffectBox has been converted to using the new API. In addition, BP_EffectBox has been set up so that the method by which the effect is queued can be changed easily in the blueprint (so that I didn't go insane while testing every queue type on every network scenario, repeatedly).

(However, despite the changes, previous code should Just Work seamlessly, though the older legacy API will have deprecation warnings.)

The bound operations queue functionality is meant to also support gameplay cues, as my next large task.

Note that this requires adding an SV_PreRemoteMoveExecution to your movement component, and calling into GMAS's PreRemoteMove call in that.

New queue types (ClientAuth, PredictedQueued) added.

Aherys · 2024-09-30T11:45:12Z

Source/GMCAbilitySystem/Private/Components/GMCAbilityComponent.cpp

Suggestion :

Backward incrementation removal instead of dual loop.

int reference as primitive type is less performant than copy (the more you know)

Aherys · 2024-09-30T11:59:16Z

First fast review, nothing to report, good job.

elegant use of template for the bound queue, i like it.

petegilb · 2024-10-02T19:23:27Z

This is super cool! I looked through it and there's nothing that I disagree with so next step would be some testing and we'll see where we're at!

rafadsm · 2024-10-03T16:14:38Z

I am experiencing a small rollback when using sprint in the demo project described (Playing as Client)
When using sprint, my character accelerates for a few milliseconds, then returns to normal speed and accelerates again
The same when sprint is turned off
The character slows down, accelerates and slows down again
I tried to bind to MaxDesiredSpeed, but that wasn't the problem
I don't know if it could be something with the ability system or the demo

Here is a video, it doesn't always happen, but at some point you notice the character doing a slight rollback

video: https://streamable.com/if7uk4

Although I don't understand much about the gmc, I believe you did a great job with this rework

petegilb · 2024-10-03T19:20:50Z

I am experiencing a small rollback when using sprint in the demo project described When using sprint, my character accelerates for a few milliseconds, then returns to normal speed and accelerates again The same when sprint is turned off The character slows down, accelerates and slows down again I tried to bind to MaxDesiredSpeed, but that wasn't the problem I don't know if it could be something with the ability system or the demo

Here is a video, it doesn't always happen, but at some point you notice the character doing a slight rollback

video: https://streamable.com/if7uk4

Although I don't understand much about the gmc, I believe you did a great job with this rework

I had the same issue. Was getting replays constantly when starting

Packetdancer · 2024-10-07T21:12:41Z

@rafadsm -- Do the most recent changes to the branch fix your desync/replay? I know they did for @petegilb, but I want to double-check.

rafadsm · 2024-10-08T00:05:31Z

@rafadsm -- Do the most recent changes to the branch fix your desync/replay? I know they did for @petegilb, but I want to double-check.

I did some tests and the desync/replay is fixed, thanks!

…the same frame.

reznok · 2024-10-09T03:42:20Z

Source/GMCAbilitySystem/Public/Utility/GMASBoundQueue.h

+	FInstancedStruct InstancedPayload {};
+
+	UPROPERTY()
+	float RPCGracePeriodSeconds { 1.f };


This default should be editable on the ASC somehow

Also it would be nice to be able to set this at the Effect level as well. Per @Aherys comment in Discord, some things absolutely need to be resolved sooner with little to no grace period (which will of course cause corrections).

Suggestion : AdvancedDisplay for this one,
Value below some reasonable amount must not be possible and we must warn the user when used/avoid using them (or why using this queue mode then ?) OR warn the user, and switch/force the queue mode if this is the case.

I would also suggest making the grace time a lot lower by default. If you use the client's move timestamp to determine if the grace time is over, you're effectively only waiting for a one-way trip (server to client RPC) and not RTT.

This means that having 1000 ms of grace time by default is a bit much. A grace period of 150 ms or 250 ms would account for most scenarios (technically double those numbers).

And high ping would result in rubberbanding anyway. If the server hitches causing a ping spike in players, they'll also feel it, so a lower grace period shouldn't affect players at all, and make it harder for cheaters to game the system.

Additionally, the header might be better holding the player's latest move timestamp (double) instead of the remaining period seconds. Seems a little more readable, you'd check against the current move timestamp you're handling from the player when checking if the grace period is over.

…erverAuth queued.

xCynDev

A few things I've spotted reading this PR. Some of these could be some pretty critical issues, others are just ideas.

xCynDev · 2024-10-22T03:34:56Z

Source/GMCAbilitySystem/Private/Components/GMCAbilityComponent.cpp

+				*GetNetRoleAsString(GetOwnerRole()), *GetOwner()->GetName())
+		}
+
+		UGMCAbilityEffect* Effect = DuplicateObject(Operation.ItemClass->GetDefaultObject<UGMCAbilityEffect>(), this);


Any reason why DuplicateObject is used over NewObject in this case? Since the duplicated object is the CDO, it's basically the same as just calling NewObject with the item class no?

No reason really, other than "it was done that way in other bits of the code which I didn't write." I was just trying to match the existing behavior for effects in case there was a side effect I wasn't aware of. I have zero objection to switching it back to NewObject.

Annoyingly it won't let me reply to the grace period comment, so I'm replying here.

I used 1 second because the existing "outside" behavior used that grace period; in case anyone was using that code, I didn't want to change the value on them by changing what was happening under the hood. (Same logic for doing the remaining time being deducted rather than a move timestamp.) Mostly, I just didn't want to change the implementation drastically and potentially change the behavior simultaneously when folks were testing it, as I wanted to have folks test the implementation sort of in isolation first; I figured behavior could be changed later.

I do think we should expose the grace period, and if no one is using the previous behavior I agree it would be good to have a shorter interval.

xCynDev · 2024-10-22T03:40:26Z

Source/GMCAbilitySystem/Public/Utility/GMASBoundQueue.h

+	FInstancedStruct InstancedPayload {};
+
+	UPROPERTY()
+	float RPCGracePeriodSeconds { 1.f };


I would also suggest making the grace time a lot lower by default. If you use the client's move timestamp to determine if the grace time is over, you're effectively only waiting for a one-way trip (server to client RPC) and not RTT.

This means that having 1000 ms of grace time by default is a bit much. A grace period of 150 ms or 250 ms would account for most scenarios (technically double those numbers).

And high ping would result in rubberbanding anyway. If the server hitches causing a ping spike in players, they'll also feel it, so a lower grace period shouldn't affect players at all, and make it harder for cheaters to game the system.

Additionally, the header might be better holding the player's latest move timestamp (double) instead of the remaining period seconds. Seems a little more readable, you'd check against the current move timestamp you're handling from the player when checking if the grace period is over.

xCynDev · 2024-10-22T03:42:39Z

Source/GMCAbilitySystem/Public/Utility/GMASBoundQueue.h

+
+	// The instanced struct representation of this payload, used to actually
+	// bind for replication.
+	FInstancedStruct InstancedPayload;


Keep in mind this is very unsafe as FInstancedStruct contains UObjects. The UScriptStruct of the wrapped struct, and the struct memory itself may contain UPROPERTY fields which the GC knows about through the AddStructReferencedObjects type trait.

I already talked to GRIM about implementing it for the GMC's bound variables since UObject* ones aren't referenced by the GC in some scenarios. You're meant to UPROPERTY instanced structs to prevent dangling pointers.

If there is no way to make it UPROPERTY, there are other solutions:

If this templated struct is held in a struct, you can implement the WithAddStructReferencedObjects type trait to manually add the instanced struct's references (and ItemClass as well) if they're not nullptr.

If this is held by a UObject, you can override UObject::AddReferencedObjects() and do the same.

I see this template is used by another template, so I would go up the chain until I find a UPROPERTY'd struct or UObject and handle it either way.

It's worth noting that the payloads being used may have UObject* properties, but those are filled in on the side actually utilizing it, so I was not particularly concerned about dangling pointers in this case as opposed to an abstract way to sync the templated payload.

However, it's a valid concern if someone blithely used the bound queue for something else. I'm not going to be able to do a rewrite for the next couple of weeks, however, so if you'd like to see that change sooner, feel free to refactor and make a PR against the source branch!

xCynDev · 2024-10-22T03:44:07Z

Source/GMCAbilitySystem/Public/Utility/GMASBoundQueue.h

+
+	FGameplayTag GetTag() const { return Header.Tag; }
+
+	bool GracePeriodExpired() const


Minor pet peeve, mostly Epic's coding convention which has bool getters be a question, like HasGracePeriodExpired().

Fair, and easy enough to change.

xCynDev · 2024-10-22T03:45:32Z

Source/GMCAbilitySystem/Public/Utility/GMASBoundQueue.h

+	// An actual class to be utilized with this, in case we need to instance it.
+	TSubclassOf<C> ItemClass { nullptr };
+
+	FInstancedStruct InstancedPayloadIds;


See comment on line 82.

InstancedPayloadIds only ever contains an FGMASBoundQueueOperationIdSet, a struct which only ever contains a single property of type TArray; it's only an instanced struct because GMC lacks a way to bind arrays.

I don't consider this one a particular risk, as a result.

That's fair. I'm mostly worried about the UScriptStruct but I suppose it shouldn't ever be GC'd for a C++ struct.

xCynDev · 2024-10-22T03:47:09Z

Source/GMCAbilitySystem/Public/Utility/GMASBoundQueue.h

+		{
+			// Get a handle to our class, for instancing purposes.
+			TSoftClassPtr<C> ClassPtr = TSoftClassPtr<C>(FSoftObjectPath(Header.ItemClassName.ToString()));
+			ItemClass = ClassPtr.LoadSynchronous();


This can cause hitches as this flushes the async streaming queue. I would use the asset manager's FStreamableManager::RequestSyncLoad() as it instead puts the request at the top of the queue, potentially finishing faster.

Reasonable change!

xCynDev

Bonus

xCynDev · 2024-10-22T06:18:57Z

Source/GMCAbilitySystem/Private/Components/GMCAbilityComponent.cpp

-			FGMCOuterApplicationWrapper Wrapper = FGMCOuterApplicationWrapper::Make<FGMCOuterEffectAdd>(Effect, InitializationData);
-			AddPendingEffectApplications(Wrapper);
+			QueuedEffectOperations.QueuePreparedOperation(Operation, false);
+			ClientQueueEffectOperation(Operation);


I do wonder if we could instead have another queue on the server, so we don't spam RPCs in case of multiple effects. Minor optimization perhaps but we could have an array of operations and send them as one RPC instead.

Crezetique · 2024-11-13T05:29:18Z

Bringing this over from discord to ensure vis and in case someone has the chance to look into it.

Replicable in fresh project. An issue occurs when nested gameplay tags are granted and removed with effects.
The effects are removed, but the granted tag is not removed correctly.
In the screenshot scenario, State.Test granted tag is never removed.
Having the effects added or removed in different order does not change the outcome.

Packetdancer and others added 13 commits September 29, 2024 17:31

Beginnings of general queue implementation

2b4053c

Further work on bound queues.

137bc26

Convert abilities over to using the new bound operations queue.

a5f9c92

Refactor to generalize ability queue processing

b38d417

Convert effects to bound queue

9a9828f

Refactor and cleanup

58bc8d1

A little more refactoring

5ff8418

Support server-auth effects queued via GMC moves.

935db5d

Note that this requires adding an SV_PreRemoteMoveExecution to your movement component, and calling into GMAS's PreRemoteMove call in that.

Ensure that Queue-via-GMC effects work in standalone as well.

ac58d82

Queue rework and cleanup finished!

bd5aa45

Last few tweaks to the queue setup.

cc707ef

Further queue refactor and cleanup.

a49cbd8

New queue types (ClientAuth, PredictedQueued) added.

Add effect handles, so that PredictedQueued works right.

732cbbe

Packetdancer added enhancement New feature or request maintenance Code cleanup, portability considerations, and such labels Sep 30, 2024

Packetdancer requested review from reznok and petegilb September 30, 2024 00:43

Aherys reviewed Sep 30, 2024

View reviewed changes

Packetdancer added 2 commits October 3, 2024 16:06

Small cleanup on effect handle expiry.

8369933

Correct desync for server-auth queue.

e6b13a6

Packetdancer added 2 commits October 8, 2024 16:36

Ensure we don't use the same ID if queuing two server-auth things in …

d298342

…the same frame.

Fix for Reznok's two-effects-in-one-frame issue.

f207ab0

reznok reviewed Oct 9, 2024

View reviewed changes

Correct issue with 2+ PredictedQueued operations in one tick, or 3+ S…

28e42e1

…erverAuth queued.

xCynDev reviewed Oct 22, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Queue Refactor #95

Queue Refactor #95

Packetdancer commented Sep 30, 2024 •

edited

Loading

Aherys Sep 30, 2024 •

edited

Loading

Aherys commented Sep 30, 2024

petegilb commented Oct 2, 2024

rafadsm commented Oct 3, 2024 •

edited

Loading

petegilb commented Oct 3, 2024

Packetdancer commented Oct 7, 2024

rafadsm commented Oct 8, 2024

reznok Oct 9, 2024

Aherys Oct 9, 2024

xCynDev Oct 22, 2024

xCynDev left a comment

xCynDev Oct 22, 2024

Packetdancer Oct 24, 2024

Packetdancer Oct 24, 2024 •

edited

Loading

xCynDev Oct 22, 2024

xCynDev Oct 22, 2024

Packetdancer Oct 24, 2024

xCynDev Oct 22, 2024

Packetdancer Oct 24, 2024

xCynDev Oct 22, 2024

Packetdancer Oct 24, 2024

xCynDev Oct 24, 2024

xCynDev Oct 22, 2024

Packetdancer Oct 24, 2024

xCynDev left a comment

xCynDev Oct 22, 2024

Crezetique commented Nov 13, 2024


		FGameplayTag GetTag() const { return Header.Tag; }

		bool GracePeriodExpired() const

Queue Refactor #95

Are you sure you want to change the base?

Queue Refactor #95

Conversation

Packetdancer commented Sep 30, 2024 • edited Loading

Aherys Sep 30, 2024 • edited Loading

Choose a reason for hiding this comment

Aherys commented Sep 30, 2024

petegilb commented Oct 2, 2024

rafadsm commented Oct 3, 2024 • edited Loading

petegilb commented Oct 3, 2024

Packetdancer commented Oct 7, 2024

rafadsm commented Oct 8, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

xCynDev left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Packetdancer Oct 24, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

xCynDev left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Crezetique commented Nov 13, 2024

Packetdancer commented Sep 30, 2024 •

edited

Loading

Aherys Sep 30, 2024 •

edited

Loading

rafadsm commented Oct 3, 2024 •

edited

Loading

Packetdancer Oct 24, 2024 •

edited

Loading