PaperMC / Waterfall

BungeeCord fork that aims to improve performance and stability.
https://papermc.io
MIT License
737 stars 295 forks source link

Railcraft crash #216

Closed LasmGratel closed 5 years ago

LasmGratel commented 6 years ago

Waterfall doesn't support custom meta entity which is defined in modding server.

For example, railcraft used custom entity meta id '15' and it would make Waterfall server crash.

7dfb421e6dc922f1

This issue is duplicated in https://github.com/HellFirePvP/AstralSorcery/issues/547 https://github.com/SpigotMC/BungeeCord/issues/2349

But they are not getting solved by the authors.

I am looking forward to the official supporting of Waterfall.

n0099 commented 6 years ago

more info: This issue could reproduce when a player placing railcraft locomotive or some type of railcraft carts (like forestry beecart), and all players within that locomotive's visual range chunks will be kicked to the fallback server or just kick out of the server by proxy, then these players are unable to rejoin the server (players get kicked again and again) until the locomotive be killed by commands.

Several mods using custom entity meta type 15 for some particular purpose too.

Black-Hole commented 6 years ago

You might want to fork this project and implement those metadata for the mods you're using. Entity meta types above 14 could be anything.

MarkL4YG commented 6 years ago

Why would forge allow breaking the protocol though... 🤔 seems useless if that breaks compatibility with proxies. Are you sure there's no easy way to allow this in BungeeCord/Waterfall?
I don't know the protocols internals but this really sounds like not being that big of a deal...

Black-Hole commented 6 years ago

Entity meta data don't have a length field. The length is determined by the type. So where does type 15 stops: http://wiki.vg/Entity_metadata#Entity_Metadata_Format

electronicboy commented 6 years ago

Basically, bungeecord is required to do some modification to entity data sent to the client in order to support teleporting across servers properly, they need to rewrite some information which is sent to the client; Now, in order to do this (because of the usage of varints), they need to rewrite all of the metadata on the entity, which we can't do for an unknown metadata type, because we have no idea how to process this.

If this type is defined by forge, we could probably support it, but for stuff added directly by mods, you're out of luck.

MarkL4YG commented 6 years ago

@Black-Hole Ah okay now I see what the issue with that is.... Thanks.
But I still feel like such a thing should not happen ... not saying there's someone at fault here but not being allowed to use any of the remaining possible IDs sounds just wrong.

electronicboy commented 6 years ago

It's not the usage of IDs that's the issue, it's a, how do you expect a system to manipulate data which it doesn't know the format of. This mechanism is a core part of what allows cross server teleportation to occur without having invalid/corrupt entities due to transporting across servers, and due to how the protocol works, requires the full packet contents to be rewritten, which we cannot do properly if we don't know the format of the data we're working with.

On Tue, Mar 13, 2018, 4:59 PM Magnus Leßmann (aka. mark332) < notifications@github.com> wrote:

@Black-Hole https://github.com/black-hole Ah okay now I see what the issue with that is.... But I still feel like such a thing should not happen ... not saying there's someone at fault here but not being allowed to use any of the remaining possible IDs sounds just wrong.

— You are receiving this because you commented.

Reply to this email directly, view it on GitHub https://github.com/WaterfallMC/Waterfall/issues/216#issuecomment-372739953, or mute the thread https://github.com/notifications/unsubscribe-auth/ABLAZHodmwpE7jnlhQ4raGQ9GfybmSTzks5td_rmgaJpZM4Sor44 .

MarkL4YG commented 6 years ago

@electronicboy Yeah I get that. But this still sounds like being unfriendly to modifications. I personally consider the need to know all entity meta types a protocol flaw. Anyways - as I said I don't want to blame or anything. I'm just giving my opinion on this issue.

LasmGratel commented 6 years ago

@electronicboy Thus I have to fork Railcraft and get this entity normalized and being friendly with Waterfall? Instead, I also have no idea with CovertJaguar's thoughts...

MarkL4YG commented 6 years ago

Nah I think they ment you'd need to fork Waterfall/Bungee and add this entity-meta-type to the mappings.

jamierocks commented 6 years ago

Indeed that is what @electronicboy meant. Bungeecord has to process entity meta data to ensure that players can teleport across servers without buggering up data. To process the meta data, Bungeecord (and intrinsically Waterfall) needs to know how to read it. Sadly this means that any meta data introduced by mods will not be compatible with Bungeecord/Waterfall - without modifications.

Mystiflow commented 6 years ago

Pending BungeeCord issue 2361 has speculation about it if entity rewriting can be removed (see md-5 referenced commit)

LasmGratel commented 6 years ago

I just comment the throw new IllegalArgumentException( "Unknown meta type " + type ); code part in EntityMap and 0x3C case in rewriteClientbound of EntityMap_1_12(_1)

Compiled binaries available below:

entitymap.zip

Repack them into net.md_5.bungee.entitymap package in BungeeCord or Waterfall.

stale[bot] commented 5 years ago

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

LasmGratel commented 5 years ago

This issue still relevant.

electronicboy commented 5 years ago

The fix here is ultimately going to be to disable entity remapping entirely and rely upon the clients logic of disposing of entities in the world upon switching world, which would require a fair chunk of testing on... If somebody wants to put in the work for this and put it behind a configurable option, I'd be more than happy to review it, but given 1.13s release and forge reworking all of their tooling, I personally cannot personally justify the work to put into this with proper testing.

stale[bot] commented 5 years ago

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

electronicboy commented 5 years ago

Closing in favor of #136