feat(extra-natives/rdr3): Add natives for train tracks

Sage-of-Mirrors commented 1 month ago

Goal of this PR

This PR provides three new natives for RedM for working with train tracks:

GET_TRACK_COUNT: Returns the number of tracks that are currently loaded in the world.
GET_TRACK_FROM_INDEX: Returns the name hash of the track at the given index, or 0 if that index is invalid.
LOAD_TRACKS_FROM_FILE: Disposes of the currently-loaded tracks and loads new ones from a given traintracks.xml file. This allows for fully customized rail networks.

It also provides patches for three crashes that can occur when using LOAD_TRACKS_FROM_FILE:

Track junctions that point to invalid tracks will fall through the post-processing step, meaning that trains trying to use those junctions will use invalid data and thus cause a crash. The patch removes junctions with invalid track references prior to post-processing.
Clients can no longer call CREATE_MISSION_TRAIN() when there are 0 tracks loaded; the function returns a null train handle.
The train tracks exist in a pool with slots for 50 of them; the function for loading track data from the XML file now bails out if the track pool is full.

This resource can be used to test the track natives. See client.lua for the list of commands.

How is this PR achieving the goal

The extra natives can be found in extra-natives-rdr3/src/TrackNatives.cpp. The patches can be found in gta-core-rdr3/src/PatchTrainTrackCrashes.cpp.

GET_TRACK_COUNT and GET_TRACK_FROM_INDEX are just directly accessing the track data.

LOAD_TRACKS_FROM_FILE is based on FiveM's LOAD_WATER_FROM_PATH.

The patches for junction crashes and preventing trains from spawning when there are 0 tracks loaded are simple trampolines.

The patch for not loading more tracks than the game has space for is an ASM patch inserted at the original exit condition check for the XML loading loop.

This PR applies to the following area(s)

RedM, Natives

Successfully tested on

Game builds: 1491.50

Platforms: Windows

Checklist

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

Fixes issues

Resolves #2424.

prikolium-cfx commented 2 weeks ago

Hi. Thanks for your contribution. Can you please provide traintracks.xml for testing and describe expected behavior, so we can test it too?

Sage-of-Mirrors commented 2 weeks ago

Hi! Thank you for the response.

This resource, which I linked in the main PR message, can be used to test the changes.

Testing Steps

In the F8 menu, run the command track_count. This should return the message "Track count: 29".
In the F8 menu, run the command track_at_index 0. This should return the message "Track name hash at index 0: 3534397256".
In the F8 menu, run the command track_at_index 51. This should return the message "Track name hash at index 51: 0".
In the F8 menu, run the command load_tracks. This should return the following message:
```
Track count before load: 29
Track count after load: 34
```
In the F8 menu, run the command track_count. This should return the message "Track count: 34".
In the F8 menu, run the command spawn_trains. The game should not crash.
Exit the game, and navigate to traintracks.xml, which is located at <resources>/tracktest/data/. Open traintracks.xml in a text editor, and remove the contents of the <train_tracks> XML element. Save the file, and re-enter the game. In the F8 menu, run the command load_tracks. This should return the following message:
```
Track count before load: 29
Track count after load: 0
```
In the F8 menu, run the command spawn_trains. This should return the message "CreateMissionTrain() failed - track pool is empty!".
Repeat Step 7, but instead of removing the contents of the <train_tracks> element, copy one of the <train_track> elements within it and paste it multiple times - enough to push the number of <train_track> elements within the <train_tracks> node over 50. Save the file, re-enter the game, and in the F8 menu, run the command load_tracks. This should return the following message:
```
Track count before load: 29
Track pool is full - no more tracks can be loaded.
Track count after load: 50
```

Please let me know if I can provide additional information. Thank you!

Sage-of-Mirrors commented 2 weeks ago

I have updated my testing steps to include steps for testing having 0 train tracks in traintracks.xml, and having more than 50 tracks in it.

Sage-of-Mirrors commented 2 weeks ago

Hey, just checking in. Is there anything I can do to help the review process along?

martonp96 commented 6 days ago

Hey @Sage-of-Mirrors, Checked the code with your testing steps and it works as intended. My only concern is reloading the tracks when trains are already spawned leads to game crash. Other than that, its ok from me.

Sage-of-Mirrors commented 6 days ago

Hey, thank you for the review!

I am aware of that crash; I did not address it in my original commit because it should only crash the client that calls LOAD_TRACKS_FROM_FILE. Would you like me to add a check in that native to prevent a reload if there are any trains in the level?

martonp96 commented 6 days ago

Hey, thank you for the review!

I am aware of that crash; I did not address it in my original commit because it should only crash the client that calls LOAD_TRACKS_FROM_FILE. Would you like me to add a check in that native to prevent a reload if there are any trains in the level?

Yes please, at least people will know something went wrong when the new api is misused.

Sage-of-Mirrors commented 6 days ago

While I was discussing this with @outsider31000, I was reminded that I had a different approach to this problem that involved an fxmanifest statement rather than natives: https://github.com/citizenfx/fivem/pull/2432. The PR was rejected by a previous member of the team.

Which method does Cfx feel is best? An fxmanifest statement seems much safer than a native that can be called at any point, since the statement would only be run at resource init. It would also get rid of the crashing-when-trains-exist issue altogether, since the reload only happens once, before any trains have been spawned.

outsider31000 commented 6 days ago

Yeah I cant see any use case to reload tracks In runtime, even if there was one, it would still need to do it for all clients, doesnt seem a viable thing to do/allow it? It should kinda be considered like a "ymap" load it once. But that's just my opinion.

prikolium-cfx commented 6 days ago

While I was discussing this with @outsider31000, I was reminded that I had a different approach to this problem that involved an fxmanifest statement rather than natives: #2432. The PR was rejected by a previous member of the team.

Which method does Cfx feel is best? An fxmanifest statement seems much safer than a native that can be called at any point, since the statement would only be run at resource init. It would also get rid of the crashing-when-trains-exist issue altogether, since the reload only happens once, before any trains have been spawned.

Just to be honest I liked previous solution more too. We need to think about it and maybe previous way will fit community needs better and in the same moment will be more stable

martonp96 commented 6 days ago

Of course the optimal solution is to make it configurable on server side and load it once on connect, while checking for multiple loaded track files to avoid collision. I thought this was designed with runtime editing in mind.

Sage-of-Mirrors commented 6 days ago

Of course the optimal solution is to make it configurable on server side and load it once on connect, while checking for multiple loaded track files to avoid collision. I thought this was designed with runtime editing in mind.

This native approach was suggested by disquse, who iirc felt that a manifest statement was not congruent with RedM's design principles and would be made obsolete when replacing the level meta became possible in RedM.

Perhaps a compromise can be reached: there is already a datafile loader for track data, TRAINTRACK_FILE. The original game code for the track loader does not attempt to dispose of the current track pool, it just adds the tracks to the existing pool and re-runs the post-processing functions on the whole pool. What if, instead of natives or a dedicated manifest statement, I modified the train file loader to dispose of the current track pool before loading the XML that was passed in? That way, a resource could just use datafile TRAINTRACK_FILE '<resource path>/data/traintracks.xml, and the tracks in the given file would replace the pool instead of add to what's already there.

Maybe there could be a server config boolean to explicitly turn on this behavior?

Sage-of-Mirrors commented 6 days ago

The crux of the issue here, however, is that the track resources have to be distributed to the client.

Another idea: maybe a replicated fxserver convar that the client checks for before loading the default track data? traintrack_override_xml or something. Server owner would still need to package the track data as a resource to distribute it, though.

outsider31000 commented 6 days ago

I would say that the best approach would be to keep it within the resource loading the tracks. A convar for this instance would not be ideal compared to using resource metadata in the fxmanifest.

Sage-of-Mirrors commented 6 days ago

I have quite a bit of free time, so I will update the fxmanifest approach and add my crash patches to it.

Let me know what Cfx decides; I'm happy to finalize either method.

Sage-of-Mirrors commented 5 days ago

Here's the manifest version of this functionality: https://github.com/citizenfx/fivem/compare/master...Sage-of-Mirrors:fivem:train-manifest-statement. This is what a resource using it would look like:

fx_version "cerulean"
game "rdr3"

rdr3_warning "I acknowledge that this is a prerelease build of RedM, and I am aware my resources *will* become incompatible once RedM ships."

files {
  'data/traintracks.xml',
  'data/greatplains_jnctw_rdr1.dat',
  'data/greatplains_main_rdr1.dat',
  'data/mexico_jncte_rdr1.dat',
  'data/mexico_jnctw_rdr1.dat',
  'data/mexico_main_rdr1.dat',
  'data/trains_old_west_intersection01.dat',
  'data/trains_old_west_intersection02.dat',
  'data/trains_old_west01.dat',
  'data/trains_old_west02.dat',
  'data/trains_old_west03.dat',
}

replace_traintrack_file 'data/traintracks.xml'

If this is a better solution than the one presented in this PR, I can close it and make another for the manifest version.

martonp96 commented 5 days ago

Here's the manifest version of this functionality: master...Sage-of-Mirrors:fivem:train-manifest-statement. This is what a resource using it would look like:
fx_version "cerulean"
game "rdr3"

rdr3_warning "I acknowledge that this is a prerelease build of RedM, and I am aware my resources *will* become incompatible once RedM ships."

files {
  'data/traintracks.xml',
  'data/greatplains_jnctw_rdr1.dat',
  'data/greatplains_main_rdr1.dat',
  'data/mexico_jncte_rdr1.dat',
  'data/mexico_jnctw_rdr1.dat',
  'data/mexico_main_rdr1.dat',
  'data/trains_old_west_intersection01.dat',
  'data/trains_old_west_intersection02.dat',
  'data/trains_old_west01.dat',
  'data/trains_old_west02.dat',
  'data/trains_old_west03.dat',
}

replace_traintrack_file 'data/traintracks.xml'
If this is a better solution than the one presented in this PR, I can close it and make another for the manifest version.

@Sage-of-Mirrors Yes, that looks much better. @prikolium-cfx

Sage-of-Mirrors commented 2 days ago

Any updates on which approach to this problem is best?

prikolium-cfx commented 7 hours ago

Any updates on which approach to this problem is best?

Let's continue with your first implementation with declaration in manifest

Sage-of-Mirrors commented 31 minutes ago

Closing in favor of #2624.

citizenfx / fivem