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

Remove startup traversal entirely #5365

Open
wants to merge 6 commits into
base: master
Choose a base branch
from

Conversation

n00kii
Copy link

@n00kii n00kii commented Aug 17, 2024

when TransformComponent starts up, it runs an initial grid traversal check. however, you can't flip the GridTraversal bool of the transform component before it's already started when you spawn an entity. therefore, i add an argument (defaulting to true) to set the GridTraversal boolean as soon as possible, before the transform component is started up.

i've only added it to SpawnAttachedTo because intuitively, if you use this function you're expecting whatever you attach to whatever to STAY attached, and not silently grid-traverse detach; so now you have the option.

now, traversal no longer subscribes to TransformStartupEvent, so you can modify traversal as you wish before it actually happens

Comment on lines 76 to 81
public virtual EntityUid SpawnAttachedTo(string? protoName, EntityCoordinates coordinates, ComponentRegistry? overrides = null, Angle rotation = default, bool traversal = true)
{
if (!coordinates.IsValid(this))
throw new InvalidOperationException($"Tried to spawn entity {protoName} on invalid coordinates {coordinates}.");

var entity = CreateEntityUninitialized(protoName, coordinates, overrides, rotation);
var entity = CreateEntityUninitialized(protoName, coordinates, overrides, rotation, traversal);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Considering we're using overloads for different functionality it would be better for traversal to just not run.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

do you mean to disable startup traversal completely? wouldn't that be a breaking change?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes, what's wrong with breaking changes?

I'd rather just rip off a bandaid instead of making the already slow entity spawning even slower.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

sg, was just confirming

@n00kii n00kii changed the title Add a codepath for using SpawnAttachedTo bypassing initial traversal Remove startup traversal entirely Aug 25, 2024

private void OnStartup(ref TransformStartupEvent ev)
{
CheckTraverse(ev.Entity);
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is going to be a massively breaking change and the corresponding spawn code needs to be adjusted to handle this.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

so if im understanding what each usecase for the different spawn functions are, here's how this should look like:

(1) base Spawn: spawn in nullspace
(2) Spawn with MapCoordinates overload: spawn on map, reparent to grid if overlap
(3) SpawnAttachedTo: spawn with parent and position provided by EntitiyCoordinates
(4) SpawnAtPosition: same as (2) but transforms provided EntityCoordinates to MapCoordinates first
(5) TrySpawnInContainer, SpawnInContainerOrDrop, TrySpawnNextTo, SpawnNextToOrDrop: as their names imply

ignoring (1) since trivial case, and (5) since those functions are built up from (1..4). (2) and (4) just need to make sure they check for overlapping grids and reparent if one is found. is it fine to do have it implemented with the changes i've made to the MapCoordinates overload of CreateEntityUninitialized? (3) uses the EntityCoordinates overload of CreateEntityUninitialized which doesn't check for grids so no need to worry about traversing in that case

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

but now im confused because this is just making traversal implicitly built into certain spawn functions rather than relying on the system for it; is that what we want?

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