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

Packets and field names #27

Open
rom1504 opened this issue Aug 7, 2015 · 21 comments
Open

Packets and field names #27

rom1504 opened this issue Aug 7, 2015 · 21 comments

Comments

@rom1504
Copy link
Member

rom1504 commented Aug 7, 2015

Currently packets names are taken from https://github.com/aadnk/ProtocolLib/blob/master/ProtocolLib/src/main/java/com/comphenix/protocol/PacketType.java#L123
I don't know where the fields names are coming from (@roblabla ?)

These names are different from what is currently found in http://wiki.vg/Protocol

Can reasonable names be chosen to make protocol.json more standard ?

The packet name difference with wiki.vg can be seen here https://gist.github.com/rom1504/1b432d5e7efc278015dd/revisions?diff=split

I mapped the new names to the old and got that which show mainly the field names difference :
https://gist.github.com/rom1504/64535ed808327b78c72e/revisions?diff=split

@roblabla
Copy link
Member

roblabla commented Aug 8, 2015

Field names are mostly taken from wiki.vg, ProtocolLib doesn't have any for them.

I think using packet names from ProtocolLib is still a better idea, than coming up with our own, y'know, wheel reinvention and all, it ain't great.

It might be worth going over all the fields and remove the discrepencies (like having entityId in some places and id in some others, for instance).

It's worth noting that if this is a problem stemming from having an automatic generator, it'd be easy enough to parse ProtocolLib's sources to find the name associated with a packetid...

@rom1504
Copy link
Member Author

rom1504 commented Aug 8, 2015

Okay so field names were originally taken from wiki.vg, but then they were probably changed because a lot of them are different from wiki.vg (see https://github.com/PrismarineJS/minecraft-data/blob/master/bin/wiki_extractor/protocol_extractor.js#L513 )

It's not so much about coming up with our own, but the fact is have there is now 2 standards : wiki.vg and ProtocolLib (and probably so many others in the list in that gist). If we are going to use protocol.json to produce something like wiki.vg/Protocol, we need to either take wiki.vg names, either push ProtocolLib names to wiki.vg. An other way might be to keep different names for every packet, but I'm not sure that's a great idea.

Yeah indeed there are some discrepancies in field names that should be fixed (like gamemode vs gameMode)

It's not a big problem from the generator side : I actually already fixed that by having a id to ProtocolLib names converter. I mean it's not a technical problem. Also the main point of my wiki generator is to be able to see the difference between the wiki and our protocol.json : to see if some types are wrong for example. The second thing I want to use it for is getting field notes into protocol.json. I'm almost there.

In a second time, that generator should be rendered useless by going the other way (protocol.json to something like wiki.vg/Protocol)

@nickelpro
Copy link
Member

+1 for wiki.vg because that's what SpockBot uses and I do not like change, plus the names are more descriptive (Collect Item vs COLLECT, Set Experience vs EXPERIENCE, Entity Properties vs UPDATE_ATTRIBUTES)

My only real weight on this conversation is almost all new third party projects end up using wiki.vg, if minecraft-data diverges from the wiki it adds a pretty big burden to starting and maintaining projects while learning all the differences between the two.

@roblabla
Copy link
Member

roblabla commented Sep 2, 2015

So after some talking with @rom1504 here's one possible idea : abstract away all the things. protocol.json would only contain unbiased data extracted from protocol.json : packetId, and types. Packet names and field names would go in another file :

  • protocollib_packetnames.json
  • wikivg_packetnames.json
  • wikivg_fieldnames.json

a packetnames file would look like this :

{
  "states": {
    "handshake": {
      "toClient": {
        "0x00": "disconnect"
      }
    }
  }
}

a fieldnames file :

{
  "states": {
    "handshake": {
      "toClient": {
        "0x00": [
          { "name": "field1name" },
          { "name": "field2name" },
        ]
      }
    }
  }
}

Then, before using a protocol.json file, you'd have to merge it with one packetName file and one fieldName file.

Open question : how do we deal with inner fields ?

Alternatives :

  • Keep the status quo
  • Keep a mapping file somewhere in the repo, like wikivgPacketNames, that would map the protocol.json names with the wiki.vg names, so projects who want to use wikivg names can do that without too much trouble.

@nickelpro
Copy link
Member

EDIT: As discussed on Gitter, I am a nincompoop who doesn't understand switches. Please ignore this comment

I am 100% in favor of that, mapping IDs to ordered types is an excellent idea. It also greatly simplifies building a protocol implementation on top of minecraft-data. I'm a little confused on what you mean by inner fields. So to me this is what the layout you describe looks like:

protocol.json

{
  "state": {
    "direction": {
      "0xID" : [
        {"type": "byte"},
        {
          "type": "switch",
          "compareTo": "0",
          "fields": {
            "0": [
              {"type": "int"},
              {"type": "float"},
            ],
          },
        },
      ],
    },
  },
}
wikivg_packetnames.json

{
  "state": {
    "direction": {
      "0xID" : "packet_name"
    },
  },
}
wikivg_fieldnames.json


{
  "state": {
    "direction": {
      "0xID" : [
        {"name": "byte_name"},
        {
          "name": "switch_name",
          "0": [
             "int_name", 
             "float_name"
          ],
        },
      ],
    },
  },
}

All fields are taken care of inherently

@rom1504
Copy link
Member Author

rom1504 commented Sep 2, 2015

(please use '''json instead of ''' for syntax highlighting)

@nickelpro
Copy link
Member

fixed

@roblabla
Copy link
Member

roblabla commented Sep 2, 2015

The only datatype that needs naming is containers (the top-level "fields" being a container as well). Switch fields don't need to be named. So I guess we could do something like :

{
  "state": {
    "direction": {
      "0xID" : [
        {"name": "byte_name"},
        {
          "name": "switch_name",
          "inner": [
             { "name": "int_name" }, 
             { "name": "float_name" }
          ],
        },
      ],
    },
  },
}

@rom1504
Copy link
Member Author

rom1504 commented Sep 3, 2015

Some POC of that idea

@rom1504
Copy link
Member Author

rom1504 commented Oct 6, 2015

Just a thought, but would it really be that hard to convert mineflayer to wiki.vg packet/field names ?
I think it would probably be doable and avoid making the protocol file unnamed and ugly.

@roblabla
Copy link
Member

roblabla commented Oct 6, 2015

I really, really don't like the idea of using wiki.vg packet names. They aren't meant to be set in stone, and can be changed by anyone anyday. The packet names from ProtocolLib are garanteed to stay the same.

It's not a problem of "can it be done". It's a problem of "should it be done".

Besides, I see nothing wrong with our names. They seem clear to me.

@Gjum
Copy link
Member

Gjum commented Oct 6, 2015

The problem is not using a particular set of names, but rather being difficult to convert your existing code to minecraft-data because of a different set of names.

So as @roblabla said, we could have no names at all, and provide an easy way to map them to your existing names.

If that's not an option, I agree with what @nickelpro said about new projects rather using wiki.vg names, even though they might change.

Also, for this to be widely used, the rest of the community should be more involved into these decisions.

@roblabla
Copy link
Member

roblabla commented Oct 6, 2015

This is an open ticket for a reason. I agree this needs discussion with the community at large. Do you think we should handle this kind of discussion differently ?

The problem I have with this issue in particular is that, to my eyes, minecraft-data should try its best to only contain data contained from the minecraft jar itself. Packet names and field names are something that don't really exist within the minecraft jar, so we must make a source authoritive. There are at least 5 different name sources I can think of (Bukkit, MCP, wiki.vg, ProtocolLib, obfuscated minecraft.jar). Out of all those, wiki.vg is arguably the 'worst choice', being changeable accross wiki edit (I don't know if this ever happened, but I would guess it already happened, and will continue to happen, since it's just a wiki).

I also fail to see how not using wiki.vg names poses a big barrier. The names are sufficiently close for the not-too-stupid user to figure out which is which. When I switched NMP to use named packets instead of IDs, all the parties involved at the time (including a few of the library's user) agreed ProtocolLib names were a smarter choice. The people using ProtocolLib in their Bukkit Plugins don't mind not being able to use the wiki.vg names either.

@nickelpro
Copy link
Member

It's nearly irrelevant to me what naming scheme is used internal to minecraft-data. But no source is authoritative on this, and as such the schema shouldn't use packet names to address packet data. The addressing scheme should be the inherently authoritative Direction->State->Packet ID.

That's what any sane encoder/decoder wants in order to implement the protocol. Packet names should be optional data

@roblabla
Copy link
Member

roblabla commented Oct 6, 2015

@nickelpro the problem with this is, take a look at what happened in 1.9 : every packet changed ID, because someone at mojang thought it'd be a fun idea to sort them by their internal, private, not-publicly-accessible names. Packet names and field names provide a small layer of abstraction that allows implementors to not have to worry about these details : we garantee that even if the ID changes, so long as the packet name is the same, the packet has the same semantic meaning across proto versions.

Also, you are free not to use the name fields, if they annoy you. They're already pretty much optional, if you think of it this way. fields are put in an array to keep order. We didn't do the same for packets in an attempt to keep the diff small, but if the community thinks we should transform the packets into lists instead of hashes, I guess we could do that.

@nickelpro
Copy link
Member

I'm not actually as committed to my ideas as I sound in Github comments. It's a tiny maintenance burden for a project to maintain name mappings if they want to. You're probably right about diff sizes as well

@rom1504
Copy link
Member Author

rom1504 commented Jan 7, 2016

@rom1504 rom1504 mentioned this issue Feb 15, 2016
@rom1504
Copy link
Member Author

rom1504 commented Mar 12, 2016

14:58 < hansihe> just thought i would nitpick, in minecraft-data, the play protocol state is just "play", while the handshake protocol state is "handshaking"

@hansihe
Copy link
Contributor

hansihe commented Mar 27, 2016

The naming is also inconsistent in movement packets.

entity_move_look and rel_entity_move are very closely related, and they both do relative movement, but the naming scheme is different. I would rename rel_entity_move to entity_move.

@hansihe
Copy link
Contributor

hansihe commented Mar 27, 2016

Reading through the discussion, I think it's mostly irrelevant what naming is used, as long as it is consistent across the board.

I would personally prefer if the naming scheme followed wiki.vg, as that is the resource that seems to be easiest to look up things on.

@rom1504
Copy link
Member Author

rom1504 commented Jun 13, 2022

Coming back to this :

Extremeheat has written a nice .yaml format that can be compiled down to our JSON. The comments can be extracted to produce html.

To do this for PC we could:

  • Convert json to yaml
  • Add doc from wiki.vg

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

No branches or pull requests

5 participants