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

fix: Don't use a hook for ClientState#TerritoryType #2108

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

KazWolfe
Copy link
Member

  • We can just read this from the game data.

Added as its own method because unsafe getters are ugly af.

- We can just read this from the game data.
@KazWolfe KazWolfe requested a review from a team as a code owner November 19, 2024 00:09
@goaaats
Copy link
Member

goaaats commented Nov 24, 2024

Why specifically, is it just because this isn't set when injecting while the game is running? I usually don't like this type of change as it makes it harder to reason about what order things are done in. i.e. the field we are reading from might be set after the hook is fired, so now subscribers that read from the field in the event won't have the correct territory.

@Haselnussbomber
Copy link
Contributor

Haselnussbomber commented Nov 24, 2024

There will certainly be confusion as to why the TerritoryType id doesn't match the id that the TerritoryChanged event delivers:
The function that's being hooked for SetupTerritoryTypeDetour is Client::Game::Event::EventFramework.SetTerritoryTypeId, called just a bit after Client::Game::GameMain.StartTerritoryTransition. All this happens inside a function that's called in ProcessPacketInitZone.
GameMain sets CurrentTerritoryTypeId to 0 in there, and the id from the packet to TransitionTerritoryTypeId.
CurrentTerritoryTypeId is set to what TransitionTerritoryTypeId was when the transition is done - inside Client::Game::GameMain.Update.
So there will be some time between when the event is fired and when CurrentTerritoryTypeId is updated.

@goaaats
Copy link
Member

goaaats commented Nov 24, 2024

I think that is a deal-breaker until we decouple that and also have two IDs for current and pending territory, if that is even necessary. I doubt anyone has ever needed it and our API might not need to support it, just seems confusing to people.
We should set the ID and fire the event once we know that we are definitely going to a territory (and loading can't be cancelled anymore). I think that used to be the case.

@Haselnussbomber
Copy link
Contributor

Haselnussbomber commented Nov 25, 2024

I just ran into this by accident in my plugin, actually.
Instead of using ClientState.IsPvP, I was using GameMain.IsInPvPArea() (which reads from GameMain.CurrentTerritoryTypeRow, that is set right after GameMain.CurrentTerritoryTypeId), and it returning false a lot later than the event ClientState.TerritoryChanged was fired. Here the log that shows this:

03:39:00.085 | OnTerritoryChanged: 250
(EnterPvP here, I was not subscribed to it)
03:39:00.897 | IsInPvPArea: True
03:39:12.290 | OnTerritoryChanged: 339
03:39:12.291 | OnLeavePvP
03:39:14.493 | IsInPvPArea: False

@wolfcomp
Copy link
Contributor

wolfcomp commented Dec 1, 2024

IMO if this has a more correct return type than the hook this should be the defacto way to get the territory type.

@goaaats
Copy link
Member

goaaats commented Dec 1, 2024

More correct is not always == a good API, especially if it breaks existing assumptions.

@wolfcomp
Copy link
Contributor

wolfcomp commented Dec 1, 2024

This isn't about following API properties since we are explicitly having to deal with how SE codes the systems so if this follows better to how the system works in SE code this is the correct way and what is currently breaks the assumptions about the system.

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