Skip to content
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

Should bevy-transform be two crates? #2758

Closed
finegeometer opened this issue Aug 31, 2021 · 9 comments
Closed

Should bevy-transform be two crates? #2758

finegeometer opened this issue Aug 31, 2021 · 9 comments
Labels
A-Transform Translations, rotations and scales C-Code-Quality A section of code that is hard to understand or change

Comments

@finegeometer
Copy link

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:

  • The concept of a parent-child hierarchy of entities.
  • The concept of a Transform, roughly a similarity transformation of Euclidean space.

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:

  • Everything in the modules components::{children, parent}.
  • Everything in hierarchy and its submodules, except for the test in hierarchy::hierarchy_maintenance.

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:

  • Everything in the modules components::{transform, global_transform}.
  • Everything in the module transform_propagate_system.
  • Everything in the root module.
  • The test from hierarchy::hierarchy_maintenance.

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:

  • Some imports needed to be fixed.
  • In three instances, the code children.0.iter() started failing, because the field children.0 is private, and is now defined in a different crate. This was easily fixable; just do children.iter() (using the Deref impl) instead.

Finally, the dependent crates needed to be adjusted.

  • bevy_scene depends on bevy_hierarchy, but not bevy_transform.
  • bevy_gltf and bevy_ui depend on both bevy_hierarchy and bevy_transform.
  • bevy_internal needed bevy_hierarchy added to its reexports and prelude.
  • bevy_pbr, bevy_render, bevy_sprite, and bevy_text, despite depending on bevy_transform, seem to need no changes.

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

@alice-i-cecile alice-i-cecile added A-Transform Translations, rotations and scales C-Code-Quality A section of code that is hard to understand or change labels Aug 31, 2021
@MinerSebas
Copy link
Contributor

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

@finegeometer
Copy link
Author

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
Copy link
Contributor

LegNeato commented Sep 2, 2021

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

@finegeometer
Copy link
Author

finegeometer commented Sep 2, 2021

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
Copy link
Member

mockersf commented Sep 2, 2021

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
Copy link
Member

cart commented Sep 4, 2021

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
Copy link
Author

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.

  • I thought there were good reasons for changing to the current representation.
  • If Transform were a 4x4 matrix, a lot of code would probably implicitly assume that every Transform is in SE(3) anyway. Such code would work fine for most applications, only to fail when I try to use it.
    • Evidence: Back in Bevy 2.1, when Transform was a Mat4, most of Transform's methods made that exact assumption.
  • There might be other use cases which can't be accommodated by 4x4 matrices. For instance, a four-dimensional game, though that's difficult for other reasons. Or a weirder curved space like S²×E, which is curved in some directions, but not others.

¹ 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
Copy link
Member

cart commented Sep 6, 2021

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

@finegeometer
Copy link
Author

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

@bors bors bot closed this as completed in a304fd9 Mar 15, 2022
aevyrie pushed a commit to aevyrie/bevy that referenced this issue Jun 7, 2022
# Objective

- Hierarchy tools are not just used for `Transform`: they are also used for scenes.
- In the future there's interest in using them for other features, such as visiibility inheritance.
- The fact that these tools are found in `bevy_transform` causes a great deal of user and developer confusion
- Fixes bevyengine#2758.

## Solution

- Split `bevy_transform` into two!
- Make everything work again.

Note that this is a very tightly scoped PR: I *know* there are code quality and docs issues that existed in bevy_transform that I've just moved around. We should fix those in a seperate PR and try to merge this ASAP to reduce the bitrot involved in splitting an entire crate.

## Frustrations

The API around `GlobalTransform` is a mess: we have massive code and docs duplication, no link between the two types and no clear way to extend this to other forms of inheritance.

In the medium-term, I feel pretty strongly that `GlobalTransform` should be replaced by something like `Inherited<Transform>`, which lives in `bevy_hierarchy`:

- avoids code duplication
- makes the inheritance pattern extensible
- links the types at the type-level
- allows us to remove all references to inheritance from `bevy_transform`, making it more useful as a standalone crate and cleaning up its docs

## Additional context

- double-blessed by @cart in bevyengine#4141 (comment) and bevyengine#2758 (comment)
- preparation for more advanced / cleaner hierarchy tools: go read bevyengine/rfcs#53 !
- originally attempted by @finegeometer in bevyengine#2789. It was a great idea, just needed more discussion!

Co-authored-by: Carter Anderson <mcanders1@gmail.com>
ItsDoot pushed a commit to ItsDoot/bevy that referenced this issue Feb 1, 2023
# Objective

- Hierarchy tools are not just used for `Transform`: they are also used for scenes.
- In the future there's interest in using them for other features, such as visiibility inheritance.
- The fact that these tools are found in `bevy_transform` causes a great deal of user and developer confusion
- Fixes bevyengine#2758.

## Solution

- Split `bevy_transform` into two!
- Make everything work again.

Note that this is a very tightly scoped PR: I *know* there are code quality and docs issues that existed in bevy_transform that I've just moved around. We should fix those in a seperate PR and try to merge this ASAP to reduce the bitrot involved in splitting an entire crate.

## Frustrations

The API around `GlobalTransform` is a mess: we have massive code and docs duplication, no link between the two types and no clear way to extend this to other forms of inheritance.

In the medium-term, I feel pretty strongly that `GlobalTransform` should be replaced by something like `Inherited<Transform>`, which lives in `bevy_hierarchy`:

- avoids code duplication
- makes the inheritance pattern extensible
- links the types at the type-level
- allows us to remove all references to inheritance from `bevy_transform`, making it more useful as a standalone crate and cleaning up its docs

## Additional context

- double-blessed by @cart in bevyengine#4141 (comment) and bevyengine#2758 (comment)
- preparation for more advanced / cleaner hierarchy tools: go read bevyengine/rfcs#53 !
- originally attempted by @finegeometer in bevyengine#2789. It was a great idea, just needed more discussion!

Co-authored-by: Carter Anderson <mcanders1@gmail.com>
dekirisu pushed a commit to dekirisu/bevy_gltf_trait that referenced this issue Jul 7, 2024
# Objective

- Hierarchy tools are not just used for `Transform`: they are also used for scenes.
- In the future there's interest in using them for other features, such as visiibility inheritance.
- The fact that these tools are found in `bevy_transform` causes a great deal of user and developer confusion
- Fixes #2758.

## Solution

- Split `bevy_transform` into two!
- Make everything work again.

Note that this is a very tightly scoped PR: I *know* there are code quality and docs issues that existed in bevy_transform that I've just moved around. We should fix those in a seperate PR and try to merge this ASAP to reduce the bitrot involved in splitting an entire crate.

## Frustrations

The API around `GlobalTransform` is a mess: we have massive code and docs duplication, no link between the two types and no clear way to extend this to other forms of inheritance.

In the medium-term, I feel pretty strongly that `GlobalTransform` should be replaced by something like `Inherited<Transform>`, which lives in `bevy_hierarchy`:

- avoids code duplication
- makes the inheritance pattern extensible
- links the types at the type-level
- allows us to remove all references to inheritance from `bevy_transform`, making it more useful as a standalone crate and cleaning up its docs

## Additional context

- double-blessed by @cart in bevyengine/bevy#4141 (comment) and bevyengine/bevy#2758 (comment)
- preparation for more advanced / cleaner hierarchy tools: go read bevyengine/rfcs#53 !
- originally attempted by @finegeometer in #2789. It was a great idea, just needed more discussion!

Co-authored-by: Carter Anderson <mcanders1@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-Transform Translations, rotations and scales C-Code-Quality A section of code that is hard to understand or change
Projects
None yet
6 participants