Skip to content

Conversation

andriyDev
Copy link
Contributor

@andriyDev andriyDev commented Oct 9, 2025

Objective

  • There was a TODO to make this type actually a trait.
  • Add a few basic tests for asset processing. #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

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.

@andriyDev andriyDev added C-Feature A new feature, making something new possible A-Assets Load files from disk to use for things like images, models, and sounds 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 labels Oct 9, 2025
@andriyDev andriyDev force-pushed the trait-log branch 2 times, most recently from afd6ef1 to 3edc5d3 Compare October 9, 2025 06:39
@andriyDev andriyDev force-pushed the trait-log branch 2 times, most recently from c7fcf4f to db9e588 Compare October 9, 2025 07:01
@andriyDev andriyDev added this to the 0.18 milestone Oct 9, 2025
}
}

/// Sets the transaction log factory prior to processing being started.
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
/// 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.

Copy link
Contributor Author

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.

@janis-bhm
Copy link
Contributor

Is there a way to pass the factory via AssetPlugin and not have the setter at all?

@andriyDev
Copy link
Contributor Author

Is there a way to pass the factory via AssetPlugin and not have the setter at all?

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 Plugin::build is given an immutable borrow to the plugin, so we can't easily take a box for the factory. We'd need to have something like processor_transaction_log_factory: Mutex<Option<Box<dyn ProcessorTransactionLogFactory>>>, which is an awful type to work with.

@andriyDev andriyDev requested a review from janis-bhm October 10, 2025 00:13
@janis-bhm
Copy link
Contributor

We'd need to have something like processor_transaction_log_factory: Mutex<Option<Box<dyn ProcessorTransactionLogFactory>>>, which is an awful type to work with.

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.

@alice-i-cecile alice-i-cecile added this pull request to the merge queue Oct 10, 2025
@alice-i-cecile alice-i-cecile added S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it and removed S-Needs-Review Needs reviewer attention (from anyone!) to move forward labels Oct 10, 2025
Merged via the queue into bevyengine:main with commit ea8f22e Oct 10, 2025
38 checks passed
@andriyDev andriyDev deleted the trait-log branch October 10, 2025 23:11
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-Feature A new feature, making something new possible D-Modest A "normal" level of difficulty; suitable for simple features or challenging fixes S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants