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

feat(extra-natives/five): Add SET_INTERIOR_PROBE_LENGTH for FiveM #2371

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

Conversation

z3t4s
Copy link
Contributor

@z3t4s z3t4s commented Feb 1, 2024

Goal of this PR

Allow servers to customize the probe distance used in CPortalTracker.

All dynamic entities own an instance of CPortalTracker. It is used to detect traversal from and into interiors, through portals and other related actions.
By default the probing function inside CPortalTracker's tick/update function probes downwards 4.0 units, trying to extract the interior the entity is in from collision data (staticboundsstore).

In many cases this will not work, leading to ~30 hard coded special cases based on interior archetype hash. Rockstar extends the probing distance for these interiors, most notably in xs_arena_interior to 150.0f.

How is this PR achieving the goal

By adding a new FiveM native SET_INTERIOR_PROBE_LENGTH to give servers individual control of game behaviour depending on their needs.

  • The override value is applied to all probes shorter than itself.
  • User input is clamped to 0.0f - 150.0f
  • Session reset is handled, the override is reset on disconnect

Test code:

RegisterCommand("setInteriorProbeLength", function(src, args, raw)
    local probeLength = (tonumber(args[1]) + 0.0)

    print("Extending interior detection probes to: ", probeLength)
    SetInteriorProbeLength(probeLength)
end)

RegisterCommand("resetInteriorProbeLength", function()
    print("Resetting interior detection probes to default settings")
    SetInteriorProbeLength(0.0)
end)

Examples of use cases:

  • Specific custom interiors with unusual dimensions
  • Housing systems with script created floors
  • Improved render behaviour when trying to locate and noclip into underground interiors

Demo:
https://streamable.com/jxm0ux

image

This PR applies to the following area(s)

FiveM

Successfully tested on

Game builds: All available FiveM builds
Configurations: Debug / Release
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

Base game behavior

@qalle-git
Copy link

Great work!

@nikez nikez force-pushed the fix/PortalTrackerProbeDistance branch from 50788c2 to ff84a3f Compare February 2, 2024 10:03
@Disquse Disquse self-assigned this Feb 4, 2024
@blattersturm
Copy link
Contributor

blattersturm commented Feb 5, 2024

I assume the expectation is the script command will be invoked from an existing spatial check/trigger script on a server, correct? The checks in CPortalTracker::Update for probe type overrides look nasty so mimicking those would seem undesirable, but I'm not sure if this makes sense as a script command either.

@nikez
Copy link
Contributor

nikez commented Feb 5, 2024

Set it once and forget or have individual spatial checks, I guess that's entirely up to whoever implements it. I've tested noclip:ing around with it set to 150f without any issues, but not with full test coverage, of course. I think most people would survive with 16f as length.

And yeah, it's unreasonably expensive to implement this on a per interior basis.. due to the nastiness of the internals of this function.

@hamcoremusic
Copy link

Really looking forward to this extra native as it will greatly help my team in being able to create the visuals they want in their MLO interiors when using dynamic props. Is there a chance this is in an update coming up?

@gottfriedleibniz
Copy link
Contributor

gottfriedleibniz commented Feb 14, 2024

Looking at CPortalTracker::Update there is one case unrelated to those xs_arena_interior interior hard codes that I'm still trying to figure out to determine potential side-effects. It is the one where traceLength is determined by the archetypes bsRadius plus another hard coded value.

This conceptually seems different to the "we are in X, increase traceLength to 150.f"; CInteriorInst creation perhaps? Would be interested to know if you found anything here.

Another question would be on the naming here, e.g., SET_ vs OVERRIDE_ vs. OVERRIDE_MIN... vs. CLAMP_. I have no strong opinion, but others may.

@z3t4s
Copy link
Contributor Author

z3t4s commented Feb 15, 2024

Another question would be on the naming here, e.g., SET_ vs OVERRIDE_ vs. OVERRIDE_MIN... vs. CLAMP_. I have no strong opinion, but others may.

Completely indifferent to that too. Happy to accept suggestions.

Would be interested to know if you found anything here

I think you're talking about the case that is guarded behind bit11 of (rcx/CPortalTracker + 0x88) (2372 struct layout) correct?

That bit seems to be only ever set from one specific system:

  • Execute some kind of scene update on all entries of the CInteriorInst pool
  • Generate a single linked list based on instance->bbsphere and loop through it (I assume this is a list of all entities inside that CInteriorInst)
  • For all entities inside that instance:
    • Check if its a dynamic entity and thus has a portal tracker
    • Check if its (typePed || typeVehicle) || (typeObject && (pickup || projectile? || scriptEntity))
    • Check if CPortalTracker::0x88 bit3 && CPortalTracker::0x88 bit4 are 0 and finally set CPortalTracker::0x88 bit11
    • Call portalTracker->Update

If you want to look at that its at 0x140923A34 in 2372

The default value for these "interior entity registration" probes is 1.0.
Its extended to 2.0 if the owner entity of the portalTracker is scripted (or 3.2f if IS_ONLINE).

I assume this is a mission script fix. Probably a floating or ceiling mounted object somewhere.
Therefore I don't see an issue in extending those probes.

PS:

