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

Fix duplicated chilren in Scene spawn #904

Merged
merged 4 commits into from
Nov 22, 2020

Conversation

lassade
Copy link
Contributor

@lassade lassade commented Nov 21, 2020

Fixes #889,

TLDR;

The Problem

  1. A entity is been added twice in Children component while spawning a scene
  2. Performance takes a massive hit, transform_propagate_system took 13ms in release with a small skeleton tree ~20 bones
  3. Affects gltf loading
  4. Right now the only "correct" way of creating a Scene is using WorldBuilder.with(Parent(...)) (still wrong for other reasons)

The Fix

  1. Impl FromResources and MapEntities for PrevousParent
  2. Checking for duplicates in the Children using a debug_assert
  3. Children.0 and PreviousParent.0 are now pub(crate), their contents are a reflect of Parent

Conclusion
This is the best short term fix, it will make so that the only true correct way of creating Scene will be WorldBuilder.with_children which has no caviats

@Moxinilian Moxinilian added C-Bug An unexpected or incorrect behavior A-ECS Entities, components, systems, and events C-Performance A change motivated by improving speed, memory usage or compile times labels Nov 21, 2020
@lassade
Copy link
Contributor Author

lassade commented Nov 21, 2020

@Moxinilian the CI error is very strange can you force a re-run to see if fixes?

@Moxinilian
Copy link
Member

This is an issue with all CI currently, I don't have permission to do anything about it. I'll make sure cart is aware.

Copy link
Member

@cart cart left a comment

Choose a reason for hiding this comment

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

I think this is a reasonable short term fix. I made one small change (see comment below)

}
}

impl DerefMut for Children {
Copy link
Member

Choose a reason for hiding this comment

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

we need some way to re-order children. order already matters for things like UI, and once we have an editor it will matter for "scene organization".

I added this to enable those scenarios:

children.swap(a_index, b_index);

@cart cart merged commit c1e499d into bevyengine:master Nov 22, 2020
@fopsdev fopsdev mentioned this pull request Jan 24, 2021
@lassade lassade deleted the fix_dup_children branch April 5, 2021 22:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-ECS Entities, components, systems, and events C-Bug An unexpected or incorrect behavior C-Performance A change motivated by improving speed, memory usage or compile times
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Children is been added twice
3 participants