cwi-dis / VR2Gather

Unity application framework for immersive social VR
MIT License
4 stars 6 forks source link

Orchestrator code needs refactoring #137

Open jackjansen opened 8 months ago

jackjansen commented 8 months ago

The current orchestrator code is a complete mess.

Related to #119, #122, #123

On the lowest level we have OrchestratorWSManager. This manages the websocket and its socketIO interface.

Then there is a low-level class OrchestratorWrapper that should in principle know only about the commands/responses implemented, and how to wrap these for the OrchestratorWSManager (basically the JSON encoding of arguments and return values). There are a few helper classes and interface definitions too in Orchestrator/API/OrchestratorWrapping. But these classes also import BestHTTP methods directly.

On top of this we have OrchestratorController. This contains most of the logic on how we use the orchestrator to create sessions, join sessions, etc. It also contains state: who is the current user, are we connected to a session, etc. So, it is really the session management API, implementing sessions on some underlying communication paradigm.

But it leaks all sorts of stuff to higher layers in a low-level-ish way. It should really have only a few callbacks (Actions) to the higher layers to say

There is an extension class to the OrchestratorController (I think that's the C# term but not sure) OrchestratorControllerExtensions. This provides the typed messages within a session that all participants get a consistent view of the world. This could also use cleanup and a rationalized API but I'm going to leave that for a future bug report.

Then finally we have OrchestratorLogin which should really be only about the GUI, but it currently contains a whole lot of business logic that needs to move down into the OrchestratorController.

jackjansen commented 8 months ago

We should look at which bits of state are kept where. My feeling is really that of the mentioned classes the OrchestratorController is the only one that should have state.

jackjansen commented 8 months ago

And another thing we can look at:

I hope @troeggla can provide some suggestions for the first two bullets, and @jackjansen will look at the last.

troeggla commented 8 months ago

I've been experimenting with this: https://github.com/itisnajim/SocketIOUnity Will commit my findings to my fork of VR2Gather