Skip to content

Return an error when adding the same asset label multiple times. #19485

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

Open
wants to merge 6 commits into
base: main
Choose a base branch
from

Conversation

andriyDev
Copy link
Contributor

Objective

Solution

  • Make the labeled assets in LoadContext be a &Mutex<HashMap>.
  • Make LoadContexts for subassets reuse the same &Mutex<HashMap>.
  • Immediately-loaded nested assets will naturally get a separate Mutex<HashMap>, so their subassets will be managed separately.

Testing

  • Added tests to show the various cases working.

@andriyDev andriyDev force-pushed the duplicate-subassets branch from 32f14fd to b80279e Compare June 4, 2025 01:26
Copy link
Member

@alice-i-cecile alice-i-cecile left a comment

Choose a reason for hiding this comment

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

Helpful fix, but the Mutex makes me nervous. Can you see any other viable solutions?

@alice-i-cecile alice-i-cecile added C-Bug An unexpected or incorrect behavior A-Assets Load files from disk to use for things like images, models, and sounds X-Contentious There are nontrivial implications that should be thought through S-Needs-Review Needs reviewer attention (from anyone!) to move forward D-Modest A "normal" level of difficulty; suitable for simple features or challenging fixes labels Jun 4, 2025
@andriyDev
Copy link
Contributor Author

@alice-i-cecile Not really. The problem is that we need to be able to differentiate between immediate nested asset loads and their subassets, from "root asset" subassets. This was the thing I missed in #15481, and not something that's easy to fix without gross hacks. This solution has the advantage that as soon as you add a duplicate subasset, you'll get the feedback.

I don't think we need to worry much about the Mutex. It's got quite a small critical section, and likely has very low contention (people are probably not adding many subassets from many threads), plus asset loading in general is IO-bound.

@andriyDev andriyDev force-pushed the duplicate-subassets branch from b80279e to 544f418 Compare June 4, 2025 01:41
Copy link
Member

@alice-i-cecile alice-i-cecile left a comment

Choose a reason for hiding this comment

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

Awesome, thanks for the justification :)

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-Bug An unexpected or incorrect behavior D-Modest A "normal" level of difficulty; suitable for simple features or challenging fixes S-Needs-Review Needs reviewer attention (from anyone!) to move forward X-Contentious There are nontrivial implications that should be thought through
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Adding the same asset label multiple times to LoadContext does not return an error.
2 participants