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

Heritable trait based property inheritance systems #4216

Open
wants to merge 5 commits into
base: main
Choose a base branch
from

Conversation

james7132
Copy link
Member

@james7132 james7132 commented Mar 15, 2022

Objective

First/naive attempt to generalize transform_propagate_system for any inheritable component.

Solution

  • Add a Heritable trait that defines the inheritance behavior across a hierarchy. GlobalTransform implements this trait.
  • Generalize transform_propagate_system using Heritable as generic systems.
  • Add extension trait for App for inserting these systems into the schedule.
  • Add dedicated system labels for scheduling.

This implementation includes the optimizations found in #4180.


There are a few drawbacks to this naive approach (originally detailed here: #4213 (reply in thread))

There are a few things to note with this initial set of assumptions if we are to generalize the existing transform_propagate_system.

  • Root queries are no longer sufficient unless we allow skipping levels. transform_propagate_system relies on every Entity from the root to the leaf to have the inheritable components.
  • Allowing skipping levels requires full hierarchy traversals for all inheritance systems, not just the hierarchies with the components present, which would negatively impact performance.
  • Top down inheritance is the only mode supported. Bottoms-up composition (i.e. AABBs as a BVH) isn't well supported. This could be supported via a different trait if need be.

Followup Work

Implement this for Visibility and ComputedVisibility. Will likely need to work with existing systems for computed visibility in some way.

@james7132 james7132 added the A-Transform Translations, rotations and scales label Mar 15, 2022
@github-actions github-actions bot added the S-Needs-Triage This issue needs to be labelled label Mar 15, 2022
@james7132 james7132 added C-Enhancement A new feature and removed S-Needs-Triage This issue needs to be labelled labels Mar 15, 2022
@james7132 james7132 marked this pull request as ready for review March 16, 2022 19:05
@superdump
Copy link
Contributor

‘Heritable’ is an odd word to me. I would have expected ‘Inheritable’. However, I feel like ‘Hierarchical’ or ‘Hierarchic’ might be a better fit semantically. Hmm.

@james7132
Copy link
Member Author

‘Heritable’ is an odd word to me. I would have expected ‘Inheritable’. However, I feel like ‘Hierarchical’ or ‘Hierarchic’ might be a better fit semantically. Hmm.

Heritable

  • (of a characteristic) transmissible from parent to offspring.

Seems pretty apt to me.

@ickk
Copy link
Member

ickk commented Mar 16, 2022

I'll throw in 'Hereditary' for good measure 😂

Heritable and inheritable are synonyms, a bit like flammable and inflammable. Personally I would err on the side of 'inheritable' because it's far more common.

@alice-i-cecile alice-i-cecile added the A-Hierarchy Parent-child entity hierarchies label Apr 4, 2022
@alice-i-cecile alice-i-cecile added the X-Controversial There is active debate or serious implications around merging this PR label Apr 25, 2022
@Weibye Weibye added the S-Adopt-Me The original PR author has no intent to complete this work. Pick me up! label Aug 10, 2022
@nicopap nicopap mentioned this pull request Apr 21, 2023
1 task
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-Hierarchy Parent-child entity hierarchies A-Transform Translations, rotations and scales C-Enhancement A new feature S-Adopt-Me The original PR author has no intent to complete this work. Pick me up! X-Controversial There is active debate or serious implications around merging this PR
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants