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

TF2Tools: Prevent CalcIsAttackCriticalHelper* from being called twice #1573

Merged
merged 2 commits into from
Sep 23, 2021

Conversation

nosoop
Copy link
Contributor

@nosoop nosoop commented Sep 3, 2021

CritManager::Hook_CalcIsAttackCriticalHelpers() is a pre-hook that calls the appropriate helper function, then passes the result over to plugins to determine whether the value should be changed.

The issue is that MRES_IGNORED is used to return after calling the helper function if the result is Pl_Continue, causing the helper function to be called a second time after the function, resulting in the server's crit calculations desyncing from the clients'. This PR stores the original return value and supercedes so it only ever gets called once.

Tested on a fresh server with the below plugin to listen for crits (and to force the hook to be enabled). Without the fix, a Soldier's rocket may have glow without the sound or vice-versa; with the fix applied, the rocket will have both or neither.

#pragma semicolon 1
#include <sourcemod>

#include <tf2>

#pragma newdecls required

public Action TF2_CalcIsAttackCritical(int client, int weapon, char[] weaponname, bool& result) {
	PrintToChat(client, "%s crit: %b", weaponname, result);
	return Plugin_Continue;
}

thx @FlaminSarge for giving me something to do

@peace-maker peace-maker merged commit 57a3863 into alliedmodders:master Sep 23, 2021
@nosoop nosoop deleted the crit-fix-desync branch September 23, 2021 10:06
@FlaminSarge
Copy link
Contributor

@peace-maker any chance at cherry-picking this for 1.10 as well?

@asherkin
Copy link
Member

asherkin commented Oct 3, 2021

@peace-maker any chance at cherry-picking this for 1.10 as well?

No, 1.10 isn't getting any significant changes - the focus is on stabilising 1.11.

@FlaminSarge
Copy link
Contributor

Is 1.11 not too stable right now? Ideally we shouldn't have to wait until 1.11 becomes stable to comfortably re-enable plugins that use this forward, but I've been out of the loop for a while so I don't know what the current issues are, if any, in 1.11. This also doesn't seem like a significant change for 1.10, more of a hotfix to bugged behavior that was introduced when this forward was reworked a while back.

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.

None yet

4 participants