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

Add TransformBuilder API #266

Closed
wants to merge 4 commits into from

Conversation

GrantMoyer
Copy link
Contributor

@GrantMoyer GrantMoyer commented Aug 21, 2020

I've seen a few people confused about how to implement rotation and other transforms in the bevy discord help channel. I think something like the TransformBuilder in this PR might make working with transformations easier for some.

There are a couple parts of this API that I'm not fully happy with. For one thing, It's possible to loose information using some of the build_*() methods. For example build_rotation() discards any translation component of the transform. But I'm not sure what can be done about this short of making a new builder for each possible type of transform, which I think would make the API a lot more confusing.

For another thing, the builder isn't as computationally efficient as it could be. It converts all transformations to a Mat4 internally, then converts back to components during the build step.

For a third, this API doesn't do a whole lot to address people's trouble with quaternions. I think the API could be extended to help with some things, for example then_look_at(point: &Vec3), but it's not clear if it could be extended to help with things like quaternion interpolation. Maybe it could use some methods like then_rotate_toward(other: &Quat, percent: f32) or then_rotate_toward(other: &Quat, angle: f32).

Finally, I'm not sure an API like this is necessary or wanted, but I'm creating this PR to get feedback.

/// with an identity transform.
/// * The current transform can be applied after another transform using
/// the `after_*()` methods.
/// * The current transform can be applied before another transform usig

This comment was marked as outdated.

@karroffel karroffel added A-ECS Entities, components, systems, and events C-Feature A new feature, making something new possible labels Aug 21, 2020
@GrantMoyer
Copy link
Contributor Author

After sitting on this for a bit, I think maybe it's better to just add this functionality to each of the individual components. For example, Rotation would have a then_translate(&self, t: &Translation) -> Transform. These type of operations would still be composable, and I think this can addresses some of my initial concerns:

  • There is no build step which silently discards parts of a transformation. And the whole transform API isn't much more complex than it already is. It even becomes a bit more uniform.
  • The implementations for the various then_*() methods could be optimized for the particular type of transform. For example, Rotation::then_rotate(&self, other: &Rotation) wouldn't need an intermediate Mat4.

@GrantMoyer GrantMoyer marked this pull request as ready for review August 26, 2020 02:03
@cart
Copy link
Member

cart commented Sep 18, 2020

Mind if we close this in favor of the current Transform rewrite (and potentially adding the ideas here: #501)

@GrantMoyer
Copy link
Contributor Author

GrantMoyer commented Sep 18, 2020

Yeah, this is obsolete.

@GrantMoyer GrantMoyer closed this Sep 18, 2020
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 C-Feature A new feature, making something new possible
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants