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(net/five): correct train track node on ownership change #2911

Merged
merged 1 commit into from
Nov 13, 2024

Conversation

Ehbw
Copy link
Contributor

@Ehbw Ehbw commented Nov 5, 2024

Goal of this PR

Fixes scenarios where a train becomes owned by the server (orphan natives) and upon a player re-entering the trains scope and becoming owner of the train, The train resets to the node 0 of the track it is currently on

How is this PR achieving the goal

By forcing the train to find its current track node based on the trains current position, only upon ownership change to a player from a server and if the new client has no knowledge of the trains current track node.

This PR applies to the following area(s)

FiveM

Successfully tested on

Game builds: 1604, 2372, 2545, 2802, 3258

Platforms: Windows

Checklist

  • Code compiles and has been tested successfully.
  • Code explains itself well and/or is documented.
  • My commit message explains what the changes do and what they are for.
  • No extra compilation warnings are added by these changes.

Fixes issues

@github-actions github-actions bot added the invalid Requires changes before it's considered valid and can be (re)triaged label Nov 5, 2024
@Legacy-TacticalGamingInteractive

Excellent

Copy link
Contributor

@Nobelium-cfx Nobelium-cfx left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for your contribution! It looks like a really valuable change. Left some minor comments.

But also, do you have a good way to reproduce/test it? I tried to play with it locally:

  • Put one player in a carriage of a train. Make sure that this player is the owner.
  • Get second player on the train as well.
  • See netObjectMgrBase__ChangeOwner being called, but the code never reaches the CTrain__SetTrainCoord(train, -1, -1); part. Seems like (CTrain__IsCarriageEngine(train) && *(int*)((char*)train + TrainTrackNodeIndexOffset) == 0) check is always false in my case.
  • Disconnect the first player.
  • After some time train disappears even tho second player in in a carriage.

Do I misunderstand the use case or this? Am I doing something wrong?

@Ehbw
Copy link
Contributor Author

Ehbw commented Nov 8, 2024

Thank you for your contribution! It looks like a really valuable change. Left some minor comments.

But also, do you have a good way to reproduce/test it? I tried to play with it locally:

  • Put one player in a carriage of a train. Make sure that this player is the owner.
  • Get second player on the train as well.
  • See netObjectMgrBase__ChangeOwner being called, but the code never reaches the CTrain__SetTrainCoord(train, -1, -1); part. Seems like (CTrain__IsCarriageEngine(train) && *(int*)((char*)train + TrainTrackNodeIndexOffset) == 0) check is always false in my case.
  • Disconnect the first player.
  • After some time train disappears even tho second player in in a carriage.

Do I misunderstand the use case or this? Am I doing something wrong?

This issue isn't related to ownership changing between players as in most (if not all cases) the client becoming owner should already have knowledge of the track node the train is located on. However, in cases where the server was the previous owner (in cases where no player is in the trains scope to have ownership over the train) the client becoming owner will have no knowledge of the trains current track node. Causing the train to reset to node 0 regardless of where it actually is.
The issue resolved with this PR can be replicated with 1 player.

  • Create a train doesn't matter the track location or train config. Applying the server-native SET_ENTITY_ORPHAN_MODE on the engine and all of it's carriages (this is simplified in Minor fixes for trains, use IsEntityValid for other calls checking if the entity exists #2902)
  • Once the train begins moving, move the player far enough away from the train to where the player leaves the trains scope (trains have a much larger scope then normal entities, the entity culling natives can be used here to make the scope smaller and make it easier to repro)
  • When re-entering the trains scope. Without this patch the train will begin calculating its next position from node 0 of the track it is on (usually 0 for metro lines, 3 for main train tracks) causing the train to teleport to node 0. With this patch, the train will continuing moving along based on the current track node it is located at.

I've also attached the repro resource i used to test this. (find a train track, do /train on the client to create a train. Leave the scope and return)
trainnode-repro.zip

@Ehbw Ehbw force-pushed the fix/train-track-node branch from ecc189a to 15365dd Compare November 8, 2024 14:07
@Ehbw Ehbw force-pushed the fix/train-track-node branch from 15365dd to 6facbc3 Compare November 8, 2024 15:05
Copy link
Contributor

@Nobelium-cfx Nobelium-cfx left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've tested it locally and can confirm that it works and makes the difference. Thank you for the great job!

PS: For some reason couldn't trigger train owner change with just one player - everything worked even without fix. But if I use one player to spawn the train, disconnect and use second to fly close to the train - then without the fix train disappears, while with the fix it stays where it should.

@Nobelium-cfx Nobelium-cfx added the ready-to-merge This PR is enqueued for merging label Nov 8, 2024
@Ehbw
Copy link
Contributor Author

Ehbw commented Nov 8, 2024

For some reason couldn't trigger train owner change with just one player - everything worked even without fix. But if I use one player to spawn the train, disconnect and use second to fly close to the train - then without the fix train disappears, while with the fix it stays where it should.

There was some instances when testing this patch where I had to decrease the trains Culling distance to 424 (onesync's default scope range). Since even going 4000+ units away from the train's position would still cause it to be relevant for the owning client. I haven't had time to investigate why this would happen though and it does seem to be inconsistent.

Though from testing and feedback from others using trains in a production environment. Trains behave very weirdly in OneSync / GTA:O especially with ownership reassignment (issues such as randomly detaching train carriages, negative speeds breaking train blending). Hoping to address some more of these issues in the future.

@Legacy-TacticalGamingInteractive

I'm so excited for this!

@prikolium-cfx prikolium-cfx merged commit 1268689 into citizenfx:master Nov 13, 2024
2 of 6 checks passed
@Legacy-TacticalGamingInteractive

@Ehbw for spawning trains will we have to do anything differently now to be able to utilize this commit or not really? Currently on client we run this snippet for NPC trains. We use another different system for drivable trains though.

Citizen.CreateThread(function()
		SwitchTrainTrack(0, true) -- Setting the Main train track(s) around LS and towards Sandy Shores active
		SwitchTrainTrack(3, true) -- Setting the Metro tracks active
		SetTrainTrackSpawnFrequency(0, 720000) -- The Train spawn frequency
		SetTrainTrackSpawnFrequency(3, 720000) -- The Metro spawn frequency
		SetRandomTrains(true) -- Telling the game we want to use randomly spawned trains
		SetTrainsForceDoorsOpen(false)
end)

@Ehbw
Copy link
Contributor Author

Ehbw commented Nov 13, 2024

@Ehbw for spawning trains will we have to do anything differently now to be able to utilize this commit or not really? Currently on client we run this snippet for NPC trains. We use another different system for drivable trains though.

Citizen.CreateThread(function()
		SwitchTrainTrack(0, true) -- Setting the Main train track(s) around LS and towards Sandy Shores active
		SwitchTrainTrack(3, true) -- Setting the Metro tracks active
		SetTrainTrackSpawnFrequency(0, 720000) -- The Train spawn frequency
		SetTrainTrackSpawnFrequency(3, 720000) -- The Metro spawn frequency
		SetRandomTrains(true) -- Telling the game we want to use randomly spawned trains
		SetTrainsForceDoorsOpen(false)
end)

If you are already using the server orphan natives. No.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
invalid Requires changes before it's considered valid and can be (re)triaged ready-to-merge This PR is enqueued for merging
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants