[OF-1784] Multiplayer System #842

MAG-mv · 2025-10-27T10:50:21Z

This is part 1 of the server-side leader election work, which contains a new MultiplayerSystem which exposes the necessary functionality for leader election from the mcs MultiplayerServices api.

This work allows us to:

GetScopesInSpce - This is needed to get the current scope information
UpdateScope - This is needed to be able to turn on leader election
PerformLeaderElectionInScope- This is added for testing, as leader election is done automatically

While setting up the MultiplayerSystem, I ran into an issue with our URIs, which is fixed within this ticket. We were conflating the MultiplayerConnection Uri and the MultiplayerServices uri, which should be separate. This has been fixed by creating 2 separate uris. and also applying the root uri transform on both uris, as they both need this.

github-actions · 2025-10-27T10:50:37Z

🚀 Pull Request Review Guidelines

Thank you for taking the time to review this Pull Request. The following is a summary of our Pull Request guidelines. The full guidelines can be found here.

💬 How to Provide Feedback

We use a comment ladder when leaving review comments to avoid any ambiguity.

Tag	Is response required?	Is a change required?	May the PR author resolve?
[Fix]	✔️	✔️	❌
[Consider]	✔️	❌	✔️
[Question]	✔️	❌	❌
[Nit]	❌	❌	✔️
[Comment]	❌	❌	✔️

All comments should be prefixed with one of the above tags, for example:
[fix] Your editor is still set to use tabs instead of spaces, or this file is old and needs to be reformatted.

⚠️ Reviewers should be sure to resolve conversations if they feel their feedback has been addressed sufficiently.

🎯 PR Author Focus Areas

Link Commits to Conversations After making a change in response to a reviewer's comment, link the specific commit in the comment thread. This will allow the reviewer to view the change without having to hunt the codebase for it.
Breaking Changes Any breaking changes made to the public interface must be specifically called out in the PR. The preferred format is a bullet list enumerating the specific nature of the change/s, as well as the types and method signatures that are affected. If necessary, migration guidance should also be provided here.

Thanks again for taking the time to review this Pull Request.

MAG-ElliotMorris

Mostly doc stuff.

Library/include/CSP/Systems/Multiplayer/MultiplayerSystem.h

MAG-ElliotMorris · 2025-10-27T11:00:18Z

Library/include/CSP/Systems/Multiplayer/MultiplayerSystem.h

+    /// @brief Gets all scopes associated with the given space id.
+    /// @param SpaceId const csp::common::String& : The id of the space we want to get scopes for.
+    /// @param Callback csp::systems::ScopesResultCallback : Callback when asynchronous task finishes.
+    /// @pre Must already have entered the space of the SpaceId parameter.


[Consider] Stating what happens when this precondition is violated.

Fixed in: 2553f80

[Comment] I still think this erroring behaviour is poor, an empty array of scopes is not a good marker for "You were not in the space", it could be caused by anything. I'm presuming we don't get a helpful error from CHS telling us why the request failed, which is what I'd ideally like.

[Question[ To give better errors, would we have to query CHS again to check if we're in the space beforehand? Seems like context the forwarded CHS response should be able to provide.

Fixed in: 9860b3d

Used the space system to check if we're in the space. We do this for HotspotSequenceSystem, as well.

Library/include/CSP/Systems/Multiplayer/MultiplayerSystem.h

MAG-ElliotMorris · 2025-10-27T11:04:12Z

Library/include/CSP/Systems/Multiplayer/MultiplayerSystem.h

+    CSP_ASYNC_RESULT void GetScopeLeader(const csp::common::String& ScopeId, ScopeLeaderResultCallback Callback);
+
+    /// @brief Starts leader election for the given scope.
+    /// This should not need to be called outside of testing, as leader election is done automatically.


[Consider] Maybe we should do the __ thing for the naming that we've been talking about for this? Heck, i'd even feature flag it.

Fixed in: 2553f80 by setting these types NO_EXPORT, until we understand how scopes are going to be used in CSP

[Question] I was under the impression we agreed on the __ naming convention for methods that only make sense in a test context going forwards. Am I mistaken in my understanding of either our new standard or what this method is? We can't just agree to do things and then not do them. I'm fine with this code, but you'll need to bring a reason why you didn't want to use the naming convention to the next sync.

Fixed in: 16e3767

Library/src/Systems/Multiplayer/MultiplayerSystem.cpp

MAG-ElliotMorris · 2025-10-27T11:22:16Z

Library/src/Systems/Multiplayer/MultiplayerSystem.cpp

+
+void MultiplayerSystem::GetScopesBySpace(const csp::common::String& SpaceId, ScopesResultCallback Callback)
+{
+    constexpr const char* ReferenceType = "GroupId";


[Consider] Thinking of/Finding a central location for magic strings like this (Unless it is specific to just this method). At least doc why this is what it is/is it arbitrary or magic, if nothing else.

Added docs. I can't think of any other magic string, and I dont ant to create a file for one string at this point. f99c3dd

[Consider] The most important piece of information about this string is whether it is magic or not, ie, could you put anything here, or does it relate to other data? Ie is it something that CHS or we populate so it has to say exactly "GroupId". That's not expressed in the comment.

Library/src/Systems/Multiplayer/MultiplayerSystem.cpp

Library/src/Systems/SystemsManager.cpp

Tests/src/PublicAPITests/MultiplayerSystemTests.cpp

Library/include/CSP/Systems/Multiplayer/Scope.h

Library/include/CSP/Systems/Multiplayer/ScopeLeader.h

Library/src/Systems/Multiplayer/MultiplayerSystem.cpp

MAG-ChristopherAtkinson · 2025-10-27T12:52:44Z

Tests/src/PublicAPITests/MultiplayerSystemTests.cpp

+    }
+
+    // Ensure default scope has correct default values.
+    const csp::systems::Scope& DefaultScope = GetScopesResult.GetScopes()[0];


[Question] Does CHS guarantee the order scopes are returned in, e.g. these are not ordered by last modified

Unsure, but there is always 1 default scope, currently.

Can we check with MCS as a precaution?

@jeromagnopus How are the scopes retrieved from /scopes/referenceType/{referenceType}/referenceId/{referenceId} ordered?

MAG-ChristopherAtkinson · 2025-10-27T12:58:39Z

Tests/src/PublicAPITests/MultiplayerSystemTests.cpp

+    if (GetScopesResult.GetScopes().Size() != 1)
+    {
+        FAIL();
+    }


[Question] This assumes that the only scope being returned is the default; it is possible that in the future we may have several scopes by default, which would cause this test to be invalid. Do we know if any additional scope is intended to be introduced? (Thinking that POI could have an impact here)

None that I know of. However, I think this is useful to keep here so we know that the behaviour has been changed

Can we check with MCS as a precaution?

@jeromagnopus Do you know of any additional default scopes that will be created?

We were previously conflating the multiplayer hub uri and the multiplayer service uri, which require different values, but also a both have the same change to the RootUri. 2 seperate uris have now been setup to represent the 2 The helper function has also been updated which transforms the RootUri to be generic, so it can be used for both multiplayer uris

This adds a multiplayey system where we can put functionality from the chs multiplayer service api. We may want to make other system abstractions in the future, as this api contains a lot of different functionality. For now, only the functions needed for testing leader election have been implemented

…e space

MAG-ChristopherAtkinson

Approved on the understanding that the open question will be answered.

MAG-mv requested a review from a team as a code owner October 27, 2025 10:50

MAG-mv requested review from MAG-ChristopherAtkinson and MAG-ElliotMorris October 27, 2025 10:50

MAG-ElliotMorris requested changes Oct 27, 2025

View reviewed changes

MAG-ChristopherAtkinson requested changes Oct 27, 2025

View reviewed changes

MAG-mv force-pushed the OF-1784-server-side-election branch from 641d1f3 to 65e9952 Compare October 27, 2025 13:34

MAG-mv added 7 commits October 30, 2025 12:29

[OF-1784] docs: Add additional docs

04ca912

[OF-1784] test: Add test for getting scopes out of space

7dcfcdf

[OF-1784] refac: updating apis to use unique_ptrs

00bf02c

[OF-1784] refac: Update to use case with logging

63f0970

[OF-1784] docs: Adding comments to reference type

a22b329

MAG-mv force-pushed the OF-1784-server-side-election branch from f99c3dd to a22b329 Compare October 30, 2025 12:29

MAG-mv requested review from MAG-ChristopherAtkinson and MAG-ElliotMorris October 30, 2025 12:29

[OF-1784] fix: Fixing initialization order

917d04a

MAG-ElliotMorris approved these changes Oct 31, 2025

View reviewed changes

MAG-mv added 2 commits November 3, 2025 11:40

[OF-1784] fix: Updated test function name

16e3767

[OF-1784] fix: Updated GetScopesBySpace to error if user is not in th…

9860b3d

…e space

MAG-ChristopherAtkinson approved these changes Nov 3, 2025

View reviewed changes

[OF-1784] docs: updated GroupId comment

43b23e1

[OF-1784] Multiplayer System #842

Are you sure you want to change the base?

[OF-1784] Multiplayer System #842

Uh oh!

Conversation

MAG-mv commented Oct 27, 2025

Uh oh!

github-actions bot commented Oct 27, 2025

🚀 Pull Request Review Guidelines

💬 How to Provide Feedback

🎯 PR Author Focus Areas

Uh oh!

MAG-ElliotMorris left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

MAG-ElliotMorris Oct 31, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

MAG-ChristopherAtkinson left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

MAG-ElliotMorris Oct 31, 2025 •

edited

Loading