bevyengine / bevy

A refreshingly simple data-driven game engine built in Rust
https://bevyengine.org
Apache License 2.0
36.42k stars 3.59k forks source link

Should bevy-transform be two crates? #2758

Closed finegeometer closed 2 years ago

finegeometer commented 3 years ago

I wrote this issue back in May, but seem to have forgotten to submit it?

Issue writeup below. I apologize if the details turn out to be somewhat out of date.


I'm sorry, none of the issue templates seemed to fit. "Feature Request" was the closest, but it didn't quite seem right.

Question

In README.md, you state that one of your design goals is to be modular; a user can choose to use only those features they need. Yet the crate bevy_transform seems to bundle two unrelated concerns:

Its description in Cargo.toml even admits this — "Provides hierarchy and transform functionality for Bevy Engine"! (Emphasis mine.)

Why aren't these two separate crates, with the latter depending on the former?

Reason

I discovered this issue while exploring the possibility of using Bevy to make games that take place in curved space. The concept of a parent-child hierarchy is still completely applicable. But Transform, at least as currently implemented in Bevy, is not.

Details of Code Splitting

To ensure that this crate splitting would actually work, I tried it in a clone of the repository. Here are the results.

The crate bevy_transform was split into two: bevy_hierarchy and bevy_transform.

bevy_hierarchy contains these things:

It has dependencies bevy_ecs, bevy_reflect, bevy_utils, and smallvec. It does not need bevy_app or bevy_math.

bevy-transform contains everything else:

It depends on the new bevy-hierarchy, as well as bevy_app, bevy_ecs, bevy_math, and bevy_reflect. It does not need bevy_utils or smallvec.

A few changes were necessary to get bevy_transform to compile again:

Finally, the dependent crates needed to be adjusted.

cargo --test passes. I have not done any performance testing.

MinerSebas commented 3 years ago

You already posted this in #2187, but there wasnt realy any discussion.

finegeometer commented 3 years ago

Oh, that's why I couldn't find it. I didn't think to check the discussion section. Sorry!

Should this be closed, or is it useful to have it here as well?

LegNeato commented 3 years ago

It sounds like you already have a fix...why not put up a PR?

finegeometer commented 3 years ago

It sounds like you already have a fix...why not put up a PR?

Because there's too much I don't know, and I'm afraid of doing something wrong.

  1. I've never written a PR before, so I'm not familiar with how to do it.
  2. While I know the necessary code changes, I don't know what other necessary changes surround them. Docs changes? A version number change? Something else I'm not thinking of?
  3. I don't really know how to use Git.

In conclusion, I'm afraid to open a PR because I feel like I have no clue what I'm doing. I'd be willing to try, but I'm afraid I'll make a mistake.

mockersf commented 3 years ago

If you want to try your hands on it, there would probably be someone available to mentor you through your first PR 👍 . Don't hesitate to ask on Discord (or here if that's better for you). I'm slow to answer nowadays but you can ping me for that. There are extensive contributing guidelines for things more specific to how Bevy work too.

Though, before starting on the PR to split transforms / hierarchy, it would probably be better to ask @cart to chime in on his point of view.

cart commented 3 years ago

I agree that the split could happen. I think its worth discussing what we "win" by splitting them out though. I expect compile times to be pretty much unaffected and in some ways splitting them out makes things harder (when both parents and transforms are needed we now need to pull in two crates instead of one). Given that most Bevy features will generally pull in both transform and hierarchy types, in most cases they will all be in the tree anyway (and they definitely will be when the bevy crate is consumed directly). @finegeometer, can you describe your specific scenario a bit more / what you want your crate tree to look like?

Note that guidelines like "split it if it can be split out" and "dont split it unless you have a good reason to", taken to their extremes are both undesirable. The former results in a giant mesh of tiny crates that is difficult to navigate. The latter results in monolithic / unmodular crates. Its all a weird "art" based on taste :)

finegeometer commented 3 years ago

My motivating use case is games that take place in curved space¹ ².

What is Transform? In my view, Transform represents the collection of ways you can move an object without distorting it³. Mathematically, it is SE(3), the group of transformations of space that do not change the distances between points, and are not mirror-image transformations.

Notice that this definition depends on the shape of space. So for curved space, Transform needs to be implemented differently. Instead of SE(3), it would need to be a group like SO(4) or SO(3,1).

I'm happy to reimplement the relevant Transforms for curved spaces in my own projects. SO(4) has a nice representation as a pair of quaternions...

But implementing my own Transform would require implementing my own version of transform_propagate_system⁴, which depends on the hierarchy types, which are in bevy_transform. It just seems wrong for a crate implementing Transform in curved space to depend on one that implements it in flat space!


There's also an aesthetic argument for the split. To me, the concept of a logical hierarchy of entities seems almost entirely unrelated to the concept of a symmetry of Euclidean space.

Imagine the reverse scenario, where bevy_transform and bevy_hierarchy had always been separate. If for some reason, you had to combine bevy_transform with another crate, is bevy_hierarchy even the one you'd choose?


My particular use case could also be accommodated by reverting Transform to be a 4x4 matrix, because all of the groups I've mentioned are subgroups of the group of 4x4 matrices. But I don't like this option, for a few reasons.


¹ Also known as non-Euclidean space, though that term has often been misused to mean flat space with portals. ² For a good example of such a game, look up HyperRogue. ³ For simplicity, I will ignore the fact that Transform has a scale field. ⁴ Off topic: I wonder if there's a useful generalization of transform_propagate_system, which folds an arbitrary function down the parent-child hierarchy? Then transform_propagate_system would be that generalization applied to GlobalTransform::mul_transform.

cart commented 3 years ago

Ok I'm sold on this. Feel free to put together a pr!

finegeometer commented 3 years ago

Based on the discussion in my attempted PR, I have decided to close this issue.