RigsOfRods / rigs-of-rods

Main development repository for Rigs of Rods soft-body physics simulator
https://www.rigsofrods.org
GNU General Public License v3.0
991 stars 175 forks source link

Fix #3041 - Random crashes when you spawn/switch/delete actor (lite edition) #3057

Closed ohlidalp closed 1 year ago

ohlidalp commented 1 year ago

A lightweight take on #3041 - split off from #3042 with only the bare essentials.

Please test these scenarios: https://github.com/RigsOfRods/rigs-of-rods/pull/3042#issuecomment-1567306964

Turns out I was really careless when I made class Actor a RefCountingObject<> - sure, the refcounting was broken before and this helper works flawlessly, except you have to remember not to use it from threads, and I didn't even think about it. For now I worked it around by tossing a mutex in RefCountingObject<>, but a sensible approach would be to just use IDs in threads and only manipulate the objects on main thread via message queue.

ohlidalp commented 1 year ago

Conclusion: I regret nothing. RefCountingPtr works flawlessly, the refcount is now thread-safe but there's a debug assert() which ensures only the main thread manipulates refcount so there's no locking overhead. I also learned some new tricks and the final solution is not only workable but even ellegant.

The picture shows an ad-hoc debug panel I made during the process. It's not included :) obrazek

CuriousMike56 commented 1 year ago

Crashes gone, great news! But now: image If a negative action key is apparently incorrect, shouldn't the value be parsed as positive instead? The same errors also occur on the "0.4.0.7 only" versions of trains such as the Henschel DHG 700C Another example: Reachstacker HRS-50 image The steering no longer works with this PR.

ohlidalp commented 1 year ago

I restored support for negative commandkeys in 'triggers' - they're now redirected to a hashmap so any value is acceptable. I was unable to figure out what is their meaning though. My best guess is, since they caused a memory write outside the commandkeys array, that they created an impression of a "virtual" commandkey which didn't interfere with any actual F-key. Either way it was still an untolerable memory corruption.

The Reachstacker HRS-50 was broken due to 2 typos... fixed now.