neoforged / NeoForge

Neo Modding API for Minecraft, based on Forge
https://neoforged.net
Other
1.24k stars 179 forks source link

PacketDistributor crashes when given a custom Level implementation #1669

Open MehVahdJukaar opened 2 weeks ago

MehVahdJukaar commented 2 weeks ago

Basically this.

Assumption is that a Level impl that does NOT return a ServerChunk cache (which would be hard to do for custom fake levels impls) IS a ClientLevel which is not the case. It should instead check if level is client side and just throw of that is true.

Image

Tested on neoforge 21.1.57

AterAnimAvis commented 2 weeks ago

It uses the ServerChunkCache to actually send the packet to the correct targets. The wording may not be completely accurate in the case of passing in a fake level but it is correct for normal usage.

So are you asking for the method to no-op on fake levels?

MehVahdJukaar commented 2 weeks ago

yes it should only throw if passed a non client side level. Right now it crashes for any level that does not return a ServerChunkCache

AterAnimAvis commented 2 weeks ago

Calling the method with a Level that doesn't contain a ServerChunkCache is incorrect, and something you shouldn't be doing, we prehaps could update the wording of the exception, but removing it (or limiting it to solely client side levels) sounds like rather a counter-intuitive thing to do.

MehVahdJukaar commented 2 weeks ago

I am not directly doing it. I am passing a fake level around and something (from mods) is eventually calling a send packet after they check level.isClientSide which is completely reasonable. What is wrong here is the assumption that a level that does not returns a ServerChunkCache is a client level as my post says. Method should no op there instead. A server level WILL always return a ServerChunkCache but the opposite is not true

Shadows-of-Fire commented 2 weeks ago

Could you show your relevant custom Level implementation for this case? Does it not have access to the server to supply the ServerChunkCache? (since it sounds like it sets isClient to false).

Shadows-of-Fire commented 2 weeks ago

Based on the current context, I think the following logic is more appropriate, but I'm intrigued by this custom server level implementation which apparently also has a custom chunk source implementation behind it.

public static void sendToPlayersTrackingEntity(Entity entity, CustomPacketPayload payload, CustomPacketPayload... payloads) {
    if (entity.level().isClientSide) {
        throw new IllegalStateException("Cannot send clientbound payloads on the client");
    } else if (entity.level().getChunkSource() instanceof ServerChunkCache chunkCache) {
        chunkCache.broadcast(entity, makeClientboundPacket(payload, payloads));
    }
    // Silently ignore custom Level implementations which may not return ServerChunkCache.
}
MehVahdJukaar commented 2 weeks ago

This is my level impl. Just returns an empty chunk source https://github.com/MehVahdJukaar/Moonlight/blob/1.20/common/src/main/java/net/mehvahdjukaar/moonlight/core/misc/FakeLevel.java