bevyengine
Update hashbrown to 0.15
Updating dependencies; adopted version of #15696. (Supercedes #15696.)
Long answer: hashbrown is no longer using ahash by default, meaning that we can't use the default-hasher methods with ahasher. So, we have to use the longer-winded versions instead. This takes the opportunity to also switch our default hasher as well, but without actually enabling the default-hasher feature for hashbrown, meaning that we'll be able to change our hasher more easily at the cost of all of these method calls being obnoxious forever.
One large change from 0.15 is that insert_unique_unchecked is now unsafe, and for cases where unsafe code was denied at the crate level, I replaced it with insert.
Migration Guide
bevy_utils has updated its version of hashbrown to 0.15 and now defaults to foldhash instead of ahash. This means that if you've hard-coded your hasher to bevy_utils::AHasher or separately used the ahash crate in your code, you may need to switch to foldhash to ensure that everything works like it does in Bevy.
@clarfonthey looks like there's a formatting issue somewhere, could you try running the formatter and push again?
Yeah, I was running into some difficulties fixing the lints but will try and fix this today or tomorrow.
Rereading the changelog, I misunderstood what hashbrown did and they actually switched to a better hash that we should probably use instead. Gonna probably find a way to factor that in.
This should be ready for review, modulo whatever tiny nitpicks CI throws at me.
Not sure if the migration guide is adequate: not sure how much we care about bevy_utils' public API, as far as like, whether people should be using it or not.
What is better about foldhash vs ahash? Is it really worth taking the extra dependency?
What is better about foldhash vs ahash? Is it really worth taking the extra dependency?
Note: it's not an extra dependency, just a different dependency. Since hashbrown is using it now, we'd actually be taking an extra dependency by continuing to use ahash.
But supposedly foldhash performs better than ahash, which seems pretty worth it.
I figure this will be a post-0.15 change, and so will likely have more merge conflicts going forward, but I'm going to try and at least get the current conflicts resolved. Seems to be taking a few tries.
(I fixed everything on my side, but an advisory issue is showing up that seems unrelated to this PR.)
Now that 0.15 is released, we should be able to merge this. I fixed the merge conflicts and reordered the imports using rustfmt for the files that were modified.
I also found a few files that were accidentally modified because they referenced some kind of HashMap but weren't actually affected by the changes, so, I pruned those from the PR so it's (hopefully) as small as it can be.
Rebasing to account for the new graph types in ECS: I think that once someone looks at that version, we should mark this as ready for final review, since I'd like to get this out before any more big PRs if possible.
(Apologies for re-requesting reviews from people: I figure that it's better to "dismiss" the old approvals whenever I make changes, but there's no expectation to review if you're busy.)
Since the PR @bushrat011899 mentioned has been merged now, I think it's probably okay to push this through soon. Will wait for CI before I bother people, though.
(Yes, I know that this comment counts as bothering people. Shh)
Also, it's passing now.
Final changes made:
- Update "ID is unique" comment to actually clarify why ID is unique
- Normalise
let x: HashMap<_, _> = HashMap::tolet x = <HashMap<_, _>>::and similarly forHashSet, to make things less verbose
(If someone could mark this as ready for final review, I'd appreciate it. I think this should qualify as ready.)
Ah, fun: the singular PR that got in the queue before this one didn't result in any merge conflicts, but it did add more uses of insert_unique_unchecked that now need unsafe blocks :(
Those have to be removed now, since bevy_reflect has deny(unsafe_code).
(Okay, this time I waited until CI passed. Should be ready to merge now. <3)
So, I had no idea about these patches that were being run in CI, but they seem pretty brittle-- it would be much better (IMHO) to use #[cfg] or cfg! to enable/disable these features than use patches, IMHO, so that at least they would be visible to people searching through the code. That said, I'll try and see what I can do.
I also have genuinely no idea why that patch failed on this PR. It appears to be referencing an earlier change where the patch should have broken, but it somehow did not trigger until now. And there were no recent changes to the CI job, either. It just makes absolutely no sense why this PR would be the one to trigger it, but at least, it did, and I fixed it.