search

projectharmonia / bevy_replicon

Server-authoritative networking crate for the Bevy game engine.
https://crates.io/crates/bevy_replicon
Apache License 2.0
350 stars 31 forks source link

Do not panic on invalid entity mapping inside events #311

Closed Shatur closed 2 months ago

Shatur commented 4 months ago

I used panic because I thought that it's impossible to happen 😅 But forgot to take visibility into account. It was reported that sending an event that contains an entity that isn't visible for a client causes crash. I think we should ignore this event and change the panic into a warning.

Shatur commented 3 months ago

There is a problem with this. EntityMapper doesn't provide a way to handle errors. I can panic or keep invalid entity in the component, but I can't stop the serialization: https://github.com/projectharmonia/bevy_replicon/blob/3d48f5a0300c27200c1e3014d62269ead1032715/src/core/ctx.rs#L90-L98

I think emitting a warning and keeping component in an invalid state is not a good idea...

UkoeHB commented 3 months ago

Maybe add try_map_entity method to EntityMapper that defaults to Ok(Self::map_entity(entity)).

Shatur commented 3 months ago

If EntityMapper have two methods, user will have too decide between them in their MapEntities impl. I.e. the component have to be aware about the mapping usage, which is not good...

UkoeHB commented 3 months ago

True. You could just replace EntityMapper with a new EntityMapperChecked trait.

Shatur commented 3 months ago

Users will need to implement both which is also not great... 🤔