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

Push clientside transform GridUids to children #5588

Closed
wants to merge 1 commit into from

Conversation

Tayrtahn
Copy link
Member

@Tayrtahn Tayrtahn commented Jan 6, 2025

When an entity is parented to another entity that does not currently exist on the client (such as when the new parent is outside of the client's PVS), the child entity does not have its GridUid set properly. This can cause issues with other systems that rely on the GridUid.

For example, when using the Follow button from the ghost roles panel, if the target entity is outside of PVS, the camera will be left at a strange rotation until the user moves:
Screenshot 2025-01-06 at 4 37 29 PM

This will also happen when parenting to a newly-created entity (see space-wizards/space-station-14#33878 (comment))

To fix this, a handler for ComponentStartup is added to the clientside TransformSystem, which calls SetGridId on any children of the parent. SetGridId handles recursing through children-of-children and aborting in the case of uninitialized transforms.

@ElectroJr ElectroJr self-assigned this Jan 7, 2025
@ElectroJr
Copy link
Member

ElectroJr commented Jan 8, 2025

While it does fix the specific issue with ghost following, I think this might still cause unexpected problems elsewhere. Entity/component initialization is a convoluted mess, and many components blindly expect things like TransformComponent to have already been properly set up by the time they do their startup, which might be before the transform starts up and the fix that this PR adds is applied.
Edit: I forgot that transform is given special treatment and is actually meant to always start first, but either way its probably still better to fix it some other way

I think a better / more general fix would be to change the entity state application so that it makes sure that you never have an initialized entity getting attached to a pre-init entity. I.e., the client side entity creation logic should be more like the map-loading logic, where parents are always initialized & started before their children. I'm going to start working on a PR that tries to do that instead.

@Tayrtahn
Copy link
Member Author

Tayrtahn commented Jan 8, 2025

Superseded by #5595

@Tayrtahn Tayrtahn closed this Jan 8, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants