[Codechange] Implemented a thread-safe truck deletion queue

[ulteq]

Trucks can no longer be deleted while the sim thread is running.

Singleplayer:

• Reloading a truck now properly deletes the old truck

Multiplayer:

• Fixes crash (on linux) when a remote player disconnects after he has spawned a truck

[ohlidalp]

Trucks can no longer be deleted while the sim thread is running.

You probably wanted to say "Deleting trucks now possible while sim-thread is running". 😄

Thought I'm happy this fixes a bug, I don't like the design much. It adds another explicit "sync me now!" call + new mutex guard instead of using the existing infrastructure, i.e. processing queued deletes as a rudimentary game task in frameStarted()

I'm probably too lost in my 'proper-mt' vision, but here's how I imagine truck delete mechanism:

• main thread queues a delete request.
• when sim-thread is synced, the truck is excluded from all simulation . Actual de-allocation can be postponed if necessary.
• rendering thread removes all truck's visuals. IMPORTANT: all material instances must be unique to truck and removed here, too!
• sound handling: to be resolved

[ulteq]

It adds another explicit "sync me now!" call

You are right, we don't need that call, because handleTruckDeletion() is called inside frameStarted() after calling SyncWithSimThread().

It adds a new mutex guard instead of using the existing infrastructure

The issue is that delete requests can come from both the main thread and the network thread.

[ohlidalp]

The issue is that delete requests can come from both the main thread and the network thread.

Then make an extra queue for the network. When synced, transfer the requests to main delete queue.

Though I'm not sure how this fits in our current design - who and where synces with network thread, anyway?

Further, how is spawning over network handled this time around? Spawning and deleting should be treated as essentialy the same operation: add or remove a vehicle from simulation/rendering.

[ulteq]

I just realized that the truck deletes are already queued in the network thread.
I think we can safely remove all syncing / locking in the current truck deletion queue implementation.

1. https://github.com/ulteq/rigs-of-rods/blob/3c2a189b8c30209c97a1ce909f5a2699f009159d/source/main/physics/BeamFactory.cpp#L928-L954

called by:

1. https://github.com/ulteq/rigs-of-rods/blob/3c2a189b8c30209c97a1ce909f5a2699f009159d/source/main/network/StreamableFactory.h#L144

called by:

1. rigs-of-rods/source/main/network/NetworkStreamManager.cpp

   Line 226 in c8066e2

   syncRemoteStreams();

called by:

1. https://github.com/ulteq/rigs-of-rods/blob/3c2a189b8c30209c97a1ce909f5a2699f009159d/source/main/gameplay/RoRFrameListener.cpp#L1281

[ulteq]

Further, how is spawning over network handled this time around?

In syncRemoteStreams(): https://github.com/ulteq/rigs-of-rods/blob/3c2a189b8c30209c97a1ce909f5a2699f009159d/source/main/network/StreamableFactory.h#L101-L150

Which is indirectly called in the frameStep() method:

bool RoRFrameListener::frameStarted(const FrameEvent& evt)
{
    ...
    BeamFactory::getSingleton().SyncWithSimThread();
    ...
    if (gEnv->network)
    {
        // process all packets and streams received
        NetworkStreamManager::getSingleton().update();
    }

void NetworkStreamManager::update()
{
    syncRemoteStreams();
    receiveStreams();
}

[ohlidalp]

Hmm... the whole Stream*whatever subsystem looks like a lot of OOP cruft for a simple network handling to me. It seems to be designed to allow multiple managers (chat, character, beamfactory) to create their own streamfactories and register streams. Everyone calls lockStreams() all he wants. That's bullshit design. For network, we need to manage bandwidth - trucks are important, characters are less, chat is lowest priority. For local data, we can queue. Further, it's full of the "loop all streams to find one by StreamID" crap, just like our sound manager, our input manager, and damn, IIRC even BeamFactory.

How about being punks and rewriting the whole StreamMess into one compilation unit (Networking.{h/cpp}), which would look like this:

/// @file Networking.h

namespace RoR {
namespace Networking {

struct Stream { ... };

// Stuff from class Network, known as gEnv->network.
// It's pseudo-singleton anyway, so why not global functions?
SendMessageRaw()
SendMessage()
ReceiveMessage()

Connect()
Disconnect()

SenderThreadStart()
ReceiverThreadStart()
// etc... from Network.h
// EDIT: Right, we need a substitute for `if (gEnv->network != nullptr)`:
IsMultiplayerActive();

// Stuff from NetworkStreamManager -- was a singleton 
// For the time being, we can preserve the abstract stream principle
AddLocalStream()
AddRemoteStream()
RemoveStream()
SendStreams()
ReceiveStreams()

// StreamableFactory => eliminate! 
// Use `#include "Networking.h"` and the function calls directly, please!

} // namespace Networking
} // namespace RoR

and this

// Networking.cpp

namespace RoR {
namespace Networking {

static container<> s_streams;
static socket_t s_socket;
static whatever_t s_whatever;

void SendMessage(message_t msg)
{
    s_socket.send_message(msg);
}

} // namespace Networking
} // namespace RoR

[ulteq]

Split into separate pull requests:

• [Codechange] Propagate the state of recycled trucks over the network,… #906
• [Cleanup] Removed broken truck deletion method #908
• [Codechange] Disable sounds on truck deletion #907
• [Codechange] Unified truck deletion methods #909