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

Split hierarchy updates and transform propagation to two stages #1198

Closed
wants to merge 1 commit into from

Conversation

cyndis
Copy link

@cyndis cyndis commented Jan 3, 2021

With hierarchy updates and transform propagation in the same stage,
GlobalTransform may fail to be updated when an entity is added
without using one of the .with_children style functions and Transform
is not changed on further frames. This can happen when creating an
entity with Transform and Parent directly.

The reason for this is that the transform propagation pass relies
on the Children component being set on the parent, but as the
parent update system is run in the same stage, the component
is not yet visible. On the following frame, the Changed
constraint filters out the entity, and as a result GlobalTransform
never gets updated.

Fix this by moving parent_update_system to a new UPDATE_HIERARCHY
stage just after the UPDATE stage (and likewise for startup stages).
This ensures that all systems in POST_STARTUP and further can rely
on the hierarchy being set up.

With hierarchy updates and transform propagation in the same stage,
GlobalTransform may fail to be updated when an entity is added
without using one of the .with_children style functions and Transform
is not changed on further frames. This can happen when creating an
entity with Transform and Parent directly.

The reason for this is that the transform propagation pass relies
on the Children component being set on the parent, but as the
parent update system is run in the same stage, the component
is not yet visible. On the following frame, the Changed<Transform>
constraint filters out the entity, and as a result GlobalTransform
never gets updated.

Fix this by moving parent_update_system to a new UPDATE_HIERARCHY
stage just after the UPDATE stage (and likewise for startup stages).
This ensures that all systems in POST_STARTUP and further can rely
on the hierarchy being set up.
@Moxinilian Moxinilian added core C-Feature A new feature, making something new possible labels Jan 4, 2021
@cart
Copy link
Member

cart commented Jan 6, 2021

Hmm while this does appear to resolve the issues mentioned, I think I'll hold off on merging for now. I don't find the "run systems in their own stage to enforce ordering" particularly satisfying. Its probably the best way to handle it with the tools we have today, but we have a number of fixes in the works:

  • System sets and parallel executor v2 #1144 is a partial solve because it allows us to specify system dependencies, which means we can force hierarchy updates to run before transform updates (however its not a full solve because command buffers won't be applied between system executions)
  • I'm currently working on support for alternative ECS storages and a hierarchy rework, which im pretty sure will resolve this nicely (because we should be able to cheaply insert components stored in Sparse Sets immediately from within a system)

That being said, lets leave this open for now because it solves real problems. If the approaches above don't pan out before the next release, we can merge this instead.

@cyndis
Copy link
Author

cyndis commented Jan 7, 2021

Agreed, it's a hacky solution. Sounds good to me!

Comment on lines +43 to +47
.add_stage_after(
bevy_app::stage::UPDATE,
stage::UPDATE_HIERARCHY,
SystemStage::parallel(),
)
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
.add_stage_after(
bevy_app::stage::UPDATE,
stage::UPDATE_HIERARCHY,
SystemStage::parallel(),
)
.add_stage_before(
bevy_app::stage::POST_UPDATE,
stage::UPDATE_HIERARCHY,
SystemStage::parallel(),
)

I think this is a better placement for the stage.

@cyndis
Copy link
Author

cyndis commented Feb 28, 2021

This is now fixed in master through system ordering constraints -- closing.

@cyndis cyndis closed this Feb 28, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C-Feature A new feature, making something new possible
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants