Skip to content

Make Changed, Mutated and Added work on Queries#625

Closed
DJMcNab wants to merge 4 commits intobevyengine:masterfrom
DJMcNab:nest-single
Closed

Make Changed, Mutated and Added work on Queries#625
DJMcNab wants to merge 4 commits intobevyengine:masterfrom
DJMcNab:nest-single

Conversation

@DJMcNab
Copy link
Member

@DJMcNab DJMcNab commented Oct 4, 2020

This allows you to use Changed<&mut T>, for example.

In theory, Changed, Added and Mutated could be abstracted over
but the savings in typing is probably counteracted by the
amount of added complexity.

@memoryruins memoryruins added the A-ECS Entities, components, systems, and events label Oct 5, 2020
@cart
Copy link
Member

cart commented Oct 8, 2020

I dig it! I think this is probably the right call / the general pattern we should use for single-component Query pointers. I'll do a full review soon.

@DJMcNab
Copy link
Member Author

DJMcNab commented Oct 8, 2020

I was also thinking about make a Not<Q> one which inverts the most recent level of should_skip, but I'm not sure if that would work with the current invariants
And it might be worth forcing them to be Deref to avoid having to double deref

I haven't done this for resources because I'm planning on unifying the resource query system with the component one (systems/local resources map one to one to entities/components with a global entity for global resources)

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 like these changes in general, but I do want to address the extra hashing this method introduces before merging. Not<Q> is definitely interesting, but I'd rather do that in a later PR because it does require some thought to make sure it won't break anything.

)
})
Some(Self(
<Q::Fetch as Fetch<'a>>::get(archetype, offset)?,
Copy link
Member

Choose a reason for hiding this comment

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

lets eliminate the multiple get_type_state calls here to avoid doing redundant (and relatively expensive) work. Its worth calling out that even after resolving the redundant lookup here, this new approach forces us to do an extra hash for these pointer types (one hash for the Fetch::get() call and one hash for retrieving modified/added state).

We might be able to hack around this by making SingleTypeFetch expose a method that returns the Fetch result and the type state. Its a little cumbersome, but for hot-spot code like this i think its worth doing.

Copy link
Member Author

Choose a reason for hiding this comment

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

That makes sense - it also should avoid the extra derefs if we use SingleTypeFetch::Whatever

I'll probably look into this over the weekend

Also make the type parameters have better names
@DJMcNab DJMcNab reopened this Oct 24, 2020
@DJMcNab
Copy link
Member Author

DJMcNab commented Oct 24, 2020

@cart I think this is done again, if CI likes it

@cart cart mentioned this pull request Nov 10, 2020
@DJMcNab
Copy link
Member Author

DJMcNab commented Nov 11, 2020

Superceded by #834.

@DJMcNab DJMcNab closed this Nov 11, 2020
@DJMcNab DJMcNab deleted the nest-single branch February 19, 2021 20:55
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

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants