Skip to content

Added bevy_transform::component::local_space::LocalSpace#3044

Closed
JonahPlusPlus wants to merge 7 commits intobevyengine:mainfrom
JonahPlusPlus:main
Closed

Added bevy_transform::component::local_space::LocalSpace#3044
JonahPlusPlus wants to merge 7 commits intobevyengine:mainfrom
JonahPlusPlus:main

Conversation

@JonahPlusPlus
Copy link
Contributor

Relation describes the relationship between parent and child transforms.

Objective

Solution

  • Relation is a component that can be added to children, meaning different children can have different relationships to the parent
  • It has three fields for translation, rotation and scale, which are optional, where None would mean there is no relationship
  • Some can hold a fn(_: &mut Vec3) (in the case of translation and scale, rotation is a Quat), which allows assigning a closure that can modify a clone of the parent's field
  • By default, fields aren't None but Some(|_| ()), which means to use the relationship that is already implemented, 1:1. This allows for users to set one part of the Relation and use ..Default::default() for the rest

Alternatives

  • I had a hard time naming this, Relation might be too generic, IDK
  • Instead of having separate fields for translation, rotation and scale, it's possible to just use one closure that takes a clone of the parent's transform, however, I felt that using separate Options allowed for users to write less code, as they would have to set fields to 1.0 if it used a transform compared to just setting it to None in what I believe would be common cases like turning off rotation or scale. Could use some more insight/research.

Relation describes the relationship between parent and child transforms.
@github-actions github-actions bot added the S-Needs-Triage This issue needs to be labelled label Oct 28, 2021
@mockersf mockersf added A-Transform Translations, rotations and scales and removed S-Needs-Triage This issue needs to be labelled labels Oct 28, 2021
Cargo.toml Outdated
Comment on lines +290 to +292
[[example]]
name = "relation"
path = "examples/ecs/relation.rs"
Copy link
Member

Choose a reason for hiding this comment

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

this example would probably be better in the 3d category as it's where the parenting example already is

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Huh, since it applies to both 3D and 2D, I just thought ECS would be the closest. Wouldn't it be confusing for people looking for an example who are making a 2D game to have to look in the 3D category?

@JonahPlusPlus

This comment has been minimized.

@JonahPlusPlus

This comment has been minimized.

@JonahPlusPlus

This comment has been minimized.

@JonahPlusPlus
Copy link
Contributor Author

JonahPlusPlus commented Oct 29, 2021

I don't know how I would solve the check advisories issue, that seems like an issue with the dependencies, so I won't bother with that. Otherwise, I suppose this pull request is good.

@JonahPlusPlus
Copy link
Contributor Author

One limitation of this implementation is the lack of flexibility when it comes to mixing properties. Say you want to make the child's scale dependent on the parent's position, you wouldn't be able to. If this passed the whole transform to one closure, then you would be able to. However, I don't see this as too much of an issue, since I can't think of any use case where it would need to mix properties like this, where it wouldn't be better to use a system to update it.

let temp = v.x;
v.x = v.y;
v.y = temp;
}),
Copy link
Contributor

Choose a reason for hiding this comment

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

Do you have a good (useful) example of a function that can't be represented as a matrix? I've found that it's best to use matrices instead of functions wherever possible. Also, I prefer Fn(T) -> T over Fn(&mut T), because it's easier to use (in this case just Some(|v| v.yxz())) and likely as efficient. Maybe you could remove the need for Option as well.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, I was considering using a Fn(T) -> T, but didn't consider methods like .yxz(). I'll change it to use that. As for the Option, I'm going to make an edit to allow parent rotation to affect position but not rotation. Currently, None and Some(|q| *q = Quat::default()) have the same result, which was an oversight. I realize now that I should have made this a draft.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Also, I'm not aware of how I would replace functions with matrices. Maybe you could explain how I would represent this example? I can't see how I would switch fields with a matrix without variables which would involve a closure, unless I used numbers that represent indices, but that seems tedious.

Copy link
Contributor Author

@JonahPlusPlus JonahPlusPlus Oct 29, 2021

Choose a reason for hiding this comment

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

What about making it fn(&mut T) -> &mut T? That would allow users to do v.x *= 2; v.yxz(). Otherwise, they would have to do v.yxz() * Vec::new(1.0, 2.0, 1.0), which is more readable but probably less performant, with the unnecessary multiplication (does the compiler know to remove this?). Of course, it can be mitigated by just writing it out as let mut v = v.yxz(); v.y *= 2; v but I like stuff to be as concise as possible.

Copy link
Contributor

Choose a reason for hiding this comment

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

You can swap x/y wih Mat3::from_cols(Vec3::Y, Vec3::X, Vec3::Z) which is the 3x3 matrix

X Y Z
0 1 0
1 0 0
0 0 1

Multiplying that matrix by a Vec3 will switch the x and y values. In fact, any linear function from N inputs to M outputs can be represented as an MxN matrix. Each element of the output is the dot product of the input and the corresponding column (for vectors.)

So in the above example, the first column project y, the next projects x, and then z.

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 see and then by changing the top row to be 0 | 2 | 0, it would multiply the y field by two. And then addition and subtraction can be handled by setting the child's transform to get an offset. All right, I'll change it to be using Option<Mat3> (Mat4 I'm guessing for Quat?). I might remove the Option if I can't figure out how to get that alternate behavior working.

Copy link
Contributor Author

@JonahPlusPlus JonahPlusPlus Oct 29, 2021

Choose a reason for hiding this comment

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

Hmm, I'm wondering if this will get messy: translation: Some(const_mat3!([0.0, 2.0, 0.0],[1.0, 0.0, 0.0],[0.0, 0.0, 1.0])). Though, another thing I realized about this is that you could mix the amount of influence by having multiple fields add to the target amount of change: 0.5, 0.5, 1.0. I believe this would allow movement on a custom axis? Instead of being a TransformMap should I call this LocalSpace? Or would that be too confusing with the local methods in GlobalTransform?

@HackerFoo
Copy link
Contributor

The name "Relation" is likely to cause confusion in the future, because there have been efforts to build general relations in Bevy: bevyengine/rfcs#18

@JonahPlusPlus JonahPlusPlus marked this pull request as draft October 29, 2021 02:37
@JonahPlusPlus
Copy link
Contributor Author

I could change the name to TransformMap, as long as that's not too long.

@alice-i-cecile
Copy link
Member

I could change the name to TransformMap, as long as that's not too long.

Sounds good to me.

@alice-i-cecile alice-i-cecile added the C-Feature A new feature, making something new possible label Oct 29, 2021
Now uses matrices for mapping Transforms
@JonahPlusPlus JonahPlusPlus changed the title Added bevy_transform::component::relation::Relation Added bevy_transform::component::local_space::LocalSpace Oct 29, 2021
@JonahPlusPlus
Copy link
Contributor Author

Actually changed the name to LocalSpace, since it better reflects the behavior you can get from using matrices, rather than function pointers.

@JonahPlusPlus JonahPlusPlus marked this pull request as ready for review October 29, 2021 20:34
@JonahPlusPlus
Copy link
Contributor Author

I also removed Option since I couldn't figure out how to get the alternative behavior I was looking for without making complicated changes to the internals. For example: rotating the parent to change the position of the child but locking the orientation of the child isn't possible right now. Otherwise, I think this good for most use cases.

@HackerFoo
Copy link
Contributor

@JonahPlusPlus could you please add some motivation to the PR description? What problem was this designed to address? If this is not a common problem, I think it would be better for the user to add a separate system and component to describe the relative movement (which should listed as an alternative.) I use something like this to manage a transform that only applies to one type of entity, so I can pretend to move the camera without affecting other meshes that are part of the interface. Using the hierarchy for this would have been a pain.

@HackerFoo
Copy link
Contributor

HackerFoo commented Oct 29, 2021

I guess this is one, but it seems pretty specific:

An example use case is a tank with a child turret entity. The turret should inherit location and scale from the tank, but should rotate independently.

It's actually not that hard to do - just take the inverse of the GlobalTransform's rotation (which "undoes" all rotations applied to it) multiplied by the desired rotation.

@JonahPlusPlus
Copy link
Contributor Author

@HackerFoo I'll admit it, it really is useful for more niche cases, but I think that's also why it's so useful, since it can be applied to a variety of niche cases. I think the main use cases though would be animation and constraints. Example

Realized that no, you can't actually get the same result.
@alice-i-cecile alice-i-cecile requested a review from james7132 May 2, 2022 16:57
@alice-i-cecile alice-i-cecile added X-Needs-SME This type of work requires an SME to approve it. A-Hierarchy labels May 16, 2022
Copy link
Member

@alice-i-cecile alice-i-cecile left a comment

Choose a reason for hiding this comment

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

Relation can't be used here; it conflicts with the usage in #3742. I also think that this design space is highly controversial.

My feeling is that this needs to be addressed as part of an RFC tackling the hierarchy management.

@alice-i-cecile alice-i-cecile added the S-Needs-Design-Doc This issue or PR is particularly complex, and needs an approved design doc before it can be merged label May 16, 2022
@JonahPlusPlus JonahPlusPlus deleted the main branch March 31, 2023 02:36
@JonahPlusPlus JonahPlusPlus restored the main branch March 31, 2023 02:36
@alice-i-cecile alice-i-cecile added A-ECS Entities, components, systems, and events and removed A-Hierarchy labels Jan 28, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

A-ECS Entities, components, systems, and events A-Transform Translations, rotations and scales C-Feature A new feature, making something new possible S-Needs-Design-Doc This issue or PR is particularly complex, and needs an approved design doc before it can be merged X-Needs-SME This type of work requires an SME to approve it.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Lock values of transforms. Allow specifying which transform components are relative to the parent, and which are global

5 participants