RobustToolbox icon indicating copy to clipboard operation
RobustToolbox copied to clipboard

Appearance ignores data that was set on HandleComponentState

Open Macoron opened this issue 3 years ago • 2 comments

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.

Macoron avatar Sep 12 '22 20:09 Macoron

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.

ElectroJr avatar Sep 13 '22 01:09 ElectroJr

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.

Macoron avatar Sep 13 '22 09:09 Macoron