search

space-wizards / RobustToolbox

Robust multiplayer game engine, used by Space Station 14
https://spacestation14.io
Other
531 stars 393 forks source link

Appearance ignores data that was set on HandleComponentState #3248

Open Macoron opened 1 year ago

Macoron commented 1 year ago

Was introduced by #3233. Read https://github.com/space-wizards/space-station-14/issues/11132 for more context.

Though usage of appearance component like this is rare, I would recommend either fix it or add some assert/documentation.

ElectroJr commented 1 year ago

The reason for this change was because the appearance component is also itself a networked component, so any changes to the component state while applying a state is either redundant, or incorrect & will result in a corrupted entity state. The only reason it was initially working with a networked appearance component itself due to a bug that prevented the client from properly resetting prediction (fixed in #3252).

3252 also adds support for appearance components that have had their syncing disabled, though as mentioned there I don't really like that given that any component that relies on non-networked appearance data will be incompatible with many other components, which might cause unexpected issues later on.

Adding a debug assert is probably going to be a huge PITA, because some (probably many) components/systems set appearance data as a side effect of state application.

Docs wise, I think the most up to date appearance docs are this and this. The former briefly mentions that SetState should be used from server-side code, which could be expanded on, but is also maybe out of scope for that doc.

Macoron commented 1 year ago

https://github.com/space-wizards/RobustToolbox/pull/3252 also adds support for appearance components that have had their syncing disabled, though as mentioned there I don't really like that given that any component that relies on non-networked appearance data will be incompatible with many other components, which might cause unexpected issues later on.

I will probably agree with that. The good example is sprite component, where netsync changes could caused a lot of really annoying and hard to trace bugs. Consider most appearance visualizers are netsynced, this could cause a lot of problems.

Adding a debug assert is probably going to be a huge PITA, because some (probably many) components/systems set appearance data as a side effect of state application.

Do you mean when they set appearance data in shared system? Sure, some code would need to be updated, but tbh I would prefer to do that. Because right now it doesn't give you any feedback that your changes will be ignored.