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

(sys_transform) Propagate update to children of dirty transforms #51

Merged
merged 5 commits into from
Oct 20, 2020

Conversation

stasm
Copy link
Member

@stasm stasm commented Oct 15, 2020

In #44 (comment) @ooflorent pointed out that re-parenting was inherently dangerous in Goodluck. If the new hierarchy resulted in children before parents in world.Signature, then the order of transform updates would not account for parents when updating the children, resulting in the children's World matrices being wrong.

@ooflorent suggested filtering transforms to identify those without parent (i.e. top-level entities), and then propagating transform updates from those top-level entities down to children, regardless of the order in world.Signature.

This led me to a little revelation. As far as I understand it, top-level entities are not enough: sometimes we want to update a child's transform even if the parent has been static this frame. For instance, an entity rotating on a pivot must have its transform updated every frame, but the pivot entity can skip the update of its transform.

Interestingly, Goodluck already has something that we could use to identify entry points – the dirty flag! It's set manually by the developer on entities at any level of the hierarchy to mean that this entity and all its children must update. In the current sys_transform the dirty flag is propagated out-of-order recursively down the hierarchy starting from the transform marked as dirty by the developer. The matrix math then happens in-order of world.Signature, relying on the fact that in a properly instantiated world, all child transforms will have their Dirty flag set to true by the parent.

In this PR I'm proposing a change to this logic. Rather than propagate just the Dirty flag, let's propagate the matrix math too.

@stasm stasm merged commit 6490f44 into master Oct 20, 2020
@stasm stasm deleted the propagate-transform-update branch October 20, 2020 22:07
@planetis-m planetis-m mentioned this pull request Oct 22, 2020
@@ -8,42 +8,38 @@ const QUERY = Has.Transform;
export function sys_transform(game: Game, delta: number) {
for (let i = 0; i < game.World.Signature.length; i++) {
if ((game.World.Signature[i] & QUERY) === QUERY) {
update(game, i);
let transform = game.World.Transform[i];

Choose a reason for hiding this comment

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

You need to fetch every transform component to check the dirty flag. Whereas adding a dirty signature would avoid this.

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks for bringing this up; I should have written this down somewhere and you question gives me an opportunity to do it.

Like I mentioned in #34, I do think a separate Has.Dirty signature is a more pure approach and in fact I thought back to your implementation when I was writing this code. I considered it here but ultimately decided against it for two reasons.

  1. I didn't want to inflate the number of components.
  2. I slightly prefer the developer experience of transform.Dirty, mostly for its convenience.

For (2), compare the following snippets which move the entity 1 unit to the left:

transform.Position[0] += 1;
transform.Dirty = true
transform.Position[0] += 1;
world.Signature[entity] |= Has.Dirty;

Like I mentioned in #34, to me transform.Dirty is a more pragmatic choice, even if it might be slightly less performant.

Copy link

@planetis-m planetis-m Oct 22, 2020

Choose a reason for hiding this comment

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

You could implement a bitset using an array that way you wouldn't have any restrictions. 🤷

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.

None yet

2 participants