Consider changing mcdata protocol.json instead of transforming it when reading it

[rom1504]

https://github.com/PrismarineJS/node-minecraft-protocol/blob/master/src/transforms/serializer.js#L21

At the very least, the "string" definition should move to protocol.json

[rom1504]

My first problem here is the collision between the position type and the position packet. It is currently solved by prefixing every packet type by packet_. But this is pretty ugly. A better solution would be either changing

1. the packet name : wiki.vg names it player_position but protolib does seem to be naming it position
2. the type name : that type is not exposed in any API, so we could change it. Maybe to pos.

I guess changing the type to pos is probably the best way.

[roblabla]

I think prefixing packets with packet_ isn't so bad. It certainly seems cleaner to my eyes, since it avoids the possibility of future collisions. NMP could have a thin layer above the protodef to remove the packet_ part of the event.

[rom1504]

It's not needed to remove the packet_ prefix because that prefix is only in the type name. nmp uses the mapper value (which doesn't have that prefix) to write/read packet names.

I'll try to transform the .json with the packet_ prefix to see how it looks.

[deathcap]

Would this mean all this code in src/transforms/serializer.js:

  Object.keys(packets).forEach(function(name) {
    proto.addType("packet_"+name,["container",packets[name].fields]);
  });

  proto.addType("packet",["container", [
    { "name": "name", "type":["mapper",{"type": "varint" ,
      "mappings":Object.keys(packets).reduce(function(acc,name){
        acc[parseInt(packets[name].id)]=name;
        return acc;
      },{})
    }]},
    { "name": "params", "type": ["switch", {
      "compareTo": "name",
      "fields": Object.keys(packets).reduce(function(acc,name){
        acc[name]="packet_"+name;
        return acc;
      },{})
    }]}
  ]]);

could be replaced with something like one call to proto.addTypes()?

[rom1504]

Yes!

That requires some drastic change to the .json though and I need to write a script to do it. Still planning on doing it. (Also got to check the resulting json looks okay)

[rom1504]

Oh I see you worked on that @deathcap , how far are you ?

I recently wrote https://gist.github.com/rom1504/58131c7876fb24ec2fc7 for node-raknet and it looks pretty good I think. (Only problem is the repeated "container" but I think that's okay, that even allow to have non-container packets which might be interesting)

So I think going that way is indeed a good idea.

[rom1504]

So experimenting with this in node raknet I found it is quite convenient, packet definition are just type definitions, so they can be reused if needed. The only real problem is that the mapping type cannot handle hex values. Would it be reasonable to add a "mapHex" option to it ?

[rom1504]

done