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

Add mutated state when inserting an already existing component #404

Merged
merged 4 commits into from
Nov 5, 2020
Merged

Add mutated state when inserting an already existing component #404

merged 4 commits into from
Nov 5, 2020

Conversation

dallenng
Copy link
Contributor

Fixes #333

Proposed Change

When calling Archetype::put_dynamic with added = false, the mutated state will be set to true. Because if a component is not added, that means it is updated so it should be marked as mutated.
I added tests to check if the behaviour is correct based on what I think the code should do, feel free to correct me if I'm wrong.

One case is not handled correctly in this PR, but I'll need a bit of guidance if this behaviour is wanted. When the entity contains A and you insert A and B, A is updated and B is added. So, A state should be mutated and B state should be added, but both state are added.

@karroffel karroffel added A-ECS Entities, components, systems, and events C-Feature A new feature, making something new possible C-Bug An unexpected or incorrect behavior labels Aug 31, 2020
@cart
Copy link
Member

cart commented Sep 2, 2020

Isn't this behavior already captured by the Changed<T> pointer? I suppose theres an argument to be made that we could merge Changed<T> and Mutated<T> pointers. What we lose is the ability to run logic only when an existing item has been mutated.

@dallenng
Copy link
Contributor Author

dallenng commented Sep 2, 2020

Changed<T> is just a logical OR between Added<T> and Mutated<T> but the similarity in the name of Changed<T> and Mutated<T> can easily bring confusion if you're not careful. So, yes this behavior is captured by Changed<T>, but it is for the "wrong" reason : triggered by Added<T> and not Mutated<T>.

Here's a little diagram of what's happening (assuming the commands are run sequentially on the same entity and the trackers are cleared between each command) :

Command Entity State in master State in PR Expected state
spawn(A(0)) (A(0)) Added<A> Added<A> Added<A>
insert(A(1)) (A(1)) None Mutated<A> Mutated<A>
insert((A(0), B)) (A(0), B) (Added<A>, Added<B>) (Added<A>, Added<B>) (Mutated<A>, Added<B>)

@dallenng dallenng closed this Sep 22, 2020
@dallenng dallenng reopened this Sep 22, 2020
@dallenng dallenng marked this pull request as ready for review September 22, 2020 17:22
@dallenng
Copy link
Contributor Author

dallenng commented Sep 26, 2020

As far as I can see, I fixed change tracking on Commands::insert and this PR is ready to merge unless I have to add an entry to the changelog or something else.

@@ -400,6 +400,8 @@ impl Archetype {
let state = self.state.get_mut(&ty).unwrap();
if added {
state.added_entities[index] = true;
} else {
Copy link
Member

Choose a reason for hiding this comment

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

Hey sorry for the delay here. These changes make sense to me. I think this works as-is, but I'm a little worried that the implicit !added == mutated here could cause problems / create corner case issues. I'd rather just pass in a mutated flag and set it explicitly each time.

Copy link
Member

Choose a reason for hiding this comment

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

Im specifically worried about cases where we change archetypes and some of the components aren't mutated.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok, I did the changes you suggested and I think it's better like this because further call of the method will need to be explicit on the state.

Rename some variables and methods names to avoid confusion between Changed and Mutated query filter.
check states are correctly set when inserting components.
@cart
Copy link
Member

cart commented Nov 5, 2020

Sorry for dragging my feat on this one. This looks good to go. Thanks for sticking with this / improving our correctness!

@cart cart merged commit 5bd6deb into bevyengine:master Nov 5, 2020
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-Feature A new feature, making something new possible
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Mutating a component with insert(_one) doesn't trigger Mutated query filter
3 participants