{"action":"created","issue":{"url":"https://api.github.com/repos/cuberite/cuberite/issues/3380","repository_url":"https://api.github.com/repos/cuberite/cuberite","labels_url":"https://api.github.com/repos/cuberite/cuberite/issues/3380/labels{/name}","comments_url":"https://api.github.com/repos/cuberite/cuberite/issues/3380/comments","events_url":"https://api.github.com/repos/cuberite/cuberite/issues/3380/events","html_url":"https://github.com/cuberite/cuberite/pull/3380","id":179003542,"number":3380,"title":"Move entity metadata writing to the entities themselves (for 1.9+)","user":{"login":"Pokechu22","id":8334194,"avatar_url":"https://avatars.githubusercontent.com/u/8334194?v=3","gravatar_id":"","url":"https://api.github.com/users/Pokechu22","html_url":"https://github.com/Pokechu22","followers_url":"https://api.github.com/users/Pokechu22/followers","following_url":"https://api.github.com/users/Pokechu22/following{/other_user}","gists_url":"https://api.github.com/users/Pokechu22/gists{/gist_id}","starred_url":"https://api.github.com/users/Pokechu22/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/Pokechu22/subscriptions","organizations_url":"https://api.github.com/users/Pokechu22/orgs","repos_url":"https://api.github.com/users/Pokechu22/repos","events_url":"https://api.github.com/users/Pokechu22/events{/privacy}","received_events_url":"https://api.github.com/users/Pokechu22/received_events","type":"User","site_admin":false},"labels":[],"state":"open","locked":false,"assignee":null,"assignees":[],"milestone":null,"comments":23,"created_at":"2016-09-24T01:33:14Z","updated_at":"2016-10-14T16:49:36Z","closed_at":null,"pull_request":{"url":"https://api.github.com/repos/cuberite/cuberite/pulls/3380","html_url":"https://github.com/cuberite/cuberite/pull/3380","diff_url":"https://github.com/cuberite/cuberite/pull/3380.diff","patch_url":"https://github.com/cuberite/cuberite/pull/3380.patch"},"body":"**This is provided as an alternative way of implementing, but not necessarily the best one**.  [Here's a table of what I think the pros and cons of this method, the current method, and several similar ones](https://github.com/cuberite/cuberite/pull/3380#issuecomment-250043754).  If you have suggestions or ideas about which is best architecturally, post them!  (Even if it's \"the current method is better\").\r\n\r\n-----\r\n\r\nThis moves the logic to write [entity metadata](http://wiki.vg/Entities) from the protocol classes to entities themselves.  This allows for much cleaner code, and avoids copying all of the metadata writing code just to increment a number for all entities.\r\n\r\nThe core concept behind it is to move metadata serialization to a virtual method in `cEntity`, and to have each subclass write its own metadata after the that of its superclass.  It's possible to determine the protocol version being written for, meaning that fields that are added in newer versions are only be written in newer versions (and IDs are shifted accordingly).  For 1.10 there's only one such field that cuberite uses, which is the \"No Gravity\" field on the base entity class (but since it's on the base entity class, the addition of that field affects all entities).\r\n\r\nDoing it this way only makes sense because metadata is assigned incrementally.  In 1.8, metadata wasn't assigned incrementally, but it is in 1.9 and above.  Since it's assigned incrementally, if a field is added to a class low on the hierarchy, all of the fields after it have their ID incremented as well, creating lots of work if IDs are hardcoded.  Dynamically creating the IDs based off of what was written already is a much simpler system in terms of maintenance.\r\n\r\nThis new system was designed with [1.11's new metadata fields](http://wiki.vg/index.php?title=Pre-release_protocol&oldid=8118#Entity_Metadata) in mind and it should be easy to implement those when the time comes.  1.8 is not affected by this change because 1.8 uses somewhat random metadata assignment rather than incremental assignment."},"comment":{"url":"https://api.github.com/repos/cuberite/cuberite/issues/comments/253856666","html_url":"https://github.com/cuberite/cuberite/pull/3380#issuecomment-253856666","issue_url":"https://api.github.com/repos/cuberite/cuberite/issues/3380","id":253856666,"user":{"login":"Pokechu22","id":8334194,"avatar_url":"https://avatars.githubusercontent.com/u/8334194?v=3","gravatar_id":"","url":"https://api.github.com/users/Pokechu22","html_url":"https://github.com/Pokechu22","followers_url":"https://api.github.com/users/Pokechu22/followers","following_url":"https://api.github.com/users/Pokechu22/following{/other_user}","gists_url":"https://api.github.com/users/Pokechu22/gists{/gist_id}","starred_url":"https://api.github.com/users/Pokechu22/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/Pokechu22/subscriptions","organizations_url":"https://api.github.com/users/Pokechu22/orgs","repos_url":"https://api.github.com/users/Pokechu22/repos","events_url":"https://api.github.com/users/Pokechu22/events{/privacy}","received_events_url":"https://api.github.com/users/Pokechu22/received_events","type":"User","site_admin":false},"created_at":"2016-10-14T16:49:36Z","updated_at":"2016-10-14T16:49:36Z","body":"My first thought on seeing that: I like the syntax.\r\n\r\nI'm not sure how creating additional classes would solve the inheritance issue, though.  You couldn't have the writer for slimes and the writer for magma cubes be the same, because `cSlime::GetSize` and `cMagmaCube::GetSize` aren't the same method (unless templates let you work around that).  Basically, to get that to work you'd still want Cuberite's entity class hierarchy to match Minecraft's.  That doesn't mean having the additional classes would be bad, but it doesn't get us out of having to change the hierarchy.\r\n\r\nAnother thought: how would the individual classes handle different protocol versions?  Using the version number would probably be the only way (which isn't _really_ an issue, but it's still not ideal).  I already have code that depends on the version number, so that's probably fine.\r\n\r\nI'm curious why you have `int & counter` as a parameter in your sample.  The way that I've done it, the `EntityWriter` equivalent keeps track of the count, which seems \"cleaner\" to me - you'd only call `writer.WriteInt(value)` and not `writer.WriteInt(counter++, value)`.  Is there any specific reason you did it that way, or was it just part of the way you did the sample?\r\n\r\nI'll try and create a mock-up of this (probably based off of what I did with method 3).\r\n\r\n> I don't understand the switch problem. For me this code looks good\r\n\r\nHonestly I'm not sure why the switch bothers me so much.  It doesn't really seem like a problem when looked at objectively.  Something about it being nested just irks me.  I'll not worry about that for now, though.\r\n\r\n-----\r\n\r\nAlso, I just noticed this:\r\n\r\n> It's like adding a \"SendEntity\" method to cEntity.\r\n\r\nThere's `cEntity::SpawnOn(cClientHandle & a_Client)` (and `cMonster::SpawnOn(cClientHandle & a_Client)` and several other overrides).  It selects (effectively) which packet to use by calling `SendSpawnMob` or `SendSpawnObject` or whichever.  That is at a higher level than what I'm doing here, but it's the same general idea.\r\n\r\n-----\r\n\r\nWhat I'm now thinking about is 1.11.  1.11 adds a bunch of new entities, but that's easy to handle.  The part of 1.11 that is going to be difficult is the way they split a bunch of entities - where there was just `minecraft:zombie` and a `Type` metadata field, there is now `minecraft:zombie`, `minecraft:husk`, and `minecraft:zombie_villager` (and technichally also `minecraft:zombie_pigman`).  This is somewhat hard to deal with - each of those has a different ID used with spawn mob (while previously, they were the same ID).  And each one had didfferent metadata fields.  The same problem applies to horses: `minecraft:horse`, `minecraft:zombie_horse`, `minecraft:skeleton_horse`, `minecraft:donkey`, `minecraft:mule`, and `minecraft:llama`.  All of which share some metadata, but not a lot of it - `minecraft:horse` has fields that `minecraft:skeleton_horse` doesn't (armor and variant).  Achieving compatibility with multiple protocols that way would be a pain.  We probably should figure out how that will work - sending the right entity IDs would be difficult, and choosing the right metadata would be important."}}
{"actor":{"display_login":"Pokechu22"}}