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

Generic hierarchy propagation #5507

Closed

Conversation

mockersf
Copy link
Member

Objective

  • There are two different hierarchy propagation for now: Transform/GlobalTransform and Visibility/ComputedVisibility
  • They slightly differ 😱

Solution

  • Make a generic system that do both
  • That can also be used by users if they want to add their own hierarchy propagation without handling all the details

@mockersf mockersf added C-Code-Quality A section of code that is hard to understand or change A-Hierarchy labels Jul 31, 2022
@nicopap nicopap self-requested a review July 31, 2022 11:33
@DJMcNab
Copy link
Member

DJMcNab commented Jul 31, 2022

Can you explain the differences between this approach and #4216 ?

Copy link
Contributor

@nicopap nicopap left a comment

Choose a reason for hiding this comment

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

Very excited about this. Though I think we still need more things (that can be added in the future):

  • Independent hierarchies (I want to be able to declare two distinct hierarchies that spans the same entities)
  • Ability to tolerate missing ComputedGlobal::Local with a default and propagating downward to other entities with a ComputedGlobal. (people wanted to reduce the amount of "required" components I think)

@mockersf
Copy link
Member Author

Can you explain the differences between this approach and #4216 ?

Well the biggest difference is that I hadn't seen it before...
Other than that, that PR is very similar.

Copy link
Contributor

@superdump superdump left a comment

Choose a reason for hiding this comment

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

I like this. Have you investigated/thought through whether it will work for RenderLayers too? I suppose Local would be RenderLayers, ToPropagate would be RenderLayers, the from_local would just be a clone, combine_with_local would be an intersection(?), and value_to_propagate would be *self? Seems pretty intuitive. And nice that it's reusable for any hierarchy someone might want to add.

@superdump
Copy link
Contributor

Indeed it looks pretty similar to #4216 , but it not requiring the App extension thing seems like an improvement.

Co-authored-by: Robert Swain <robert.swain@gmail.com>
@bas-ie
Copy link
Contributor

bas-ie commented Oct 4, 2024

Backlog cleanup: given that ComputedVisibility was already split back in 0.12 I'm guessing this one is not quite so relevant anymore. I'm sure @mockersf will correct me if I'm wrong 🙂

@bas-ie bas-ie closed this Oct 4, 2024
@alice-i-cecile alice-i-cecile added A-ECS Entities, components, systems, and events and removed A-Hierarchy labels Jan 28, 2025
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-Code-Quality A section of code that is hard to understand or change
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants