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

Use new_observer_frame in Transform::look_at #1142

Merged
merged 2 commits into from Nov 16, 2018

Conversation

Projects
None yet
4 participants
@JamesAngstrom
Copy link
Contributor

JamesAngstrom commented Nov 14, 2018

Make the forward vector of and entity to point towards position (See #1134) when using Transform::look_at, as per the documentation. In particular, for a camera:

camera_transform.look_at(point, world_up);

should point the camera towards point, as in Unity's Transform.LookAt example:

https://docs.unity3d.com/ScriptReference/Transform.LookAt.html

I have an example that demonstrates this change here:

https://github.com/JamesAngstrom/look_at

you can switch between my branch and current amethyst master in Cargo.toml and see how the camera behavior changes.

Before (camera rotates away from the sphere):
before
After (camera points at sphere and rotates around it):
after

We could also have a separate function that has the new behavior with a different name, as suggested in the issue thread, but I think the above is likely going to be the expected behavior of look_at, and matches other game engines.

Use new_observer_frame in Transform::look_at
We want the forward vector of the entity to point towards position
when using Transform::look_at, as per the documentation. In
particular, for a camera

camera_transform.look_at(point, world_up);

should point the camera towards point, as in Unity's Transform.LookAt
example:

https://docs.unity3d.com/ScriptReference/Transform.LookAt.html
@magnonellie

This comment has been minimized.

Copy link
Contributor

magnonellie commented Nov 14, 2018

I should probably note that what I meant with the new function name was to remove the old one entirely and just have the new one. That way people who used the function before will get compile errors since the behavior was incorrect in cgmath too. Then there's also the confusion between nalgebra and amethyst that occurs if it's kept this way which is probably the most important reason to rename it.

@torkleyy
Copy link
Member

torkleyy left a comment

Thanks for the contribution! Other than the issue @magnonellie brought up, looks good 👍

Rename Transform::look_at to Transform::face_towards
Update the example above the method to make sure it's correct, and
add a little example using t.move_forwards() because that might be
more helpful to more people
@JamesAngstrom

This comment has been minimized.

Copy link
Contributor

JamesAngstrom commented Nov 15, 2018

I changed the name of the function to face_towards as suggested. Since the name changed I also updated the changelog.

I also checked the example in the comment for the function and made sure it works. I also added a little extra using move_forward after face_towards because that might be more helpful to people that the existing rotation_between example.

Also I added some extra objects to my example to make sure it wasn't somehow a camera specific issue:
rotatecubes

@Rhuagh

Rhuagh approved these changes Nov 16, 2018

@magnonellie
Copy link
Contributor

magnonellie left a comment

Thank you! It's a little bit of a shame that we can't use face_towards in nalgebra yet since it isn't implemented, but I'm fairly certain we'll get a deprecation warning in the future 😄

bors r+

bors bot added a commit that referenced this pull request Nov 16, 2018

Merge #1142
1142: Use new_observer_frame in Transform::look_at r=magnonellie a=JamesAngstrom

Make the forward vector of and entity to point towards position (See #1134) when using Transform::look_at, as per the documentation. In particular, for a camera:

```rust
camera_transform.look_at(point, world_up);
```

should point the camera towards point, as in Unity's Transform.LookAt example:

https://docs.unity3d.com/ScriptReference/Transform.LookAt.html

I have an example that demonstrates this change here:

https://github.com/JamesAngstrom/look_at

you can switch between my branch and current amethyst master in Cargo.toml and see how the camera  behavior changes.

Before (camera rotates away from the sphere):
![before](https://user-images.githubusercontent.com/44684728/48505476-0d635f00-e83f-11e8-8277-317ed2384076.png)
After (camera points at sphere and rotates around it):
![after](https://user-images.githubusercontent.com/44684728/48505512-22d88900-e83f-11e8-9f24-131771c0cd26.png)

We could also have a separate function that has the new behavior with a different name, as suggested in the issue thread, but I think the above is likely going to be the expected behavior of look_at, and matches other game engines.


Co-authored-by: James Angstrom <james.angstrom@gmail.com>
@bors

This comment has been minimized.

Copy link
Contributor

bors bot commented Nov 16, 2018

@magnonellie

This comment has been minimized.

Copy link
Contributor

magnonellie commented Nov 16, 2018

Why can't Linux be the only OS? Everything would have been so much easier that way 😣

bors retry

bors bot added a commit that referenced this pull request Nov 16, 2018

Merge #1142
1142: Use new_observer_frame in Transform::look_at r=magnonellie a=JamesAngstrom

Make the forward vector of and entity to point towards position (See #1134) when using Transform::look_at, as per the documentation. In particular, for a camera:

```rust
camera_transform.look_at(point, world_up);
```

should point the camera towards point, as in Unity's Transform.LookAt example:

https://docs.unity3d.com/ScriptReference/Transform.LookAt.html

I have an example that demonstrates this change here:

https://github.com/JamesAngstrom/look_at

you can switch between my branch and current amethyst master in Cargo.toml and see how the camera  behavior changes.

Before (camera rotates away from the sphere):
![before](https://user-images.githubusercontent.com/44684728/48505476-0d635f00-e83f-11e8-8277-317ed2384076.png)
After (camera points at sphere and rotates around it):
![after](https://user-images.githubusercontent.com/44684728/48505512-22d88900-e83f-11e8-9f24-131771c0cd26.png)

We could also have a separate function that has the new behavior with a different name, as suggested in the issue thread, but I think the above is likely going to be the expected behavior of look_at, and matches other game engines.


Co-authored-by: James Angstrom <james.angstrom@gmail.com>
@bors

This comment has been minimized.

Copy link
Contributor

bors bot commented Nov 16, 2018

@bors bors bot merged commit 39bd79c into amethyst:master Nov 16, 2018

4 checks passed

bors Build succeeded
Details
concourse-ci/linux-book-tests Concourse CI build success
Details
concourse-ci/linux-tests Concourse CI build success
Details
concourse-ci/rustfmt Concourse CI build success
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment