Make bevy_math optional in bevy_core#2900
Conversation
alice-i-cecile
left a comment
There was a problem hiding this comment.
Looks good! Any detangling of our dependency bush is a win IMO.
The code changes here are very simple.
inodentry
left a comment
There was a problem hiding this comment.
Yes, this seems like an easy win for bevy's modularity and usefulness in other projects, without significant drawback for use inside bevy. I like this. :)
|
This feels slightly "off" to me, probably because the context of |
|
I think my argument comes down to two main things:
My "goal" here is to use bevy_asset in my own project that isn't using bevy_math or anything above it in the dependency tree. It's sort of an arbitrary cutoff, but it's also the level I'm interested in working at. (Side note: you could even make an argument against I understand there's a need to at some point in the tree just say that external use without part of the crate just isn't a supported use case. I'm just trying to make a case to make If there's a better way to achieve this (e.g. invert the bevy_asset -> bevy_diagnostic dependency or make that dependency optional), I'm happy to do that instead. Or I'm honestly fine keeping this patch locally (what I'm playing with likely won't be a published crate). |
|
I think the core-ish conflict here is that The "problem" then comes from pulling one necessarily pulling the others. bevy_diagnostic only uses the time types, and bevy_asset even only optionally reports any diagnostics. My "issue" isn't that bevy_core assumes bevy_math is desirable, it's more that bevy_asset, which is quite individually useful, has a dep noticeably fatter tree than required for standalone use with bevy_app/bevy_ecs but not the whole of bevy. It's fine if this is a modularity border that the bevy project doesn't see value in setting -- after all, the difference is just cold compile times for downstream using bevy piecewise, since the unused code/crates will trivially optimize out -- but I personally think that this is a border worth putting at least a little bit of "as-is" support into, thus filing this PR. But, again, it's perfectly understandable if the bevy project doesn't want to carve this out this way. And if there is a better way that you'd like to provide bevy_asset for lighterweight use, I'd be happy to implement that approach as well. Trimmed, annotated dep tree of bevy_assetSymbol key: |
Notably, this makes bevy_core, bevy_diagnostic, and bevy_asset no longer require pulling in bevy_math. None of these crates directly depend on the math primitives, so this is a net positive for use outside of bevy, which can now use these crates without pulling in bevy_math and glam.
|
I do agree that this is worth discussing and I think you've zero-ed in on the core issue:
It is the "glue" that ties miscellaneous things without their own crates into bevy apps, often when the actual crates cannot do the registering themselves due to circular dependencies (ex: bevy_tasks is used in bevy_ecs, so we can't handle plugin stuff in bevy_tasks). I do think that it is worth revisiting bevy_core. I object to the current impl in this pr for the reasons I called out above, but I think we can probably find a solution that satisfies everyone:
In short, we might be able to do away with bevy_core entirely. I'd prefer to start doing this stuff after the new renderer is merged to avoid conflicts / allow us to remove the Bytes stuff entirely. These are all straightforward changes that won't take any real time investment. |
|
Filed #2931 to track the enhancement properly. |
Notably, this makes bevy_core, bevy_diagnostic, and bevy_asset no longer require pulling in bevy_math. None of these crates directly depend on the math primitives, so this is a net positive for use outside of bevy, which can now use these crates without pulling in bevy_math and glam.
Objective
I'm building my own micro-engine off of the bevy primitives for learning purposes, and as part of that, I'm using my own simplistic math library rather than using bevy_math/glam. (Basically, I would like to have everything up to but not including rendering and math.) I was looking at pulling in
bevy_assetto handle asset loading (an ugly task I have no interest in re-solving), but noticed it was pulling inbevy_mathindirectly.Cue surprise that
bevy_mathisn't actually used in any fashion, just to eagerly.register_typeinCorePlugin. Putting that functionality behindfeatures = ["bevy"](as is done inbevy_reflect) is a very minor change that enables me to usebevy_assetwithout needlessly pulling in glam.Solution
CorePlugin's registering ofbevy_mathtypes for reflection behind an optionalbevy_mathdependencybevyfeature tobevy_corewhich enablesbevy_mathandbevy_reflect'sbevyfeaturebevy_core'sbevyfeature viabevy_internalCorePlugin, which is thebevyend-user.