Skip to content

Convert the MS packet's contents to JSON#398

Open
Cerapter wants to merge 11 commits intoAttorneyOnline:masterfrom
Cerapter:feature/ms_json
Open

Convert the MS packet's contents to JSON#398
Cerapter wants to merge 11 commits intoAttorneyOnline:masterfrom
Cerapter:feature/ms_json

Conversation

@Cerapter
Copy link
Contributor

@Cerapter Cerapter commented Feb 5, 2025

I offhandedly mentioned in The [DATA EXPUNGED] that if we wanna do something with the MS packet, probably the best way to go about it is to make at least its cursed insides a JSON, and the best way to go about that is to create a MVP of sorts.

So, here it is.

I specifically aimed this to be the minimalest of viable products -- there is no real extra fancy decision making about AO's MS packet, it's literally just the flattest possible representation of it, except in JSON.

I'll write the client part of it later, but until then, I'll keep this a draft.

As to how to proceed from this point on, it'll largely depend on what we want. The Akashi part of the deal is going to be the easier pain to deal with. This MS data representation is preferably thrown out, however.

I could also imagine a separate library just for the MS packet (and perhaps other packets later) that both the client and Akashi pull in.

@Salanto
Copy link
Collaborator

Salanto commented Feb 5, 2025

You have failed the clang vibe check.

@stonedDiscord
Copy link
Member

How do you want to handle the packet header?
Put it as the first key, wrap the arguments in a nested object or just put it behind MS# ?
Salanto suggested

[{
        "header": "foo",
        "content": {}
    },
    {
        "header": "foo",
        "content": {}
    }
]

2 years ago to handle multiple packets coming in at once when changing areas for example.

@Cerapter
Copy link
Contributor Author

Cerapter commented Feb 6, 2025

[{
        "header": "foo",
        "content": {}
    },
    {
        "header": "foo",
        "content": {}
    }
]

A header-data setup like that was the idea I entertained as well. This PR was actually born from an attempt locally where I was trying to do something like that, but then I realised it might make more sense to force some smaller JSON-ification through first.

The way I can see MS (and possibly other packets) reworked is:

  1. We change their contents to the flattest possible JSON as an MVP, but otherwise keep the AO MS#...#% "header" and "footer" for the packets for now.
  2. As this forces getting rid of QStringLists in the client code, when we get around to doing that, for this step, we simply just, well, get rid of them, and create a "proper" object representing the contents of a packet, and access those.
  3. This should clear up exactly when and what is getting accessed. When we have that, we can start enforcing functions having limited access to stuff. Not everything needs to directly access chatmessage, for example play_preanim() could most likely do with a const ref to / copy of char_name, preanim, side, and deskmod.
  4. Moving to a fully JSON-based packet format at this point should be more than possible.

@Cerapter Cerapter marked this pull request as ready for review February 9, 2025 14:20
@Cerapter
Copy link
Contributor Author

Cerapter commented Feb 9, 2025

The PR is ready to review. I tested all I could, but it obviously needs a much more intensive testing.

Copy link
Collaborator

@Salanto Salanto left a comment

Choose a reason for hiding this comment

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

This should be merged after the next check-in. It works, but needs extensive testing since we are breaking the chat protocol here. Great foundation.

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

Successfully merging this pull request may close these issues.

3 participants