Align bevy_transform's feature flags#23919
Conversation
633274e to
2125100
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. |
| # bevy | ||
| bevy_app = { path = "../bevy_app", version = "0.19.0-dev", default-features = false, optional = true } | ||
| bevy_ecs = { path = "../bevy_ecs", version = "0.19.0-dev", default-features = false, optional = true } | ||
| bevy_log = { path = "../bevy_log", version = "0.19.0-dev", default-features = false, optional = true } |
There was a problem hiding this comment.
Can you say more about why it was helpful to remove this dependency in this PR? Does it not have no_std support?
There was a problem hiding this comment.
The trace feature in bevy_transform is intentionally aligned with bevy_ecs, since bevy_transform is a sub-crate that should follow the parent crate's conventions:
bevy/crates/bevy_ecs/Cargo.toml
Line 39 in 64fe756
bevy/crates/bevy_ecs/Cargo.toml
Line 124 in 64fe756
bevy_transform uses exactly the same pattern. The feature name, dependency declaration, and enable condition are identical.
bevy_log is a user-facing subscriber configuration crate — pulling it in for a single tracing::warn! adds unnecessary weight to a mid-graph crate that should stay minimal.
alice-i-cecile
left a comment
There was a problem hiding this comment.
Very clean, but needs a migration guide. Feature flags are a pain to try and diagnose during upgrades.
I'd also like a better understanding of the bevy_log situation.
This comment was marked as off-topic.
This comment was marked as off-topic.
|
I don't think performance concerns are very relevant here. Did you see the comment from the bot above about the migration guide? We need to explain how users can migrate their feature flag setup. |
Co-authored-by: Kevin Chen <chen.kevin.f@gmail.com>
Objective
bevy_transform's multi-threading behavior was previously gated onfeature = "std", which incorrectly conflated standard library availability with multi-threading capability. This means:no_stdbut had multi-threading available lost parallelism unintentionally.stdbut wanted single-threaded execution (e.g. WASM) still attempted to use the parallel path.bevy_internal'smulti_threadedfeature did not propagate down tobevy_transform, so the parallel implementation was not activated when expected.bevy_logdependency was pulled in unconditionally via thestdfeature, when the only use was optional trace-level warnings.Solution
multi_threadedfeature tobevy_transform, backed bybevy_tasks/multi_threaded, matching the pattern used bybevy_ecsand other crates.#[cfg(feature = "std")]/#[cfg(not(feature = "std"))]guards on parallel/serial code paths with#[cfg(all(not(target_arch = "wasm32"), feature = "multi_threaded"))]/#[cfg(any(target_arch = "wasm32", not(feature = "multi_threaded")))]— the same guard pattern already used inbevy_ecs.bevy_transform/multi_threadedtobevy_internal's andbevy_render'smulti_threadedfeature lists so parallel transforms are activated end-to-end when building Bevy withmulti_threaded.bevy_logdependency frombevy_transform. Its sole usage was awarn_once!insidepropagate_transforms_for. This is replaced with a directtracing::warn!wrapped inbevy_utils::once!, gated on a new opt-intracefeature (dep:tracing). This keeps the default build lighter.tracefeature spans inmark_dirty_treesthat previously depended onbevy_log's re-exportedtracing, now usingtracingdirectly under#[cfg(feature = "trace")].Testing
bevy_transform::systems::testpass unchanged — no behavioral changes, only cfg guard corrections.