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

xilem_masonry: Robust generational indexing for ViewSequence #220

Merged
merged 8 commits into from
Apr 26, 2024

Conversation

DJMcNab
Copy link
Member

@DJMcNab DJMcNab commented Apr 26, 2024

Alternative to #218

The version of viewsequence added in #205 is robust for all properties supported in that version. However, it would be incompatible with future expansions to the Xilem model, in particular async wiring.

In this PR, I propose a system to resolve this, which uses generational indexes in the view id path (for the items where this is relevant).

Some other historical record: #9

This brings back the View::State associated type (renamed to ViewState and SeqState depending on the trait), in order to perform this wiring.

@DJMcNab DJMcNab marked this pull request as ready for review April 26, 2024 14:34
@DJMcNab DJMcNab requested a review from Philipp-M April 26, 2024 14:49
Copy link
Contributor

@Philipp-M Philipp-M left a comment

Choose a reason for hiding this comment

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

Looks pretty good, I think this indeed should be robust for async updates. I like that we got rid of the linear search in Vec<impl ViewSequence> (and likely other similar containers in the future).

Comment on lines +265 to +274
// The known solution mentioned in the above message is to use a different ViewId for the index and the generation
// We believe this to be superfluous for the default use case, as even with 1000 rebuilds a second, each adding
// to the same array, this would take 50 days of the application running continuously.
// See also https://github.com/bevyengine/bevy/pull/9907, where they warn in their equivalent case
// Note that we have a slightly different strategy to Bevy, where we use a global generation
// This theoretically allows some of the memory in `seq_state` to be reclaimed, at the cost of making overflow
// more likely here. Note that we don't actually reclaim this memory at the moment.

// We use 0 to wrap around. It would require extremely unfortunate timing to get an async event
// with the correct generation exactly u32::MAX generations late, so wrapping is the best option
Copy link
Contributor

Choose a reason for hiding this comment

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

Great writeup, and I agree, the probability that this happens per accident, is likely in the ballpark of a cryptographic hash collision... Not sure if this could be a security issue with a targeted attack, but I don't think we care at this point...

}

fn count(&self) -> usize {
self.0.count()
}
}

const BASE_ID: NonZeroU64 = match NonZeroU64::new(1) {
Copy link
Member Author

Choose a reason for hiding this comment

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

For future reference, TIL about NonZeroU64::MIN

It's been a long time since I have had a `fmt` CI check fail
@DJMcNab
Copy link
Member Author

DJMcNab commented Apr 26, 2024

Merging to avoid merge conflicts over the weekend. I'm pretty happy with this now, and I think the main infrastructure work on xilem_masonry is done.
One remaining task is AnyViewSequence, which is extremely analogous to AnyView, but otherwise it's improvements to the set of widgets and to Masonry.

(Also on the agenda is working out how to handle flex items - the idiomatic way is a specific View/ViewSequence trait, but that requires a huge amount of boilerplate)

@DJMcNab DJMcNab enabled auto-merge April 26, 2024 19:53
@DJMcNab DJMcNab added this pull request to the merge queue Apr 26, 2024
Merged via the queue into linebender:main with commit 6d1dd97 Apr 26, 2024
7 checks passed
@DJMcNab DJMcNab deleted the view_state branch April 26, 2024 20:01
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.

2 participants