There is something else to consider. Currently this version of the PR is patching one call to the probing function inside a function that is only ever invoked from the function you referred to as CPortalTracker::Update. That is enough to fix visual bugs, audio occlusion and mini map. However it does not fix a few other potential issues which could be solved by directly hooking the function invoked one level deeper.
Things a direct hook would solve additionally:

  • Natives IS_COLLISION_MARKED_OUTSIDE && GET_INTERIOR_FROM_COLLISION would work correctly
  • audEmitterAudioEntity probes a fixed 20.0 units downwards to set the interior data on its associated naEnvironmentGroup. If an emitter is placed into a custom interior at a level higher than that, audio would not occlude correctly (0x14034832E)
  • Something that is related to ped and vehicle population. I suspect this is scenario / cargen / pedgen related (0x0140C2C5A5, 0x14070127C)
  • Some position check from camCinematicCamManCamera::vft_0xD0 (0x1402120AC). It compares the retrieved interiorInst against an entity flag of an entity inside camCinematicCamManCamera.
    This looks like (bool)intInstancePtr == this->entity->flags.someIsInsideInteriorBit to me and I highly suspect this is to prevent usage of the cinematic camera to peak outside or into an interior (depending on where you are). This one seems pretty important to fix, although I haven't tested if my theory is correct yet

I do think a few of those listed points are worth considering, but I would welcome some thought on that

@gottfriedleibniz
Copy link
Contributor

gottfriedleibniz commented Feb 15, 2024

Completely indifferent to that too. Happy to accept suggestions.

I trust Disquse's judgment on this.

I assume this is a mission script fix. Probably a floating or ceiling mounted object somewhere.

This is part of the confusion. From my own testing this path seemed to be hitting quite a few ped and vehicle entities with registered rage::netObject's. This being CInteriorInst creation related would make sense, i.e., figuring out what belongs to the interior.

Consequently, one concern is whether this override (esp. large values) for this specific case will cause any weird interactions with any interior-related bits in netcode. This would include our own NetInteriorLocationHacks and servers without OneSync enabled (e.g., ownership of far away entities). The limited probe length for mission entities w/ how some custom interiors are designed is certainly in issue in this case. Ugh.

Hard to be definitive here since so much of this (dis)assembly is nasty and gross. I do not think this should be prohibitive of throwing this on canary and seeing if users notice any unusual artifacts. Although, some clarity here would be nice.

audEmitterAudioEntity probes a fixed 20.0 units

Overriding this specific value seems like an instant value add.

Something that is related to ped and vehicle population.

Keeping this value default would probably be best 😅. The others I have no strong opinion on (e.g., modifying native behaviour vs. providing two new ones which offer custom probe lengths.)

@Mellowtri
Copy link

This is awesome! I have been wanting a solution like this for a long time. Thank you, can't wait to try it out. Hope to see it on canary soon.

Out of curiosity, did you look into why vehicles do not seem to care about floor distance in the same way peds do?
If I am in a vehicle and fly into an interior, it will load without any issues, even when I am not above the floor collision and the distance doesn't seem to matter at all.

CPortalTracker probeLength adjustments via custom ScRT Native

Co-authored-by: nikez <1504588+nikez@users.noreply.github.com>
Co-authored-by: okqut <38298546+okqut@users.noreply.github.com>
@z3t4s z3t4s force-pushed the fix/PortalTrackerProbeDistance branch from ff84a3f to 226cf72 Compare March 15, 2024 00:38
@z3t4s
Copy link
Contributor Author

z3t4s commented Mar 15, 2024

Finally had some time to work on this.

Added:

  • Fix for IS_COLLISION_MARKED_OUTSIDE and GET_INTERIOR_FROM_COLLISION
  • Fix for static emitters / audEmitterAudioEntity
    • Done via replicated CVar, active by default
    • Has constraints
    • Resets on network kill
  • Fix for local ped attaching to another dynamic entity and entering interiors without portal traversal
    • Demo: https://streamable.com/iov8i0
    • Improves already existing behaviour in CPhysical's attachment processing
    • Forces continuous re-scans while local ped is attached to another dynamic entity

Tested on all builds, debug and release
test script: https://pastebin.com/Ws6AedVi

Out of curiosity, did you look into why vehicles do not seem to care about floor distance in the same way peds do? If I am in a vehicle and fly into an interior, it will load without any issues, even when I am not above the floor collision and the distance doesn't seem to matter at all.

The game has quite a few conditions in which it either forces or prevents a re-scan of the interior.
Some are related to specific vehicle types (Aircrafts) others are velocity checks.
It also has special handling for standing on top of vehicles (probably to handle standing on top of trains going through tunnels)

@github-actions github-actions bot added the triage Needs a preliminary assessment to determine the urgency and required action label Mar 15, 2024
@gottfriedleibniz
Copy link
Contributor

Would it be possible to split the UpdatePortalTracker changes into its own PR? Bit of feature creep and a change that is a bit more dangerous. This will also allow us to get the interior probe length changes onto canary a bit faster.

Other comments:

  • The __fastcall keyword can be dropped (x64 and all).
  • Should SET_INTERIOR_PROBE_LENGTH allow values smaller than 1.0f? That seems to be the smallest value used by the game. Would there be any reason to go below that?
  • The default value to game_emitterAudioEntityProbeLength should match the current in-game value (20.0f; same on reset). Secondly, would there be any value in this being made scriptable? Would the same assumption above apply here?
  • After further consideration, I question whether changing those natives is something worth risking at this point. In addition to creating a weird dependency between natives, @Disquse in another conversation raised a good point in that we have generally avoided altering behaviour here except to maintain backward compatibility or fix in-game bugs.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
triage Needs a preliminary assessment to determine the urgency and required action
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

8 participants