-
-
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 - There was a TODO to make this type actually a trait. - bevyengine#21409 introduced a test flake on WIndows (since the transaction log file may not have been fully released by the time the next test ran), and made tests create the `crates/bevy_asset/imported_assets/log` file. ## Solution - Create two traits: `ProcessorTransactionLogFactory` and `ProcessorTransactionLog`. The former creates the latter. - Rewrite the test helper for asset processing to use a fake transaction log that doesn't do anything. - Note: pretty much the entire implementation was just reformatted into the trait. - Note: these transaction log methods are returning `BevyError` instead of something like `std::io::Error`. ## Testing - I ran the asset tests on repeat for a while (including with bevyengine#21433 where the flake was first seen and I was able to reproduce fairly quickly). No issues! 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.
Objective
crates/bevy_asset/imported_assets/logfile.Solution
ProcessorTransactionLogFactoryandProcessorTransactionLog. The former creates the latter.BevyErrorinstead 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.