-
-
Notifications
You must be signed in to change notification settings - Fork 3.6k
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
Alignment API for Transforms #12187
Alignment API for Transforms #12187
Conversation
The generated |
Little bit niche, but definitely useful API. The explanation can be a bit rough to users, the Since this is a very 'generic' function, which will be exposed under wrappers, it can use a more explicit name like As for the bloat with various axis combinations, perhaps we can use an enum, similar to AlignAxes {
XY,
NXY,
NXNY,
XNY,
...
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I like this, nice work! Do you think you could also add a doc-test for align
?
Whats the plan for moving this up into glam
?
Done!
Great question. Mathematically speaking, the process that we're doing here really consists of two steps:
Algebraically, number 2 is basically trivial (at least given what's already in So I think the trick will be finding the right balance in terms of the kinds of things that "belong" in More than anything, I'm concerned about the tension between:
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice work!
I'd prefer if the doc test asserted the invariant presented in the doc for some standard case, but I'm going to approve as is.
- applying it to
main_axis
results inmain_direction
- applying it to
secondary_axis
produces a vector that lies in the half-plane generated bymain_direction
andsecondary_direction
(with positive contribution bysecondary_direction
)
Please do link the glam
PR here if you end up making one. I think implementing it on the transform is a good improvement on it's own.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One doc nit, but otherwise I'm pleased by this. Thanks for motivating this so well, and the really clear internals!
… behavior in comments, updated example description
…comparing output)
Cool! I changed the doctest so that the examples are more like a standard library function — demonstrating some inputs and outputs along with the fallback behavior.
Will do! |
Objective
Solution
Transform::align
, which allows a rotation to be specified by four pieces of alignment data, as explained by the documentation:Transform::aligned_by
, the returning-Self version ofalign
:Transform
APIs, so when run, the example demonstrates what the API does in space:Changelog
align
,aligned_by
toTransform
.Discussion
On the form of
align
The original issue linked above suggests an API similar to that of the existing
Transform::look_to
method:Not allowing an input axis of some sort that is to be aligned with
direction
would not really solve the problem in the issue, since the user could easily be in a scenario where they have to compose with another rotation on their own (undesirable). This leads to something like:However, this still has two problems:
Specifying both leads us to the present situation, with two local axis inputs (
main_axis
andsecondary_axis
) and two target directions (main_direction
andsecondary_direction
). This might seem a little cumbersome for general use, but for the time being I stand by the decision not to expand further without prompting from users. I'll expand on this below.Additional APIs?
Presently, this PR introduces only
align
andaligned_by
. Other potentially useful bundles of API surface arrange into a few different categories:Transform::look_at
, which might look something like this:(This is simple but still runs into issues when the user wants to point the local Y-axis somewhere.)
(Here, use of the
up
vector doesn't lose any generality, but it might be less convenient to specify than something else. This does naturally leave open the question of whatalign_y
would look like if we provided it.)Morally speaking, I do think that the
up
business is more pertinent when the intention is to work with cameras, which thelook_at
andlook_to
APIs seem to cover pretty well. If that's the case, then I'm not sure what the ideal shape for these API functions would be, since it seems like a lot of input would have to be baked into the function definitions. For some cases, this might not be the end of the world:(However, this is not symmetrical in x and z, so you'd still need six API functions just to support the standard positive coordinate axes, and if you support negative axes then things really start to balloon.)
The reasons that these are not actually produced in this PR are as follows:
align
to particular inputs; e.g.:With that in mind, I would prefer instead to focus on making the documentation and examples for a thin API as clear as possible, so that users can get a grip on the tool and specialize it for their own needs when they feel the desire to do so.
Dir3
?As in the case of
Transform::look_to
andTransform::look_at
, the inputs to this function are, morally speaking, directions rather than vectors (actually, if we're being pedantic, the input is really really a pair of orthonormal frames), so it's worth asking whether we should really be usingDir3
as inputs instead ofVec3
. I opted forVec3
for the following reasons:Dir3
in user-space is just more annoying than providing aVec3
. Even in the most basic cases (e.g. providing a vector literal), you still have to do error handling or call an unsafe unwrap in your function invocations.Vec3
, so we are just adhering to the same thing.Of course, the use of
Vec3
has its own downsides; it can be argued that the replacement of zero-vectors with fixed ones (which we do inTransform::align
as well asTransform::look_to
) more-or-less amounts to failing silently.Future steps
The question of additional APIs was addressed above. For me, the main thing here to handle more immediately is actually just upstreaming this API (or something similar and slightly mathier) to
glam::Quat
. The reason that this would be desirable for users is that this API currently only works withTransform
s even though all it's actually doing is specifying a rotation. Upstreaming toglam::Quat
, properly done, could buy a lot basically for free, since a number ofTransform
methods take a rotation as an input. Using these together would require a little bit of mathematical savvy, but it opens up some good things (e.g.Transform::rotate_around
).