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

Make more Transform APIs interoperate with Dir3 #12481

Closed
mweatherley opened this issue Mar 14, 2024 · 0 comments · Fixed by #12530
Closed

Make more Transform APIs interoperate with Dir3 #12481

mweatherley opened this issue Mar 14, 2024 · 0 comments · Fixed by #12530
Labels
A-Math Fundamental domain-agnostic mathematical operations A-Transform Translations, rotations and scales C-Usability A targeted quality-of-life change that makes Bevy easier to use D-Trivial Nice and easy! A great choice to get started with Bevy

Comments

@mweatherley
Copy link
Contributor

What problem does this solve or what need does it fill?

Presently, Transform::look_to and Transform::align (along with other APIs derived from them) take Vec3 inputs even when their parameters are, morally speaking, directions. Conversions between Vec3 and Dir3 are annoying for end-users, and some of our other APIs provide Dir3, while doing mathematics 'by hand' often results in a Vec3, so simultaneously capturing these would be nice.

What solution would you like?

After some discussion on Discord, it appears that using impl TryInto<Dir3> would be natural for these cases, since they

  1. treat their input essentially as a direction even though it's a vector
  2. already provide replacement input in the case that the conversion Vec3 -> Dir3 would fail

Thus, in particular, changing these inputs from Vec3 to impl TryInto<Dir3> is not even a breaking change, but it would allow Dir3 input to be given just as easily as Vec3.

What alternative(s) have you considered?

Users can manually convert between Vec3 and Dir3 to get around this.

Additional context

On the level of API design, just using Dir3 was considered for Transform::align because it is more "natural", but providing a Dir3 is often not ergonomic (especially since the Vec3 conversion is fallible) and because Transform::look_to used Vec3 already.

@mweatherley mweatherley added C-Feature A new feature, making something new possible S-Needs-Triage This issue needs to be labelled labels Mar 14, 2024
@alice-i-cecile alice-i-cecile added D-Trivial Nice and easy! A great choice to get started with Bevy C-Usability A targeted quality-of-life change that makes Bevy easier to use A-Transform Translations, rotations and scales A-Math Fundamental domain-agnostic mathematical operations and removed C-Feature A new feature, making something new possible S-Needs-Triage This issue needs to be labelled labels Mar 14, 2024
github-merge-queue bot pushed a commit that referenced this issue Mar 17, 2024
# Objective

Make `Transform` APIs more ergonomic by allowing users to pass `Dir3` as
an argument where a direction is needed. Fixes #12481.

## Solution

Accept `impl TryInto<Dir3>` instead of `Vec3` for direction/axis
arguments in `Transform` APIs

---

## Changelog
The following `Transform` methods now accept an `impl TryInto<Dir3>`
argument where they previously accepted directions as `Vec3`:
* `Transform::{look_to,looking_to}`
* `Transform::{look_at,looking_at}`
* `Transform::{align,aligned_by}`


## Migration Guide

This is not a breaking change since the arguments were previously `Vec3`
which already implements `TryInto<Dir3>`, and behavior is unchanged.

---------

Co-authored-by: Alice Cecile <alice.i.cecile@gmail.com>
Co-authored-by: IQuick 143 <IQuick143cz@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-Math Fundamental domain-agnostic mathematical operations A-Transform Translations, rotations and scales C-Usability A targeted quality-of-life change that makes Bevy easier to use D-Trivial Nice and easy! A great choice to get started with Bevy
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants