-
-
Notifications
You must be signed in to change notification settings - Fork 4.2k
Share AssetSources between the AssetServer, the asset processor, and the asset processor's internal asset server.
#21763
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
base: main
Are you sure you want to change the base?
Conversation
…set server, and the regular asset server.
17e33b9 to
a5f1ca9
Compare
|
It looks like your PR is a breaking change, but you didn't provide a migration guide. Please review the instructions for writing migration guides, then expand or revise the content in the migration guides directory to reflect your changes. |
|
I added a migration guide, though I think in a future PR I'm just gonna make these private. I am very skeptical that users are initializing these manually and I think the fact that these are public is more a mistake than a desired feature. There would need to be a really compelling reason to support this and we should probably have a more appropriate "front door" if that's the case. |
kristoff3r
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, but I haven't tested it
| .map(|r| r()) | ||
| .map(Into::<Arc<_>>::into), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Shouldn't the AssetSourceBuilder functions generate Arcs instead of always converting the Box they make?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
They could do. I didn't want to change too much since it's not fully necessary, and also returning a box means we own the reader rather than being given a cloned arc they got from somewhere else. I'd say that's malicious, but it lines up!
In the future, we might refactor this even more since I think the builder API mostly only made sense with AssetSourceBuilders which I'm getting rid of in the next PR.
greeble-dev
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Clicking approve as I couldn't spot any issues and the tests pass for me. Only nitpick is that the PR's explanation for ungated_processed_reader should also be a comment in the code.
|
I included a short blurb about it. I initially didn't know where to put the comment, but I think a comment on the field itself is sufficient. It should prevent people from removing it at least without understanding the intent. |
Objective
AssetSourceswill be reflected in all the uses.Solution
watch_for_changesis false. If we don't do this, sharing the sources between the asset server and the processor-internal asset server will result in two tasks consuming asset events, so the regular asset server will miss asset events.One thing I'm starting in this PR is making things more private. For example, it's not clear why
ProcessorGatedReaderwaspub. I've also madeAssetSources::gate_on_processorno longerpub. This does mean that users can no longer initialize their ownAssetServerproperly (e.g., gated on the processor), but I don't think we should really support this - our focus should be on theAssetServerinitialized byAssetPlugin, not hypothetical uses where someone wants to insert their ownAssetServer.Testing