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

Orchestrator code needs refactoring #137

Open
jackjansen opened this issue Feb 19, 2024 · 3 comments
Open

Orchestrator code needs refactoring #137

jackjansen opened this issue Feb 19, 2024 · 3 comments
Assignees

Comments

@jackjansen
Copy link
Collaborator

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

  • Something has changed in your session state
  • Something has changed in the global state (available sessions)
  • A high-level message was received within your session

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 jackjansen self-assigned this Feb 19, 2024
@jackjansen
Copy link
Collaborator Author

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
Copy link
Collaborator Author

And another thing we can look at:

  • which messages are we sending to the orchestrator that are pointless (because the orchestrator doesn't need it to do its job),
  • which parameters are pointless (for example the password parameter to login)
  • which reply parameters/return values are pointless (for example the userName and userId in the login response)

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

@troeggla
Copy link
Member

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants