bevyengine / bevy

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

`Angle` type #11132

Closed Jondolf closed 8 months ago

Jondolf commented 8 months ago

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

Angles come up quite frequently: transform rotations, primitive shapes, physics, spotlights, rendering, and even touch input for stylus altitude angles, just to name a few.

However, angles come with several ambiguities. Consider the following:

transform.rotate_x(1.4);

Is the angle in degrees or radians? If it represents a rotation, is it clockwise or counterclockwise? While experienced Bevy users might know that it's in radians and counterclockwise, it's not immediately clear from the actual code and requires checking the documentation.

The code above is just a single example, but the issue is quite common. To improve readability and ergonomics, it would be nice to have a consistent and expressive way of representing angles and/or 2D rotations.

This issue focuses mainly on an Angle type, as Rotation2d would have slightly different needs, and I'm not sure if we even need a new type for it. See the "Angle vs. Rotation2d" section at the end.

Let's establish some goals:

  1. Explicitness: It should be clear that a value is an angle in degrees or radians and not just some magic number.
  2. Ergonomics: Creating and using angles should be straightforward and ergonomic.
  3. Memory efficiency: The type should be very minimal in terms of data.
  4. Conversion efficiency: Converting to/from radians/degrees should be efficient.
  5. Computational efficiency: Common computations (like trigonometric operations) should be efficient.

What solution would you like?

There are several options here, and they all have their tradeoffs. I don't think there is an approach that ticks all the goals listed above, so some compromises are necessary.

Option 1: Type aliases

The simplest option is to just add type aliases:

pub type Degrees = f32;
pub type Radians = f32;

This very straightforward approach accomplishes goals 2, 3, 4, and (decently) 5, but explicitness is lacking.

While it is clear from the function signature or property type what kind of angle it should be, it's not clear from the actual value, which hurts readability. For example, the issue when rotating a transform remains:

transform.rotate_x(1.4);

Is 1.4 in degrees or radians, and is it clockwise or counterclockwise? You'd need to check the type and/or docs to find out, so this doesn't really help improve code readability that much.

There's also a potential footgun here; Radians + Degrees does not produce a sensible value, because they are just aliases for normal floats and don't actually treat the values as special angles.

Option 2: Enum

The second approach is to use an enum:

pub enum Angle {
    Degrees(pub f32),
    Radians(pub f32),
}

Alright! This ticks 3 and 4, and mostly ticks 1 and 2.

It's very explicit and ergonomic to write Angle::Degrees(90.0). However, what should happen if you do Angle::Degrees(90.0) + Angle::Radians(PI)? Should it return degrees or radians? Mathematical operations would need separate methods that return different types, and it quickly becomes highly unergonomic.

Another issue is that a specific variant will always be more efficient. Most internal computations in e.g. physics and rendering use radians, so if the user provides degrees, it might be necessary to always convert it to radians, which is just unnecessary computation. However, I suppose you could internally convert just once and reuse that everywhere, so I'm not sure if this is actually a big issue.

Option 3: Struct with radians internally

The third and perhaps most common option is to have a struct that just stores the angle in radians:

pub struct Angle {
    radians: f32,
}

This is very similar to the Rotation2D type in the euclid crate. I'd say that it ticks goals 1, 2, 3, 4 and mostly ticks 5, so it's quite close to ideal.

The API could be roughly like this:

let deg = Angle::degrees(180.0);
let rad = Angle::radians(PI);

assert_eq!(deg, rad);
assert_eq!(deg.as_radians(), rad.as_radians());

From the outside, this approach doesn't make it clear what unit the value inside Angle uses, but it's not necessarily an issue. It simply abstracts the unit away via methods and makes computations between radians and degrees straightforward, unlike the previous approaches.

Internally, the angle is stored as radians, because it's needed for trigonometric operations and therefore much more commonly used in actual computations. This does mean that degrees must be converted to radians, but it's only during the construction of the angle, and usually it would need to be done anyway.

Option 4: Struct with sine and cosine of angle

This is a more special approach that attempts to optimize common operations. The angle is represented like this:

pub struct Angle {
    /// The sine of the angle in radians.
    sin: f32,
    /// The cosine of the angle in radians.
    cos: f32,
}

The approach and API is nearly identical to Option 3, and at least ticks goals 1, 2, and 5. However, instead of storing the raw angle, we store its sine and cosine.

The reasoning here is that usually when working with angles, we actually want the sin/cos/tan of the angle. By computing it just once, we can make several computations more efficient. This is what my physics engine bevy_xpbd does for the 2D version of the Rotation component, and it speeds up e.g. the common task of rotating vectors:

// Rotate vector by self.
// Storing sin and cos skips 4 (or 2) trigonometric operations here.
// This is potentially called tens or hundreds of thousands of times per frame in bevy_xpbd.
Vec2::new(
    vec.x * self.cos - vec.y * self.sin,
    vec.x * self.sin + vec.y * self.cos,
)

However, here come the caveats that probably make this approach undesirable for a general-purpose Angle type.

Extra

Number type

We don't always want just f32 angles. The Bevy codebase already has some f64 angles.

We could add a DAngle or AngleF64 type for this, but I believe generics are nicer here. I have a working version of Angle<T> with f32 and f64 support already, although it does require a trait like Num for generic number types.

Angle vs. Rotation2d

Instead of adding an Angle type, we could just add a 2D rotation type. However, I feel like that'd be a bit too niche for now, as it'd only really be used for a rotation in Transform2d once we have that, and maybe things like rotating colliders and primitive shapes. We don't really have that many proper use cases for Rotation2d in Bevy itself yet.

If we added Rotation2d, it'd also make sense to have a Rotation3d, but that's just a quaternion, which already has a type, so the naming wouldn't be very consistent.

I think a general-purpose angle type would be more valuable for now, and we could always add a rotation type that builds on it if needed in the future.

The needs for Rotation2d are also different, so a different representation could make more sense. I believe the options would be:

  1. An Angle clamped to the [-pi, pi] range
  2. A 2x2 rotation matrix, a Mat2
  3. The sine and cosine of the angle, as shown in Option 4

Angle in Glam?

Angle has the issue that we can't use it for all APIs in Glam types (Bevy's math library). For example, Quat::from_rotation_x just takes an f32, and we can't change that with a type in Bevy.

As it turns out, Glam did originally have an Angle type, but it was removed, see https://github.com/bitshifter/glam-rs/issues/22. The reasoning was basically that it was perceived as too high level for a low-level library like Glam, as it can just assume everything to use radians.

I think that this is a valid point, but in practise I'd still like to have an Angle type in Bevy due to the explicitness and ergonomics it provides. Our options are to (1) just add it into bevy_math, ignoring Glam's APIs that use angles, or (2) open discussion to add back Angle to Glam itself.

Conclusion

Personally, I think I prefer Option 3, and I have it mostly implemented for Bevy already. However, if we don't want to add a new struct, then the type alias approach could be okay for now, as it'd at least help make function signatures a bit more explicit.

Other opinions on this and the need for a Rotation2d type would be appreciated though :)

alice-i-cecile commented 8 months ago

I like option 3. Option 2 seems frustrating and inferior to 3. Option 1 doesn't seem like a meaningful improvement over just using floats.

RobWalt commented 8 months ago

What do you think about using the angle crate?

They split the two representations into two structs which implement a common Angle trait.

Obvious cons:

Solutions to the cons:

alice-i-cecile commented 8 months ago

IMO we should just write our own rather than worrying about more trivial dependencies.

aleqdev commented 8 months ago

Personally i dont like this idea at all, because angles are too common to restrict the ergonomics of calculations with typed float. Rust is a good language to guide its users using its type system, but sometimes they should think about the logic of their application themselves.

If community would prefer typed angles, I propose the following:

  1. Change bevy functions to take IntoAngle. (Into angle means into angle in radians)
  2. Add two new structs: Degrees(f32) and Radians(f32)
  3. Implement IntoAngle for Degrees, Radians and f32. (f32 would be radians by themselves)
  4. Overload operators for Degrees and Radians

Working with angles would be like so:

let x = Degrees(90.0);
rotate_x(x / 2.0 + 60.0);
rotate_x(Degrees(90.0) + Radians(1.0));
rotate_x(pi / 2.0)

We can also add functions with _rad and _deg:

rotate_x(pi);
rotate_x_rad(pi); // same as above
rotate_x_deg(180.0);

No breaking changes should occur (i think, i am writing this on phone right after i saw this)

Davier commented 8 months ago

I think the current situation is fine actually. Both the unit and the direction are the standard across CS and mathematics. It's something you only need to learn once. I don't want to have to write something like transform.rotate_x(Angle::Radian(PI)) (and that still doesn't specify the direction). I see that the docs don't mention the direction of the rotation either, that could definitely be improved. Otherwise, @aleqdev 's first suggestion seems more ergonomic, at the cost of a more complex API.

Jondolf commented 8 months ago

@Davier

Both the unit and the direction are the standard across CS and mathematics.

Counterclockwise is the convention for radians, but I wouldn't say that radians are necessarily "the standard" in mathematics. For working with pi and trigonometric operations, it is, but a lot of geometry also uses degrees.

More importantly though, a very large chunk of game developers don't have a deep background in CS or mathematics. A lot of them are hobbyists and some are artists. In most art programs, clockwise degrees are actually the standard for 2D rotations, and that's also what Godot uses for 2D transforms.

While radians are more common, at least in internal computations, there is no singular convention that works for everything. In a lot of cases, degrees can just be more intuitive from the user's perspective.

@aleqdev

Personally, I still like the third option I proposed, and updating Bevy's examples to use it just convinced me further of its benefits. For me, ergonomics isn't just about saving some typing, but that the code is also as readable as possible. It should be clear that a value is an angle without having to look at docs or the surrounding code for extra context.

You can also still do most math operations on Angle. You can add or subtract angles, multiply or divide by floats, and so on.

That being said, I do really like your proposal. It's pretty similar to the enum approach, but having two separate types like that is more ergonomic and explicit. I'm personally not a fan of the implicitness of automatically converting floats to angles, but if others prefer that, I'm open to it. I'd probably change IntoAngle to just Into<Radians> and Into<Degrees> though.

I'll probably make two PRs, one with the third option I proposed, and one with the Degrees(f32) and Radians(f32) approach. That way, we can get a better idea of what it'd be like in practise and what people prefer.

aleqdev commented 8 months ago

Main purpose of IntoAngle is to allow to write single api that uses eigher radians or degrees. rotate_x(a: impl IntoAngle) {...} covers both rotate_x(a: impl Into<Radians>) and rotate_x(a: impl Into<Degrees>)

RobWalt commented 8 months ago

I'm totally on your side alice. I also would rather vote for a reimplementation of the angle crate instead of using it. That's what I wanted to argue for with the second half of my message. I just wanted to say that we can use the crate as a reference.

The ideas proposed by aleqdev are exactly the ideas from the angle crate. In addition, they ship a Angle trait which probably corresponds to the idea of IntoAngle.

I'm also not a fan of the implicit conversions from primitive rust types like f32 into the user defined types. Jondolf made some really good points already and I think it is generally an antipattern to impl Into/From just to save some keystrokes.

Other than that splitting the trait up into two traits for each angle type doesn't really make sense to me yet. I thought that the unified Angle trait could provide methods to convert to desired representation of the library authors. So it's more like: the user can provide anything (rad or deg) and the library just converts into whatever fits the use case.

That being said, I probably need to experiment with all of this a bit myself. I'd happily provide/contribute to the PR for this idea as well.

Jondolf commented 8 months ago

I've been experimenting with this more, and I think Radians + Degrees structs along with a shared Angle trait might indeed be one of the best approaches. It's very ergonomic, explicit, and efficient, and doesn't seem to have significant drawbacks other than math operations between the units like Radians(PI) + Degrees(180.0) needing separate methods for type inference to work. But I think that'd be an issue with any approach that separates the units like this.

I made a PR for this already: #11148. For now, I didn't add implicit conversion from f32 since I (and @RobWalt and most likely Alice) prefer the more explicit approach. That can be discussed further in the PR though, I can change it if that's what the community wants.

If people want, I can still make a PR using the third option originally shown in this PR, and that's what I actually started with. I do think the Radians & Degrees approach might be more ergonomic, efficient, and explicit though.

Thanks everyone for the feedback and ideas! Feel free to continue sharing thoughts and alternative API ideas if you have any :)

Edit: After further experimentation and feedback, I think the issue of math operations like Radians(PI) + Degrees(180.0) not working nicely is very annoying and a pretty big downside of having separate angle types like this. I'll make a PR for the third option I originally proposed too, as I believe that's the other top-contender, and it doesn't have this issue. That way we can compare and see which (if any) approach we should go for.

djeedai commented 8 months ago

I'm generally all in favor of helping usability and lowering the barrier to entry, but I have to admit here that I'm in favor of, if doing anything at all, at least a solution where the math standard (rad, ccw) is the default everywhere, and can be used implicitly.

Degrees are best used in UIs etc. because they yield "round" numbers like 90 degrees, easier to input. But there's no math function using them (look at trig functions in std, also in C/C++ and a lot of other languages). And we should avoid any extra conversion cost and rounding error making them available broadly.

For orientation, supporting both CW and CCW equally is a recipe for confusion and errors. There's a strong argument for having a unique standard internally across all Bevy code, third party plugins included.

I'd be a lot warmer to the idea if we were talking about a higher level layer like the Editor UI or some scripting language, which are targeting a less code savvy audience. For Bevy internally, not so much.

rlidwka commented 8 months ago

However, angles come with several ambiguities. Consider the following:

transform.rotate_x(1.4);

Is the angle in degrees or radians? If it represents a rotation, is it clockwise or counterclockwise?

It is in radians because every angle in Bevy is in radians.

As far as rotation direction, rotate_x is just a horrible name. The function should be named rotate_yz instead (which means a shortest rotation from Y to Z in YZ plane), that leaves no place for ambiguity.

Calling rotations "clockwise" or "counterclockwise" is ambiguous without specifying the direction you're looking in. This comes up often with yaw if vertical direction is up, since most people instinctively measure direction with respect to the ground, and thus coming up with the exact opposite answer.

Jondolf commented 8 months ago

It is in radians because every angle in Bevy is in radians.

How do you know that from the perspective of a user who doesn't know Bevy's codebase?

Godot uses (clockwise) degrees for 2D transform rotation and skew, and most art programs use degrees for rotating things. It's not a fully universal convention to use radians for everything, and even if it was, new users wouldn't necessarily know that.

For public APIs, degrees can just be more intuitive and ergonomic in some cases (see my comment here for a bit more examples). This doesn't mean that Bevy's APIs should actually use degrees, but taking an impl Into<Radians> allows both ways.

As far as rotation direction, rotate_x is just a horrible name. The function should be named rotate_yz instead (which means a shortest rotation from Y to Z in YZ plane), that leaves no place for ambiguity.

Not sure about this, but it might just be because I'm used to the current way of thinking about it. I believe rotate_x is basically taking an axis-angle representation, i.e. rotating about a rotation axis (X in this case) by the given angle, which is relatively intuitive to me.

Thinking of it in terms of planes is more confusing to me, but I suppose it's mostly a matter of viewpoints.

djeedai commented 8 months ago

How do you know that from the perspective of a user who doesn't know Bevy's codebase?

Because it's the default convention in almost any math code, everywhere. This goes back again to the fact that standard libraries in most languages only provide radian trig functions. But I agree that a sprinkle of documentation would help remind the users about this.

Godot uses (clockwise) degrees for 2D transform rotation and skew, and most art programs use degrees for rotating things. It's not a fully universal convention to use radians for everything, and even if it was, new users wouldn't necessarily know that.

In code? Or in UI? Because this doc of Transform2d shows all methods taking radians only, never degrees. I'm not familiar with Godot, but as pointed above, it's perfectly reasonable and expected to use degrees in UI for user interaction. However in code, it's pretty rare.

For public APIs, degrees can just be more intuitive and ergonomic in some cases (see my comment https://github.com/bevyengine/bevy/pull/11148#issuecomment-1873292258 for a bit more examples). This doesn't mean that Bevy's APIs should actually use degrees, but taking an impl Into allows both ways.

Yes, I agree that allowing some API entry points to take degrees via an Into<> makes the API easier to use. But that's to me the same kind of feature than taking impl Into<String> instead of &str as function parameter; this has no consequence on the internal representation, and you rarely store some enum StringLike, you just pick String. So to that end, even with an impl Into<Angle> function parameter, all angles are still considered in radians when not otherwise specified. This I think gives you the best of both worlds: explicitness in APIs, performance and simplicity in algorithms and data structures.

The drawback of introducing an explicit Angle everywhere (whatever its form, trait or enum or ...) is that, paradoxically, this introduces some uncertainty, because unlike the current situation where any f32 angle is in radian, after such a change any code using f32 becomes ambiguous. So you have to be sure that the API for Angle is so well formed that it will be ubiquitous and no one will ever want to skip it and fallback to f32 for any reason.

djeedai commented 8 months ago

As far as rotation direction, rotate_x is just a horrible name. The function should be named rotate_yz instead (which means a shortest rotation from Y to Z in YZ plane), that leaves no place for ambiguity.

Note that this is copied from Quat::from_rotation_x() and similar in glam, and to that respect is consistent with the current convention in glam and Bevy. I don't think we should change that. It's maybe arguable, but inconsistency is worse I think.

rlidwka commented 8 months ago

How do you know that from the perspective of a user who doesn't know Bevy's codebase?

Because I know that x.sin() ≈ x (not 57.29*x).

Godot uses (clockwise) degrees for 2D transform rotation and skew, and most art programs use degrees for rotating things.

It is common to see degrees in any kind of user interfaces. If bevy had an editor, it would be reasonable to use degrees there by default (with mandatory ° symbol for disambiguity), and convert to radians under the hood.

In code, it's radians everywhere, because you're supposed to pass them around as a number to quaternions and such without needing to think about whether degree multiplier is attached or not.

I believe rotate_x is basically taking an axis-angle representation, i.e. rotating about a rotation axis (X in this case) by the given angle, which is relatively intuitive to me.

Here's usual graph paper from a school. Which direction is "clockwise"?

https://graph-paper.net/wp-content/uploads/2019/11/Graph-Paper-with-Axis-.jpg

rlidwka commented 8 months ago

The drawback of introducing an explicit Angle everywhere (whatever its form, trait or enum or ...) is that, paradoxically, this introduces some uncertainty, because unlike the current situation where any f32 angle is in radian, after such a change any code using f32 becomes ambiguous.

That is exactly my argument against it.

Note that this is copied from Quat::from_rotation_x() and similar in glam, and to that respect is consistent with the current convention in glam and Bevy. I don't think we should change that.

Can't change that as long as you use Quat, but this part can be documented better.

If you decide to change to Rotation2d / Rotation3d for whatever reason, it might be worth considering as part of developing a better API for both.

rlidwka commented 8 months ago

That being said, having Angle(f32) struct might be useful for the purpose of user-facing reflection API (namely, bevy_inspector).

It would have:

Is it worth having it as part of bevy? Probably yes, because we want an to have an editor eventually, where degrees are preferable.


This is similar to color (where we should use RGB everywhere in code, but in the editor user might prefer to pick HSV).

This is similar to rotation (where we should use Quat everywhere in code, but in the editor user might prefer to select yaw-pitch-roll).

Jondolf commented 8 months ago

I think I'll close this for now; I myself am unsure if we actually want angle types, at least in the form displayed here. While they would (in my opinion) be more explicit and potentially ergonomic when implemented properly, I agree that in code we should perhaps just use radians consistently for all APIs and leave degrees for very high-level things like editor UI. Radians are the sensible default for almost all code, and if you really wanted to use degrees for clarity, you could just do e.g. 90.0.to_radians().

Feel free to reopen if you have any new ideas or thoughts on this though!


Something that is adjacent to this topic but that we will actually need is some kind of Rotation2d type like the one I briefly touched on originally. We need a struct that holds the sine and cosine of an angle, either in a rotation matrix or a unit complex representation. This is because physics and especially collision detection requires rotating vectors all the time (bounding volume computation, support mapping, contact data transformation, constraints...) and having to recompute the sine and cosine every time is unnecessary and expensive. For reference, Parry passes around Nalgebra's Isometry type a ton, and that typically contains a UnitComplex for 2D rotation.

I'll probably make an issue and PR for this soon-ish. A separate rotation type would also allow us to explicitly state a consistent default for the rotation direction and unit, as well as make the API more ergonomic for rotations if possible.

johannesvollmer commented 1 week ago

We do have explicit units in the colour system and in the sizing (e.g. pixels as explicit unit for UI). It would seem inconsistent to disallow the same for angles.

From my dayjob in unity, actually 90% of the time I code in neither angles nor radians, but only in full rotations. For me, this is the most intuitive notation (the metric unit of angles), and it plays nicely with all kinds of other procedural maths, as multiplication is neutral with a full rotation. I would love an explicit IntoAngle which supports this third unit.