Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

PacketDistributor crashes when given a custom Level implementation #1669

Open
MehVahdJukaar opened this issue Nov 7, 2024 · 7 comments
Open
Labels
bug A bug or error

Comments

@MehVahdJukaar
Copy link

MehVahdJukaar commented Nov 7, 2024

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

@MehVahdJukaar MehVahdJukaar added the triage Needs triaging and confirmation label Nov 7, 2024
@AterAnimAvis
Copy link
Contributor

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?

@AterAnimAvis AterAnimAvis added the needs response Further information is requested label Nov 7, 2024
@MehVahdJukaar
Copy link
Author

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
Copy link
Contributor

AterAnimAvis commented Nov 7, 2024

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
Copy link
Author

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
Copy link
Contributor

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
Copy link
Contributor

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.
}

@Shadows-of-Fire Shadows-of-Fire added bug A bug or error and removed needs response Further information is requested triage Needs triaging and confirmation labels Nov 8, 2024
@MehVahdJukaar
Copy link
Author

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug A bug or error
Projects
None yet
Development

No branches or pull requests

3 participants