-
-
Notifications
You must be signed in to change notification settings - Fork 4.2k
Convert ProcessorTransactionLog
into a trait.
#21476
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
Conversation
afd6ef1
to
3edc5d3
Compare
c7fcf4f
to
db9e588
Compare
} | ||
} | ||
|
||
/// Sets the transaction log factory prior to processing being started. |
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.
/// Sets the transaction log factory prior to processing being started. | |
/// Replaces the default transaction log factory. If this is called after asset processing has begun in the `Startup` schedule, it will fail and return `SetTransactionLogFactoryError::AlreadyInUse`. |
just a nit, but I think this could be clearer.
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.
I expanded this with your inspiration. I don't want to say "replaces the default", since if you call this multiple times it'll be replacing your transaction log. I did however clarify not calling it will use the default log. I also don't want to be specific about what error you get, since there may be other errors, but I highlighted the startup schedule like you suggested.
Is there a way to pass the factory via |
I initially didn't want to do this because it means putting a lot of processor specific config in the AssetPlugin, but I decided to give it a shot. The problem is that |
Yeah that doesn't sound great. My hope was that the ambiguity of when the factory can be set could be turned into more of a type-thing rather than being a docs-thing, if that makes sense. |
Objective
crates/bevy_asset/imported_assets/log
file.Solution
ProcessorTransactionLogFactory
andProcessorTransactionLog
. The former creates the latter.BevyError
instead of something likestd::io::Error
.Testing
Note: we could make a release notes for the fact that users can make their own transaction logs, but this feature is primarily for unit testing. Maybe a user could make a more robust transaction log, but it's really not clear that it would be important for anyone.