Anders429 / brood

A fast and flexible entity component system library.
Apache License 2.0
39 stars 1 forks source link

Entry access does not work in `System`s #191

Closed Anders429 closed 1 year ago

Anders429 commented 1 year ago

The problem is trait bounds related, I believe. Basically, any entry query done on can't be proven to be valid, because the EntryViews aren't proven to be contained in the Registry.

I don't have time to do a full investigation on a fix right now, but I will address it as soon as I have time.

Anders429 commented 1 year ago

So at a high level, the issue is that query::Entry::query() takes a SubViews generic parameter that must be contained within some other generic Registry. In normal queries, this SubViews can always be shown to be contained within the Registry, because the Registry is known. But in the case of a System or ParSystem, the Registry is passed to run() as a generic parameter. We can't prove that it contains SubViews for any valid SubViews.

I think there may be a way to remove the trait dependency on Registry from the query::Entry::query() method. The SubSet trait can be rewritten to instead index directly into the superset, rather than using the canonical form. Additionally, the querying itself can be done through the SubSet trait by just querying with the superset and then indexing into that result with the SubSet trait.

If we remove this dependency on the Registry, then the trait bounds can again be specified for any generic Registry and the full EntryViews, and it will be sufficient for all sub-queries. The last thing to do will be to specify an additional trait bound on System::run() to ensure Registry: ContainsViews<Self::EntryViews<'a>>.

Due to the changing of System's interface, this will be a major breaking change.

Anders429 commented 1 year ago

For viewing with sub-views through super-views, I think the best way to do it would be to view the row as a MaybeUninit<Views> type, which can be converted to a (MaybeUninit<View>, MaybeUninit<Views>) type for each view. This can then be converted to the sub-view safely after checking the bit in the archetype::Identifier.

Anders429 commented 1 year ago

The above comment may not actually work. Layout of types containing MaybeUninit are not guaranteed, unfortunately: https://doc.rust-lang.org/stable/std/mem/union.MaybeUninit.html#layout-1

Currently looking for another way to do this.

Anders429 commented 1 year ago

Rather than attempting to transmute tuples of different types, which is unsound, I've found I can instead define another associated type for Views for MaybeUninit views on components. This provides more advantages, because I only have to wrap the hard, non-optional views in MaybeUninit, instead of wrapping everything. That means there are less unsafe conversions to deal with later.

Anders429 commented 1 year ago

This is fixed and will be a part of the 0.8.0 release.