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

MultiServer: implement NoText and deprecate uncompressed Websocket connections #4540

Conversation

black-sliver
Copy link
Member

What is this fixing or adding?

Part of the 300 player sync performance problems, hopefully.

Also some minor typing.

How was this tested?

Connecting with my demo clients with various tags and compression/no compression.

@github-actions github-actions bot added affects: core Issues/PRs that touch core and may need additional validation. waiting-on: peer-review Issue/PR has not been reviewed by enough people yet. labels Jan 22, 2025
Copy link
Collaborator

@qwint qwint left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code and docs updates look good, tested with a local server + postman
connecting with NoText tag saw no chat messages from the server and only saw the RecievedItems (no printjson) when cheating in items and when releasing another slot
reconnected without NoText tag and retried the same steps and saw the appropriate printjson messages

@black-sliver
Copy link
Member Author

For completeness sake, I tested the compression warning showing up/not showing up with a not-yet-released version of black-sliver/ap-textclient that has a build-time define for compression and with CommonClient.

@massimilianodelliubaldini
Copy link
Contributor

Idea for consideration: a tag called OnlyConcernsSelfText, so that text clients receive messages that only concern themselves (i.e. they found an item belonging to another/their game, or they received an item from another/their game).

Currently, a couple common client implementations have to include boilerplate like this in order to filter relevant JSON messages from the stream of all other text messages:

    def on_print_json(self, args: dict) -> None:
        # goes to this world
        if "receiving" in args and self.slot_concerns_self(args["receiving"]):
            relevant = True
        # found in this world
        elif "item" in args and self.slot_concerns_self(args["item"].player):
            relevant = True
        # not related
        else:
            relevant = False

        if relevant:
            self.announcements.put(self.raw_text_parser(copy.deepcopy(args["data"])))

        super(SC2Context, self).on_print_json(args)

It might be nice to have this kind of filter formalized so that each client doesn't need to do it themselves?

@Berserker66
Copy link
Member

At that point I'd rather have the text_handling: flags solution, than a bunch of tags.

@black-sliver
Copy link
Member Author

Is this something we actually want? I believe that we explicitly added stuff to the PrintJSON so the client can filter it, Adding additional filtering to the server seems less flexible and the amount of traffic/cpu time it might save seems negligible compared to clients with NoText.

Client side handling also has more potential UX features, such as message reordering that shows all of chat but makes sure you see your own and hide/show after the fact.

@massimilianodelliubaldini
Copy link
Contributor

That's fair! I just mention it here because Violet considered it worth mentioning in ap-core-dev.

Copy link
Member

@Berserker66 Berserker66 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My personal preference would be for text_handling: bitflags as I have a strong suspicion we're headed there anyway. And for this to be two PRs :P

@black-sliver
Copy link
Member Author

I mean, I can still change it, but I think we'd need a schema for it first.
I can also split the PR in two if that's preferred. Just lmk.

But, how do we specify "NoText" vs. "AllText"? Do we pass in 0xffffffff and hope that's enough bits? Is there gonna bit a bit per MessageType (countdown vs. chat)?

@NewSoupVi
Copy link
Member

I agree with Berserker that a bitflag would be cool, I don't think it needs to be super granular to be useful

Nonetheless I think this PR can go in as is - conceptually, I did not review the code, only the documentation.

@black-sliver
Copy link
Member Author

black-sliver commented Jan 29, 2025

Gonna merge as-is for now. Talked with Vi and we didn't see a strong (enough) reason to add text_handling at this point and NoText can be back compatible to whatever we come up with in the future.

Questions that should be evaluated if we want text_handling in the future:

  • (how) do we filter by type?
  • how do we filter future (not yet defined) flags? - how many bits do we reserve?
  • what flags need to exist? (local vs. remote items?)
  • is the lost flexibility on the client worth saving the boiler plate?
  • how many % of messages or bytes could we save?
  • is there a better way to save traffic, assuming that's the primary goal? (such as delaying/buffering and bundling messages or making single PrintJSON more efficient)

@black-sliver black-sliver merged commit 41898ed into ArchipelagoMW:main Jan 29, 2025
16 checks passed
@black-sliver black-sliver deleted the feat/multiserver-text-performance-pt1 branch January 29, 2025 00:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
affects: core Issues/PRs that touch core and may need additional validation. waiting-on: peer-review Issue/PR has not been reviewed by enough people yet.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants