Skip to content

Conversation

@Strikeless
Copy link

Objective

This PR adopts what's left of #14162, implementing VisitAssetDependencies for HashMap. This was implemented for HashSet's in #20735.
This accomplishes the ability to mark Handle<T>/UntypedHandle values of a hashmap as dependencies of a struct deriving Asset using #[dependency] on the hashmap field.

Solution

impl VisitAssetDependencies for HashMap

Testing

  • Verified to build using cargo build at bevy root
  • verified to not crash in use on a cherry-pick to 0.17.3 release

I believe there is no need for more testing, given the simplicity of the change and the similarity of the two added impl blocks to previous ones.

@github-actions
Copy link
Contributor

Welcome, new contributor!

Please make sure you've read our contributing guide and we look forward to reviewing your pull request shortly ✨

@andriyDev andriyDev added D-Trivial Nice and easy! A great choice to get started with Bevy A-Assets Load files from disk to use for things like images, models, and sounds S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it C-Refinement Improves output quality, without fixing a clear bug or adding new functionality. and removed S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it labels Dec 20, 2025
Copy link
Contributor

@andriyDev andriyDev left a comment

Choose a reason for hiding this comment

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

Hmmm I'm a little nervous to do this, because it means that keys of a hashmap won't be included in VisitAssetDependencies. I feel like forcing all uses of HashMap to manually impl VisitAssetDependencies is better than accidentally doing the wrong thing if someone has HashMap<Handle<A>, Handle<B>>.

@andriyDev andriyDev added the S-Needs-Design This issue requires design work to think about how it would best be accomplished label Dec 20, 2025
@Strikeless
Copy link
Author

Fair concern I hadn't thought about. I feel like manually implementing VisitAssetDependencies for concrete hashmap types is a bit clunky still, although for sure better than messing up a HashMap<Handle<A>, Handle<B>>.
Due to orphan rules you'd need to wrap hashmap in a newtype if you wanted to impl VisitAssetDependencies with a key/value type external to Bevy. I wonder if there's a practical solution to this, or if I should close the PR and stick with newtyping and manual impl?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

A-Assets Load files from disk to use for things like images, models, and sounds C-Refinement Improves output quality, without fixing a clear bug or adding new functionality. D-Trivial Nice and easy! A great choice to get started with Bevy S-Needs-Design This issue requires design work to think about how it would best be accomplished

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants