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

Switch TF2 extension to hook CTFGameRules::IsHolidayActive for holiday forward (bug 6137). #42

Merged
merged 3 commits into from
Jun 23, 2014

Conversation

psychonic
Copy link
Member

As noted on bug 6137, some of the calls to TF_IsHolidayActive that we detour on TF2 are now inlined on Windows, hampering functionality.

Instead, we can hook the virtual CTFGameRules::IsHolidayActive. There may be a case or two that bypass this function, going directly to the former one, but we are already missing those and more on Windows right now.

I tested a couple of things that can be done with the TF2_OnIsHolidayActive forward such as equipping holiday-restricted wearables, holiday taunts, and holiday-themed ammo boxes. These all work fine.

As a bonus, our automated bot will still alert us of changes, but also now giving the fixed offset if broken, rather than us manually having to dig through the binary to find the new byte signature on Windows.

@asherkin or @voided care to review code?

@FlaminSarge @powerlord comments regarding functionality?

@FlaminSarge
Copy link
Contributor

From what I can tell, this'll cause Strange counters that are affected by Holidays to use the actual holiday values rather than any the forward sets. In addition, the Halloween paint attribute may not be affected by the forward, though I think the visible color is actually determined clientside. The subset of item attributes that have __halloween as their attribute class suffix also won't be affected by the forward, which is less desirable. I'll look further into what occurs at the other places where TF_IsHolidayActive is used, but there may be few enough that we can attempt to come up with some sort of coverage for those cases.

@powerlord
Copy link
Contributor

On the surface, I have no objections.

However...

Do we know if the most recent major update introduced a new detour point? Valve changed something to make all _event maps run Halloween mode.

I haven't had a chance to look at it and thought it might be worth looking at before we move forward down this route.

@psychonic
Copy link
Member Author

From what I can tell, this'll cause Strange counters that are affected by Holidays to use the actual holiday values rather than any the forward sets.

Those intentionally bypass the gamerules function so that people can't just change the holiday cvar to level up their weapons. Respecting that same thought in our implementation (even if unintentionally) isn't necessarily bad.

In addition, the Halloween paint attribute may not be affected by the forward, though I think the visible color is actually determined clientside.

Clientside, so irrelevant(?)

The subset of item attributes that have __halloween as their attribute class suffix also won't be affected by the forward, which is less desirable.

That sounds less than ideal. Do you have an example of one of these?

but there may be few enough that we can attempt to come up with some sort of coverage for those cases.

Do we know if the most recent major update introduced a new detour point?

The two goals from my end are making this more maintainable and to re-unify functionality of it across platforms. I would love to avoid more detours if possible as they're become more and more unusable on Windows as Valve continues to update compiler versions and settings. Inlining nullifies them without notice, as you saw with the bug that prompted this, and if they were to enable LTCG in the future like on CS:GO and Dota 2, the calling conventions of the non-inlined ones have potential to change often as well.

@psychonic
Copy link
Member Author

Do any of you see any reason not to push this, keeping in mind that we can always add to it later if necessary? Is there even any functionality that this doesn't give that is still available on Windows right now?

@FlaminSarge
Copy link
Contributor

Since we don't have the functionality on Windows anyways, I guess there's no reason to wait. I'm still investigating the effects of the attribute change, but it affects 6 of 680something attributes, so the scope of the 'damage' is really limited in that regard.

I don't think the forward will properly obtain the value being used by the _event maps, but since they're _event maps, that case can easily be handled by any plugin authors that need it.

@psychonic
Copy link
Member Author

Okay, thanks. If you want to submit a PR to add more functionality, I'd be happy to review it.

psychonic added a commit that referenced this pull request Jun 23, 2014
Switch TF2 extension to hook CTFGameRules::IsHolidayActive for holiday forward (bug 6137, r=VoiDeD).
@psychonic psychonic merged commit dbcfbdb into master Jun 23, 2014
dvander added a commit that referenced this pull request Nov 4, 2015
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.

4 participants