/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

Refactoring Camera as a Component #440

Merged
merged 2 commits into from Oct 23, 2017

Conversation

Reviewers

@omni-viral omni-viral

@Aceeri Aceeri

@Xaeroxe Xaeroxe

@torkleyy torkleyy

Assignees

@Aceeri Aceeri

Labels
Projects
None yet
Milestone
No milestone
6 participants
@Rhuagh @minecrawler @omni-viral @Aceeri @Xaeroxe @torkleyy
@Rhuagh
Member

Rhuagh commented Oct 21, 2017

Render pass use Transform of Camera entity to compute the view matrix.
@torkleyy

torkleyy approved these changes Oct 21, 2017
View changes

..and is great, too!

amethyst_renderer/src/pass/flat/interleaved.rs
@@ -138,7 +140,8 @@ where
..
} = self;
- let camera = &camera;
+ // TODO: multiple cameras
+ let camera = (&camera, &global).join().next();

This comment has been minimized.

Show comment
Hide comment
@torkleyy

torkleyy Oct 21, 2017

Member

Maybe log an error in case next().is_some()
@torkleyy

torkleyy Oct 21, 2017

Member

Maybe log an error in case next().is_some()

This comment has been minimized.

Show comment
Hide comment
@torkleyy

torkleyy Oct 21, 2017

Member

But then again I'm not sure if we should do this everywhere
@torkleyy

torkleyy Oct 21, 2017

Member

But then again I'm not sure if we should do this everywhere

@torkleyy torkleyy added project: core status: ready type: improvement labels Oct 21, 2017

@torkleyy torkleyy assigned Aceeri Oct 21, 2017

@minecrawler

This comment has been minimized.

Show comment
Hide comment
@minecrawler

minecrawler Oct 21, 2017

Contributor

Hey, that's half my PR, however you cleared up one of my major pain points, so that's really awesome :D

  1. If possible, could you add the ActiveCamera component we talked about over there, so multiple cameras are possible? Only always take the first ActiveCamera!
  2. Shouldn't a camera have both, Transform and LocalTransform, for example in case you want the camera to follow a mesh? (I hope I got the purpose of LocalTransform right this time)
Contributor

minecrawler commented Oct 21, 2017

Hey, that's half my PR, however you cleared up one of my major pain points, so that's really awesome :D

  1. If possible, could you add the ActiveCamera component we talked about over there, so multiple cameras are possible? Only always take the first ActiveCamera!
  2. Shouldn't a camera have both, Transform and LocalTransform, for example in case you want the camera to follow a mesh? (I hope I got the purpose of LocalTransform right this time)
@Rhuagh

This comment has been minimized.

Show comment
Hide comment
@Rhuagh

Rhuagh Oct 21, 2017

Member

For 2. That's up to the user, a LocalTransform will always be converted to a Transform before arriving in the renderer, this is done by Transformsystem.
Member

Rhuagh commented Oct 21, 2017

For 2. That's up to the user, a LocalTransform will always be converted to a Transform before arriving in the renderer, this is done by Transformsystem.
@Rhuagh

This comment has been minimized.

Show comment
Hide comment
@Rhuagh

Rhuagh Oct 21, 2017

Member

For 1. I'm still debating solutions to this problem
Member

Rhuagh commented Oct 21, 2017

For 1. I'm still debating solutions to this problem
@omni-viral

omni-viral approved these changes Oct 21, 2017
View changes

Nice!

@omni-viral

This comment has been minimized.

Show comment
Hide comment
@omni-viral

omni-viral Oct 21, 2017

Member

ActiveCamera as optional resource is better. Otherwise changing active camera will have more boilerplate.
Member

omni-viral commented Oct 21, 2017

ActiveCamera as optional resource is better. Otherwise changing active camera will have more boilerplate.
@Rhuagh

This comment has been minimized.

Show comment
Hide comment
@Rhuagh

Rhuagh Oct 21, 2017

Member

I think I'm gonna add ActiveViewport in a later PR
Member

Rhuagh commented Oct 21, 2017

I think I'm gonna add ActiveViewport in a later PR
@Rhuagh

This comment has been minimized.

Show comment
Hide comment
@Rhuagh

Rhuagh Oct 21, 2017

Member

I will add the active camera resource for now, as optional, fallback to first camera if its not set, and fallback to identity from that
Member

Rhuagh commented Oct 21, 2017

I will add the active camera resource for now, as optional, fallback to first camera if its not set, and fallback to identity from that
amethyst_renderer/src/pass/flat/interleaved.rs
+ .and_then(|a| {
+ let cam = camera.get(a.entity);
+ let transform = global.get(a.entity);
+ cam.into_iter().zip(transform.into_iter()).next()

This comment has been minimized.

Show comment
Hide comment
@omni-viral

omni-viral Oct 21, 2017

Member

This is unusual but good alternative to cam.and_then(|cam| transform.map(|transform| (cam, transform))) 😄
@omni-viral

omni-viral Oct 21, 2017

Member

This is unusual but good alternative to cam.and_then(|cam| transform.map(|transform| (cam, transform))) 😄

This comment has been minimized.

Show comment
Hide comment
@Rhuagh

Rhuagh Oct 22, 2017

Member

That one doesn't work because of borrow rules.
@Rhuagh

Rhuagh Oct 22, 2017

Member

That one doesn't work because of borrow rules.
@Xaeroxe

Xaeroxe approved these changes Oct 21, 2017
View changes

I love it! This was how it should have been all along imo.

@Rhuagh

This comment has been minimized.

Show comment
Hide comment
@Rhuagh

Rhuagh Oct 22, 2017

Member

r? @Aceeri
Member

Rhuagh commented Oct 22, 2017

r? @Aceeri

Rhuagh added some commits Oct 21, 2017

@Rhuagh
Refactoring Camera as a Component. Render pass use Transform of Camer… 
…a entity to compute the view matrix.
bf4ae51
@Rhuagh
Add as an optional resources to control which camera to use for rende… 
…ring. Will fallback to the first camera in the component list if the resource does not exist.
Loading status checks…
475b692
@Aceeri

Aceeri approved these changes Oct 23, 2017
View changes

@Rhuagh

This comment has been minimized.

Show comment
Hide comment
@Rhuagh

Rhuagh Oct 23, 2017

Member

bors r+
Member

Rhuagh commented Oct 23, 2017

bors r+

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

@bors[bot]
Merge #440 
440: Refactoring Camera as a Component r=Rhuagh a=Rhuagh

 Render pass use Transform of Camera entity to compute the view matrix.
Loading status checks…
30ccdd8
@bors

This comment has been minimized.

Show comment
Hide comment
@bors

bors bot Oct 23, 2017

Contributor

Build succeeded
Contributor

bors bot commented Oct 23, 2017

Build succeeded

@bors bors bot merged commit 475b692 into amethyst:develop Oct 23, 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

@Rhuagh Rhuagh deleted the Rhuagh:refactor-camera branch Oct 23, 2017

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