Using Protocols #162

rom1504 · 2015-05-14T17:36:32Z

#159 and #161 brought node-minecraft-protocol much closer of https://github.com/roblabla/Protocols

It would be nice to actually use Protocols.
In order to do that, what's missing is mainly figuring out what to put into Protocols's stream.js to handle all the various kind of parse* there is in protocol.js and client.js.

Once that's done it would be possible to simply adding Protocols as a dependency and use it.

EDIT by @roblabla :
Protocol improvements I can't live without :

string should specify a count/countType like buffer and Array. (edit by @rom1504 : that would be a cstring, which can be implemented in Protocols)
CountType in addition to count for array, buffer, and string
Context defaults : NMP uses the old this scheme. Instead, use super and root scheme
typeInfo : a top-level tag listing all the data-types used in the protocol file, optionally extending some. See Data Types ProtoDef-io/node-protodef#1
Error type enhancement : Keep track of which packet we're in, and which dataType caused the error. Contrary to the current protocol, this should be implemented in the separate container types (struct and array, mostly).

The text was updated successfully, but these errors were encountered:

rom1504 · 2015-05-23T13:33:59Z

Pretty close now !
Some things that might go into protocols :

packets.js (reading the .json file)
the transforms stream

They are almost generic so it wouldn't be too hard to put them into Protocols.

client.js is almost generic too but I don't think it really fits into Protocols, we probably need to do finish #176 first.

Got to think about better defining the scope of Protocols too.

roblabla · 2015-05-23T13:48:23Z

Todo list of changes for dataTypes :

Align string type so it looks like buffer (takes a count argument)
Add countType to array/buffer/string
Instead of using "this"-like syntax, use "super"-like syntax in container.
?

rom1504 · 2015-08-21T12:19:35Z

countType done

rom1504 · 2015-08-21T18:07:30Z

So we talked and switch cover all that condition does and it's more clean, so better replace condition by switch (see https://github.com/roblabla/Protocols/blob/master/lib/datatypes/conditional.js#L17 )

rom1504 · 2015-08-25T22:38:23Z

That's the kind of thing that should be fixed by the last point, this is caused by a write, but no idea which one :

[ERR]  TypeError: value is out of bounds
    at TypeError (<anonymous>)
    at checkInt (buffer.js:784:11)
    at Buffer.writeInt8 (buffer.js:892:5)
    at NMProtocols.writer (/media/documents/Documents/programmation/interlangage/minecraft/node-minecraft-server/node_modules/minecraft-protocol/dist/datatypes/numeric.js:28:25)
    at NMProtocols.write (/media/documents/Documents/programmation/interlangage/minecraft/node-minecraft-server/node_modules/minecraft-protocol/dist/protocol.js:42:18)
    at /media/documents/Documents/programmation/interlangage/minecraft/node-minecraft-server/node_modules/minecraft-protocol/dist/transforms/serializer.js:104:20
    at Array.forEach (native)
    at createPacketBuffer (/media/documents/Documents/programmation/interlangage/minecraft/node-minecraft-server/node_modules/minecraft-protocol/dist/transforms/serializer.js:100:10)
    at Serializer._transform (/media/documents/Documents/programmation/interlangage/minecraft/node-minecraft-server/node_modules/minecraft-protocol/dist/transforms/serializer.js:209:19)
    at Serializer.Transform._read (/media/documents/Documents/programmation/interlangage/minecraft/node-minecraft-server/node_modules/minecraft-protocol/node_modules/readable-stream/lib/_stream_transform.js:184:10)

rom1504 · 2015-08-26T19:54:32Z

Using longjohn is not much better :

[ERR]  TypeError: value is out of bounds
    at TypeError (<anonymous>)
    at checkInt (buffer.js:784:11)
    at Buffer.writeInt8 (buffer.js:892:5)
    at writer (/media/documents/Documents/programmation/interlangage/minecraft/craftyjs/node_modules/minecraft-protocol/dist/datatypes/numeric.js:28:25)
    at NMProtocols.write (/media/documents/Documents/programmation/interlangage/minecraft/craftyjs/node_modules/minecraft-protocol/dist/protocol.js:42:18)
    at /media/documents/Documents/programmation/interlangage/minecraft/craftyjs/node_modules/minecraft-protocol/dist/transforms/serializer.js:104:20
    at Array.forEach (native)
    at createPacketBuffer (/media/documents/Documents/programmation/interlangage/minecraft/craftyjs/node_modules/minecraft-protocol/dist/transforms/serializer.js:100:10)
    at Serializer._transform (/media/documents/Documents/programmation/interlangage/minecraft/craftyjs/node_modules/minecraft-protocol/dist/transforms/serializer.js:209:19)
    at Transform._read (/media/documents/Documents/programmation/interlangage/minecraft/craftyjs/node_modules/minecraft-protocol/node_modules/readable-stream/lib/_stream_transform.js:184:10)
---------------------------------------------
    at inject (/media/documents/Documents/programmation/interlangage/minecraft/craftyjs/lib/serverPlugins/log.js:9:8)
    at MCServer.connect (/media/documents/Documents/programmation/interlangage/minecraft/craftyjs/index.js:33:30)
    at Object.createMCServer (/media/documents/Documents/programmation/interlangage/minecraft/craftyjs/index.js:18:12)
    at Object.<anonymous> (/media/documents/Documents/programmation/interlangage/minecraft/craftyjs/app.js:12:10)
    at Module._compile (module.js:456:26)
    at Module._extensions..js (module.js:474:10)
    at Module.load (module.js:356:32)
    at Module._load (module.js:312:12)
    at Module.runMain (module.js:497:10)

line 9 of log.js :

serv.on("error",function(error){
    console.log('[ERR] ', error.stack);
    log('[ERR]: Server:', error.stack);
  });

with :

self._server.on('error', function(error) {
    self.emit('error',error);
  });

and self._server is just a nmp Server instance.

So basically longjohn tell us : "the error comes from the on("error")" , not quite useful.

(the problem is knowing which write this error is coming from)

rom1504 · 2015-08-27T14:26:45Z

That's the error when you try to write a packet with a packet name that doesn't exist, it should be way clearer :

16:19 < mhsjlw> /root/node-minecraft-server/node_modules/longjohn/dist/longjohn.js:194
16:19 < mhsjlw>         throw e;
16:19 < mhsjlw>               ^
16:19 < mhsjlw> TypeError: Cannot call method 'toString' of undefined
16:19 < mhsjlw>     at Client.write (/root/node-minecraft-server/node_modules/minecraft-protocol/dist/client.js:207:81)
16:19 < mhsjlw>     at attackEntity (/root/node-minecraft-server/lib/playerPlugins/pvp.js:10:20)
16:19 < mhsjlw>     at Client.<anonymous> (/root/node-minecraft-server/lib/playerPlugins/pvp.js:19:7)
16:19 < mhsjlw>     at EventEmitter.emit (events.js:95:17)
16:19 < mhsjlw>     at Deserializer.<anonymous> (/root/node-minecraft-server/node_modules/minecraft-protocol/dist/client.js:149:12)
16:19 < mhsjlw>     at EventEmitter.emit (events.js:95:17)
16:19 < mhsjlw>     at readableAddChunk (/root/node-minecraft-server/node_modules/minecraft-protocol/node_modules/readable-stream/lib/_stream_readable.js:195:16)
16:19 < mhsjlw>     at Readable.push (/root/node-minecraft-server/node_modules/minecraft-protocol/node_modules/readable-stream/lib/_stream_readable.js:162:10)
16:19 < mhsjlw> ---------------------------------------------
16:19 < mhsjlw>     at Client.on (/root/node-minecraft-server/node_modules/minecraft-protocol/dist/client.js:85:29)
16:19 < mhsjlw>     at inject (/root/node-minecraft-server/lib/playerPlugins/pvp.js:17:18)
16:19 < mhsjlw>     at Server.<anonymous> (/root/node-minecraft-server/index.js:52:32)
16:19 < mhsjlw>     at EventEmitter.emit (events.js:95:17)
16:19 < mhsjlw>     at loginClient (/root/node-minecraft-server/node_modules/minecraft-protocol/dist/index.js:242:14)
16:19 < mhsjlw>     at /root/node-minecraft-server/node_modules/minecraft-protocol/dist/index.js:218:11
16:19 < mhsjlw>     at /root/node-minecraft-server/node_modules/minecraft-protocol/dist/yggdrasil.js:78:9
16:19 < mhsjlw>     at Request.callback (/root/node-minecraft-server/node_modules/minecraft-protocol/node_modules/superagent/lib/node/index.js:575:3)

rom1504 · 2015-09-23T17:43:47Z

All changes we talked about here are done.
Remaining here : moving/updating code in Protocols and trying to make nmp use Protocols.

roblabla · 2015-09-23T18:36:43Z

string isn't done. cstring is different from string in that cstring is a value-delimited string, whereas string is a length-prefixed string. Not the same ;)

rom1504 · 2015-09-23T18:42:22Z

@roblabla so why should string have a count/countType ?

roblabla · 2015-09-23T19:28:59Z

@rom1504 because minecraft here is the special case, prefixing string with a varint. Not every protocol format do use a varint as the prefix count

rom1504 · 2015-09-23T19:30:38Z

ah right okay.

rom1504 · 2015-10-02T23:19:36Z

In order to use Protocols I propose to create a branch in Protocols (possibly in a fork) containing NMP protocol.js, datatypes (not minecraft.js), test/dataTypes and a branch/PR in nmp using that Protocols branch.
Once we have this working, we can easily compare to Protocols master and do changes to that "update" Protocols branch that appear interesting.
We will also be able to figure out a serializer for protocols once that basic thing works.

rom1504 · 2015-11-03T01:13:34Z

Okay I got myself needing "string should specify a count/countType like buffer and Array."
Because NBT string are short prefixed, not varint prefixed.

Not sure if we want to name the generalized string "string", because we might want to keep calling a string a string in protocol.json (by extending the type)

roblabla · 2015-11-03T13:25:34Z

TBH, I'm ok with renaming every string "mcstring" in protocol.json. But if you think that's annoying maybe we can figure something out.

roblabla · 2015-11-03T13:26:14Z

I wonder how much stuff would break by doing "string": ["string", { "countType": "varint" }] :P

rom1504 · 2015-11-03T13:35:17Z

@roblabla https://github.com/roblabla/ProtoDef/blob/master/src/protodef.js#L68 would break, we have a type map, not a type stack :p

I don't know if we should rename the generalized string (lengthPrefixedString or something shorter) or the one used in nmp.
We have a cstring so even lengthPrefixedString wouldn't cover all cases (~it wouldn't cover cstring), so we might as well rename the generalized string I think. (it doesn't necessarily make a lot of sense to keep it called "string")

lengthPrefixedString is way too long though.

https://en.wikipedia.org/wiki/String_(computer_science)#Length-prefixed

Apparently pstring might be a good name for it.

rom1504 · 2015-11-06T00:34:47Z

pstring is now implemented in ProtoDef.

Only thing remaining here is fixing the last issues of #274 and merging.

rom1504 · 2015-11-09T09:39:22Z

So this issue is done now :)

roblabla mentioned this issue May 22, 2015

Putting protocol.json into minecraft-data #163

Closed

roblabla mentioned this issue Aug 25, 2015

Protocol only works on major verions #65

Closed

rom1504 mentioned this issue Oct 2, 2015

[WIP] import from nmp ProtoDef-io/node-protodef#8

Merged

rom1504 closed this as completed Nov 9, 2015

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Using Protocols #162

Using Protocols #162

rom1504 commented May 14, 2015

rom1504 commented May 23, 2015

roblabla commented May 23, 2015

rom1504 commented Aug 21, 2015

rom1504 commented Aug 21, 2015

rom1504 commented Aug 25, 2015

rom1504 commented Aug 26, 2015

rom1504 commented Aug 27, 2015

rom1504 commented Sep 23, 2015

roblabla commented Sep 23, 2015

rom1504 commented Sep 23, 2015

roblabla commented Sep 23, 2015

rom1504 commented Sep 23, 2015

rom1504 commented Oct 2, 2015

rom1504 commented Nov 3, 2015

roblabla commented Nov 3, 2015

roblabla commented Nov 3, 2015

rom1504 commented Nov 3, 2015

rom1504 commented Nov 6, 2015

rom1504 commented Nov 9, 2015

Using Protocols #162

Using Protocols #162

Comments

rom1504 commented May 14, 2015

rom1504 commented May 23, 2015

roblabla commented May 23, 2015

rom1504 commented Aug 21, 2015

rom1504 commented Aug 21, 2015

rom1504 commented Aug 25, 2015

rom1504 commented Aug 26, 2015

rom1504 commented Aug 27, 2015

rom1504 commented Sep 23, 2015

roblabla commented Sep 23, 2015

rom1504 commented Sep 23, 2015

roblabla commented Sep 23, 2015

rom1504 commented Sep 23, 2015

rom1504 commented Oct 2, 2015

rom1504 commented Nov 3, 2015

roblabla commented Nov 3, 2015

roblabla commented Nov 3, 2015

rom1504 commented Nov 3, 2015

rom1504 commented Nov 6, 2015

rom1504 commented Nov 9, 2015