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.

Sign up for GitHub

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

Jump to bottom

Fixed paused actor physics resuming after switching to another actor #3146

Merged
merged 3 commits into from
Jun 27, 2024
Merged

Fixed paused actor physics resuming after switching to another actor #3146

merged 3 commits into from
Jun 27, 2024

Conversation

ohlidalp
Copy link
Member

Fixes #3105.

This was quite an endeavor - it turned out there's no single spot to correctly reset the physics pausd state (or skeletonview state - related). While researching how to add it, I realized scripts may want to know when (un)linking of actors happens, so I added a script event SE_GENERIC_TRUCK_LINKING_CHANGED for it.

What changed: code changed signifficantly; all data remained the same - code handling hooks/ties/ropes/slidenodes got only cosmetic edits. There are many new sanity/integrity checks in the (un)linking logic which will trigger Asserts on Debug and just prevent damage on Release.

What to test: Operation of ties/hooks/ropes/slidenodes, including savegame and abrupt deleting of one of the actors.

@CuriousMike56
Copy link
Collaborator

CuriousMike56 commented Apr 22, 2024 •
edited
Loading

Almost perfect. Found one crash scenario:

  • Spawn Liebherr LTM 1050-3.1 and any load, such as the Rock pack
  • Press O to attach ties to the load
  • Delete the load, disconnecting ties first doesn't matter.
  • Spawn a copy of the load with CTRL+PERIOD, or spawn a different load
  • Try to attach ties to other load -> crash

Under Debug I get infinite asserts when trying to backspace with the crane:

RoR_2024-04-22_19-22-25.mp4

And clicking retry gives:
devenv_2024-04-22_19-22-55

@ohlidalp
Copy link
Member Author

ohlidalp commented May 27, 2024 •
edited
Loading

I fixed the asserts on backspacing the Liebherr crane, and added many more asserts in the process, because those that triggered were completely legit, SyncReset() was actually forcing hooks/ropes/ties to inconsistent state.

@ohlidalp
Copy link
Member Author

ohlidalp commented May 28, 2024 •
edited
Loading

obrazek
I pressed L to attach the hook, then tried to delete via top menu.
EDIT: turned out to be faulty assert.
EDIT2: fixed the actual problem.

@CuriousMike56
Copy link
Collaborator

CuriousMike56 commented May 30, 2024 •
edited
Loading

Attached loads now move faster than the crane when using live repair mode (soft reset enabled):

8mb.video-lNy-61XoPei1.mp4

vs master:

RoR_2024-05-30_16-16-36.mp4

@ohlidalp
Copy link
Member Author

ohlidalp commented Jun 13, 2024 •
edited
Loading

@CuriousMike56 Ready for another test.

Turns out there was another bug in forced unlinking of inter-actor beams (both when deleting actor and hard-resetting) which left the actor data in inconsistent state. It's actually a very old bug in code written by Ulteq in ~2016/17 to fix multithreading problems related to inter-linking. It passed unnoticed until I introduced the RefCountingObjectPtr<> mechanic, which triggered the need for MSG_SIM_ACTOR_LINKING_REQUESTED and a bunch of asserts() to finaly kick the data into consistency.

@CuriousMike56
Copy link
Collaborator

CuriousMike56 commented Jun 16, 2024 •
edited
Loading

#3146 (comment) still happens

@ohlidalp
Copy link
Member Author

ohlidalp commented Jun 18, 2024 •
edited
Loading

@CuriousMike56 Mystery solved: why did this only manifest when using ties (your scenario) and not hook (my initial scenario)? The reason is - by accident, the movement sync between the crane and the load was done once for each interconnecting beam, instead of once per actor pair. And because there's always just one hook beam, it worked OK with a hooked load, but there may be multiple ties (in this case 4), the load moved faster (4x in this case).

Fixed by adding a cache of unique actor pairs, and updating it each time linking changes.

@CuriousMike56
Copy link
Collaborator

CuriousMike56 commented Jun 18, 2024 •
edited
Loading

Crane won't follow if you switch to the crate:

8mb.video-kdq-xAMJiVUT.mp4

Also happens with a truck+trailer+load:

RoR_2024-06-18_02-33-34
RoR_2024-06-18_02-35-10

@ohlidalp
Reverted the SyncLinkedActors() commit which caused RigsOfRods#3105
c017ad8 
This reverts commit 9431e1b where I tried to tidy-up duplicate syncing code but oversimplified the logic to reset unlinked actors. As result, the physics paused state and skeletonview state could only be active on player's actor and connected actor, not on any free standing one.
@ohlidalp
Copy link
Member Author

ohlidalp commented Jun 19, 2024 •
edited
Loading

@CuriousMike56 I apologize for all the trouble - it's all because I tried to automagically sync all state toggles in one place.

I'm rebooting this PR by reverting the offending commit. The paused physics problem is gone, as well as all the LiveRepair glitches, also the crash in #3146 (comment) is gone. However, now I'm observing these glitches:

  • when you release a load from a hook (be it by pressing L or Hard-resetting), the skeletonview and physics paused states don't get reset. In 2022.12 it's the same except physicspause does get reset when unhooking by L (not by hardreset tho.).
  • when you release a load from ties using O key, skeleton view gets reset, but physics-pause does not. Also when you hard reset, everything remains dirty. 2022.12 behaves the same.

@ohlidalp
Copy link
Member Author

ohlidalp commented Jun 20, 2024 •
edited
Loading

@CuriousMike56 I've refurbrished the prior commits without all the parts that touch LiveRepair (aka interactive reset).
I tested all these cases:

  • Switching from a physics-paused actor to another one -> stays paused.
  • Unhooking/untying the load with sekeletonview or physics paused -> both are turned off on the load
  • Removing the crane (with hooked/tied load) with sekeletonview or physics paused -> both are turned off on the load
  • Hard-resetting the crane with sekeletonview or physics paused -> both are turned off on the load
  • movement in soft reset when driving the crane -> works OK, one follows the other
  • movement in soft reset when sitting in the load -> works OK, one follows the other

@CuriousMike56
Copy link
Collaborator

Seems the only remaining issue is the truck+trailer+load combo:
RoR_2024-06-22_00-42-33
RoR_2024-06-22_00-42-48
Master (Viper N/B is visible, soft reset works as expected):
RoR_2024-06-22_00-40-55

@ohlidalp
Copy link
Member Author

@CuriousMike56 Ready for another test. I had to revert most of my new code, including the asserts. I re-tested all the above scenarios, including the crash scenario you found originally, including the truck/trailer/car combo.

@ohlidalp
🐛 Fixed hook/tie/rope glitches.
58520dd 
PROBLEMS:
* when you release a load from a hook (be it by pressing L or Hard-resetting), the skeletonview and physics paused states don't get reset. In 2022.12 it's the same except physicspause does get reset when unhooking by L (not by hardreset tho.).
* when you release a load from ties using O key, skeleton view gets reset, but physics-pause does not. Also when you hard reset, everything remains dirty. 2022.12 behaves the same.
* When you Hard-reset or delete a vehicle with hooks/ties attached, they are not removed correctly, causing a crash if re-attached. The `DisjoinInterActorBeams()` helper (used by SyncReset() and Actor destructor) was leaving data in inconsistent state - specifically, it properly removed the inter-beams themselves and linkage records (both the per-actor and global lists), but it didn't update the `tie_t/hook_t/rope_t` objects itself - these kept pointing to the deleted/reset actor.

SOLUTIONS:
* Remake the actor-linking code to track when 2 actors become linked/unlinked - and sync the sekeletonview & physicspause there.
* Instead of employing custom code to do Hard-reset, extend the existing (old!) `hookToggle()/tieToggle()/ropeToggle()` funcs (used for regular locking/unlocking of those elements) to also do forced unlocking upon removing/SyncReset-ing an actor, and then modify `DisjoinInterActorBeams()` to use them.

DEV NOTE:
This is quite a significant remake of actor-linking code; a direct follow-up to d36c4ec which introduced ✉✉ MSG_SIM_ACTOR_LINKING_REQUESTED. A remake was needed because there was no single spot to correctly reset the skeleton/physicspause on actor unlink - there was simply no concept of "actors were just linked/unlinked". While researching how to add it, I realized scripts may want to know when that happens, so I added a script event `SE_GENERIC_TRUCK_LINKING_CHANGED` for it. Keep in mind there can be multiple interlinking beams at the same time, so not every added/removed beam means linking changes. The new event also helps navigate the codebase and documents how stuff works.

CODECHANGES:
* SimData.h: the ActorLinkingRequestType enum was merged with HookAction enum and got new fields: HOOK_RESET, TIE_RESET, ROPE_RESET - the `DisjoinInterActorBeams()` function now uses these.
* Actor.h: `hookToggle()/tieToggle()/ropeToggle()` funcs got new parameter `forceunlock_filter` and all now accept ActorLinkingRequestType param;
* Actor.cpp: `SyncReset()` no longer manipulates beams/hooks/ties/ropes directly - that's all done by the extended `DisjoinInterActorBeams()`.
* ScriptEvents.h - added SE_GENERIC_TRUCK_LINKING_CHANGED
* All other files are just fallout from changes above.
@ohlidalp
🐛 Fixed 'L' not locking slidenodes.
14017e1 
PROBLEM: typo (a copypaste derp) in GameContext.cpp

ADDITIONAL FIX: an assert() when loading Penguinville-TrolleyLine terrain. The blendmap info was incomplete (understandable since the terrain is meshed-only) and the game ended up reading bad memory. Fixed by adding a check for valid data which reports "[RoR|Terrain] Page {}-{} has no blend layers defined, blendmap will not be set up." to RoR.log.
@ohlidalp ohlidalp merged commit d992377 into RigsOfRods:master Jun 27, 2024
2 checks passed
@ohlidalp ohlidalp deleted the 3105_Mike_physicsPaused branch June 27, 2024 22:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@CuriousMike56 CuriousMike56 CuriousMike56 approved these changes

@tritonas00 tritonas00 Awaiting requested review from tritonas00

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Paused actor physics resume when switching to another actor
2 participants
@ohlidalp @CuriousMike56