Track entity generation in ComponentEvent (fixes #720) #724
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Checklist
API changes
ComponentEvent
now contains anEntity
instead of anIndex
Join::get
works withEntity
s nowJoinIter::get_unchecked
has been removed, since there is no way to obtain generation from a pure functionFlaggedAccessMut
,OccupiedEntry
,VacantEntry
, andPairedStorage
all store anEntity
instead of anIndex
UnprotectedStorage
s now work withEntity
s instead ofIndex
esObviously, this feature requires many far-reaching changes across many different components of Specs in order to provide the required information all the way down to the level where ComponentEvents are generated and consumed. This is definitely a breaking change in so many different areas and it's unlikely it would be accepted even without the core problems that prevent it from being fully implemented as-is.
Help needed
There are three remaining compiler errors that I need to fix:
Perhaps those are a sign that this PR is impossible. Perhaps it's a sign that I need someone more skilled than myself to fix the issue.
ChangeSet
. It iterates through its bitmask and removes every component found inside. The issue is that while it has a bitmask of IDs it does not know the generations for each ID. This will require some deeper modifications to fix.next
method ofJoinIter
. This iterator not only needs to know the IDs it's iterating over but also the generations. This is complicated to implement because the iterator itself is created from only the bitmask and storage from an implementor ofJoin
, which would mean that all implementors ofJoin
would have to store generations in addition to the bitmask, which would probably lead to high memory usage and/or inefficiency. Because of the complexity of this issue it's possible that it will prevent this PR from ever being implemented since it would hurt performance and efficiency.map
function onBitIter<J::Mask>
is providing an index but no generation, and that's really all we need to know. It's iterating over a bitmask again with no generation information. We see that theBitProducer
itself that contains theBitIter
is stored inside aJoinProducer
, which is created using theBitProducer
as a component, which means that theBitProducer
is what needs to provide generations. Well, theBitProducer
is first created inJoinParIter::drive_unindexed
which once again produces theBitProducer
using only an implementor ofJoin
, which brings us back to the exact same issue as the second compiler error. This makes me sad and it makes the compiler sad too and it also probably makes @Imberflur sad.These issues are so complex and involved that it's entirely possible that the architecture of Specs completely prohibits this feature from being implemented effectively unless every
Join
stored generation information. And no, we can't just not changeJoin
s to work withEntity
s because theJoin
is how you accessFlaggedStorage
s and that's where theFlaggedStorage
gets information for constructingComponentEvent
s.