gamenet: Add fields to SvGameMsg #90
Conversation
| {"name": ["para", "I"], "type": {"kind": "optional", "inner": {"kind": "int32"}}}, | ||
| {"name": ["para", "II"], "type": {"kind": "optional", "inner": {"kind": "int32"}}}, | ||
| {"name": ["para", "III"], "type": {"kind": "optional", "inner": {"kind": "int32"}}} | ||
| ], |
There was a problem hiding this comment.
Even this JSON is supposed to be generated by gamenet/generate/loader.py.
There was a problem hiding this comment.
You mean editing this line?
That would differ from upstream then 🤔
There was a problem hiding this comment.
No, editing this description in loader.py, and hopefully also pull-requesting something to upstream to fix this incorrect protocol description.
There was a problem hiding this comment.
It might have no arguments so not sure how incorrect it is.
There was a problem hiding this comment.
It's incorrect in the sense that it apparently doesn't do what you want it to do.
There was a problem hiding this comment.
Okay so this one should be added for sure NetIntRange("m_GameMsg", 'GAMEMSG_TEAM_SWAP', 'GAMEMSG_GAME_PAUSED')
But if the parameters are also added there then that would not always be correct. The teeworlds py protocol spec does not support optional types. So they just left it empty and made it optional in c++.
| _p.finish(warn); | ||
| result | ||
| } | ||
| pub fn encode<'d, 's>(&self, mut _p: Packer<'d, 's>) -> Result<&'d [u8], CapacityError> { | ||
| assert!(self.para_I.is_some()); | ||
| assert!(self.para_II.is_some()); | ||
| assert!(self.para_III.is_some()); |
There was a problem hiding this comment.
As you can see, the code does not allow for actually optional fields yet, only fields that can be omitted for backward compatibility.
Perhaps the optional code could be replaced by something that allows default values to be specified?
There was a problem hiding this comment.
I would consider sending some default like 0 instead of nothing wrong in that case. Those parameters are not always sent. Unused defaults seem misleading at best and break the protocol at worst. Doesn't rust have an optional type? How is the default supposed to be represented in the json?
There was a problem hiding this comment.
I would consider sending some default like 0 instead of nothing wrong in that case. Those parameters are not always sent. Unused defaults seem misleading at best and break the protocol at worst.
I tend to agree.
Doesn't rust have an optional type?
Yes, it does.
How is the default supposed to be represented in the json?
Currently, the JSON description does not support optional fields. It only supports fields that can be omitted for backwards compatibility. If we want to accurately support this 0.7 message, we'd first have to figure out a way to represent this in the JSON and then adapt all users of this JSON to deal with it.
There was a problem hiding this comment.
we'd first have to figure out a way to represent this in the JSON
Yes that is what I am asking about. I will not come up with the format if you have your ideas anyways. I have no good idea so that one is on you.
and then adapt all users of this JSON to deal with it.
By users do you mean humans with their libtw2 based projects? Or parts of the code in libtw2 that consume the json? Either way do you have a list of those users?
There was a problem hiding this comment.
Perhaps we could specify that this is a repetitions of i32s to the end of the message. That sounds kinda sane to me. Perhaps "kind":"array-until-end"?
Parts of libtw2 that consume the JSON, wireshark dissector and code generation for the gamenet crates, I think.
There was a problem hiding this comment.
Yes I like the idea sounds good. I sadly already spent way more time on this pr than initially planned. And it turned into something that got way bigger than I have motivation to work on.
I am closing this pr now so it does not get stale.
I would appreciate if one day you add support for the sv game msg to the dissector. Until then I will just use my branch that added this message because it works wonderfully for my use case which is debugging traffic including this very game message.
Correct.
Yea, that would be wrong. The JSON is supposed to declaratively describe the protocol.
I agree, I think it'd be better to develop protocols where fields don't depend on values. Makes it easier to write parsers. |
ChillerDragon
force-pushed
the
pr_gamemsg
branch
from
3e571a2
to
3732b11
Compare
|
By the way is this command hidden in some generate_all script? It is quite tedious to type. Shouldn't that be included in here? |
It's kinda intentionally omitted because it's not supposed to be run regularly. I agree that this makes a bad contributor experience. Not sure what to do about it. :( |
Para I-III are not i32 at all times. Some of them might be the team enum. But that depends on the gamemsg field and I do not think the current code supports such logic. I would love to add some if statements to the generated rust code but it seems wrong. Not sure how complex the generation code would be if it would support if statements or field relations. Also might look ugly in the json. So I went with the raw i32 for now.
If you need a refresher on what this message does I fully documented it here https://chillerdragon.github.io/teeworlds-protocol/07/game_messages.html#NETMSGTYPE_SV_GAMEMSG
Here is how it looks in action used by the wireshark dissector.
Before:
After: