/amethyst

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

Implement advanced local movement #447

Merged
merged 1 commit into from Oct 25, 2017

Conversation

Reviewers

@Rhuagh Rhuagh

@Xaeroxe Xaeroxe

@torkleyy torkleyy

Assignees
No one assigned
Labels
Projects
None yet
Milestone
No milestone
5 participants
@minecrawler @Rhuagh @Xaeroxe @torkleyy @Aceeri
@minecrawler
Contributor

minecrawler commented Oct 24, 2017

Cleaned #342 , implements movement helpers on LocalTransform
@Rhuagh

Rhuagh approved these changes Oct 24, 2017
View changes

amethyst_renderer/src/scene.rs
@@ -65,8 +65,8 @@ impl Scene {
/// Returns the active camera in the scene.
///
/// TODO: Render to multiple viewports with possibly different cameras.
- pub fn active_camera(&self) -> Option<&Camera> {
- self.cameras.first()
+ pub fn active_camera(&mut self) -> Option<&mut Camera> {

This comment has been minimized.

Show comment
Hide comment
@Rhuagh

Rhuagh Oct 24, 2017

Member

Why is this must?
@Rhuagh

Rhuagh Oct 24, 2017

Member

Why is this must?

This comment has been minimized.

Show comment
Hide comment
@Rhuagh

Rhuagh Oct 24, 2017

Member

Mut
@Rhuagh

Rhuagh Oct 24, 2017

Member

Mut

This comment has been minimized.

Show comment
Hide comment
@minecrawler

minecrawler Oct 24, 2017

Contributor

I guess it's not. It was, when I put all the movement on Camera in the beginning. Should I remove it?
@minecrawler

minecrawler Oct 24, 2017

Contributor

I guess it's not. It was, when I put all the movement on Camera in the beginning. Should I remove it?

This comment has been minimized.

Show comment
Hide comment
@Rhuagh

Rhuagh Oct 24, 2017

Member

Are we even using scene? I think it might be dead code tbh, so ignore it
@Rhuagh

Rhuagh Oct 24, 2017

Member

Are we even using scene? I think it might be dead code tbh, so ignore it

This comment has been minimized.

Show comment
Hide comment
@Rhuagh

Rhuagh Oct 24, 2017

Member

Umm, scene.rs don't exist in the develop branch
@Rhuagh

Rhuagh Oct 24, 2017

Member

Umm, scene.rs don't exist in the develop branch

This comment has been minimized.

Show comment
Hide comment
@Rhuagh

Rhuagh Oct 24, 2017

Member

No sorry, the file exist but it's not used. So ignore it.
@Rhuagh

Rhuagh Oct 24, 2017

Member

No sorry, the file exist but it's not used. So ignore it.
@Rhuagh

This comment has been minimized.

Show comment
Hide comment
@Rhuagh

Rhuagh Oct 24, 2017

Member

r? @Aceeri @Xaeroxe @torkleyy
Member

Rhuagh commented Oct 24, 2017

r? @Aceeri @Xaeroxe @torkleyy
@Rhuagh

This comment has been minimized.

Show comment
Hide comment
@Rhuagh

Rhuagh Oct 24, 2017

Member

I created an issue for the dead code
Member

Rhuagh commented Oct 24, 2017

I created an issue for the dead code
@Rhuagh

Rhuagh approved these changes Oct 24, 2017
View changes

@torkleyy

torkleyy reviewed Oct 24, 2017
View changes

Otherwise this is great 👍

amethyst_core/src/transform/components/local_transform.rs
}
impl LocalTransform {
+ /// Rotate to look at a point in space (without rolling)
+ pub fn look_at(&mut self, orientation: &Orientation, position: Vector3<f32>) -> &mut Self {

This comment has been minimized.

Show comment
Hide comment
@torkleyy

torkleyy Oct 24, 2017

Member

I'd like to note that with this PR we would expose cgmath types.
@torkleyy

torkleyy Oct 24, 2017

Member

I'd like to note that with this PR we would expose cgmath types.

This comment has been minimized.

Show comment
Hide comment
@torkleyy

torkleyy Oct 24, 2017

Member

I'm not sure if that's a good thing.
@torkleyy

torkleyy Oct 24, 2017

Member

I'm not sure if that's a good thing.

This comment has been minimized.

Show comment
Hide comment
@minecrawler

minecrawler Oct 24, 2017

Contributor

We already expose cgmath in other places as well (see the camera object as an example). However, I also think that mint would be a better idea. I've been thinking about streamlining the API with traits and Mint and expanding the functionality to global transformation. The question for me would be: Should I put in Mint types here, or do so in the other PR with the traits (if they are acceptable)?
@minecrawler

minecrawler Oct 24, 2017

Contributor

We already expose cgmath in other places as well (see the camera object as an example). However, I also think that mint would be a better idea. I've been thinking about streamlining the API with traits and Mint and expanding the functionality to global transformation. The question for me would be: Should I put in Mint types here, or do so in the other PR with the traits (if they are acceptable)?

This comment has been minimized.

Show comment
Hide comment
@torkleyy

torkleyy Oct 24, 2017

Member

I don't really mind as long as it gets done before the release.
@torkleyy

torkleyy Oct 24, 2017

Member

I don't really mind as long as it gets done before the release.
amethyst_core/src/transform/components/local_transform.rs
+ }
+
+ /// Set the position.
+ pub fn set_position(&mut self, position: Point3<f32>) -> &mut Self {

This comment has been minimized.

Show comment
Hide comment
@torkleyy

torkleyy Oct 24, 2017

Member

E.g. here I have the feeling that exposing the cgmath type just makes it more verbose.
@torkleyy

torkleyy Oct 24, 2017

Member

E.g. here I have the feeling that exposing the cgmath type just makes it more verbose.
@Rhuagh

This comment has been minimized.

Show comment
Hide comment
@Rhuagh

Rhuagh Oct 24, 2017

Member

I opened an issue for removing cgmath from the public facing APIs (#452). Can we approve this then?
Member

Rhuagh commented Oct 24, 2017

I opened an issue for removing cgmath from the public facing APIs (#452). Can we approve this then?

@Aceeri Aceeri added project: core type: improvement labels Oct 24, 2017

@Xaeroxe

Xaeroxe approved these changes Oct 25, 2017
View changes

LGTM!

bors r+

bors bot added a commit that referenced this pull request Oct 25, 2017

@bors[bot]
Merge #447 
447: Implement advanced local movement r=Xaeroxe a=minecrawler

Cleaned #342 , implements movement helpers on `LocalTransform`
Loading status checks…
a58ac8d
@bors

This comment has been minimized.

Show comment
Hide comment
@bors

bors bot Oct 25, 2017

Contributor

Build failed
Contributor

bors bot commented Oct 25, 2017

Build failed
@Rhuagh

This comment has been minimized.

Show comment
Hide comment
@Rhuagh

Rhuagh Oct 25, 2017

Member

Failed internally in appveyor on rustc download.

bors r+
Member

Rhuagh commented Oct 25, 2017

Failed internally in appveyor on rustc download.

bors r+

bors bot added a commit that referenced this pull request Oct 25, 2017

@bors[bot]
Merge #447 
447: Implement advanced local movement r=Rhuagh a=minecrawler

Cleaned #342 , implements movement helpers on `LocalTransform`
Loading status checks…
edbe376
@bors

This comment has been minimized.

Show comment
Hide comment
@bors

bors bot Oct 25, 2017

Contributor

Build succeeded
Contributor

bors bot commented Oct 25, 2017

Build succeeded

@bors bors bot merged commit 3de77f9 into amethyst:develop Oct 25, 2017
3 checks passed

3 checks passed

bors Build succeeded
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@Xaeroxe Xaeroxe referenced this pull request Nov 8, 2017

Closed

Question: more functionality on specs Camera #176

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment