/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

Feature/#593 add arc ball camera #700

Merged
merged 4 commits into from May 14, 2018
+256 −10

Conversation

Reviewers

@jojolepro jojolepro

@Rhuagh Rhuagh

@MrMinimal MrMinimal

@torkleyy torkleyy

Assignees

@jojolepro jojolepro

Labels
Projects
None yet
Milestone
No milestone
5 participants
@mnivoliez @torkleyy @Rhuagh @jojolepro @MrMinimal
@mnivoliez

mnivoliez commented May 8, 2018 •
edited by torkleyy
Edited 1 time
  • @torkleyy torkleyy edited May 8, 2018 (most recent)

Here is my work on issue #593 .
For now, I have set up a system and component checking distance between the camera and the target, the free rotation system should take care of the rotation itself.

There is at this point no test, I'll do that as soon as possible.
Also, I do not know if it's the case but in the FPS/TPS case, the camera should be child to the player and evolve in player space (in the case of TPS, we should also check for other collidable object).

This change is Reviewable

@torkleyy torkleyy added type: feature status: ready labels May 9, 2018

@torkleyy

torkleyy reviewed May 9, 2018
View changes

Thanks for your Pull Request @mnivoliez!

I've got some concerns regarding the implementation of the system. Otherwise I like it, could you please add the camera to an example (maybe a dedicated one)?

amethyst_controls/src/systems.rs
+ position = Some(target_transform.translation + new_target_to_cam);
+ }
+ }
+ if let Some(new_pos) = position {

This comment has been minimized.

Show comment
Hide comment
@torkleyy

torkleyy May 9, 2018

Member

This won't work for multiple arc ball cameras, I wonder if we should support that.
@torkleyy

torkleyy May 9, 2018

Member

This won't work for multiple arc ball cameras, I wonder if we should support that.

This comment has been minimized.

Show comment
Hide comment
@jojolepro

jojolepro May 9, 2018

Collaborator

You should only be controlling a single camera at once normally. If you want multi controller support input events will need to have a tracker of which controller/mouse does what.
@jojolepro

jojolepro May 9, 2018

Collaborator

You should only be controlling a single camera at once normally. If you want multi controller support input events will need to have a tracker of which controller/mouse does what.
amethyst_controls/src/systems.rs
+ );
+
+ fn run(&mut self, (mut transforms, tags): Self::SystemData) {
+ let mut position = None;

This comment has been minimized.

Show comment
Hide comment
@torkleyy

torkleyy May 9, 2018

Member

There's a slightly better solution that also resolves my concern below: you can use RestrictedStorage to iterate over the transforms and then set the position in the same iteration of the loop. I'll come back to you and explain in more detail later.
@torkleyy

torkleyy May 9, 2018

Member

There's a slightly better solution that also resolves my concern below: you can use RestrictedStorage to iterate over the transforms and then set the position in the same iteration of the loop. I'll come back to you and explain in more detail later.

This comment has been minimized.

Show comment
Hide comment
@mnivoliez

mnivoliez May 9, 2018

I though that it would be only one arcball camera (I got no reference about a game having several arcball camera other that split screen game). The exemple is planned, but at 1h30 am, it was difficult to explain to my partner I was still working on code ^^'
The restricted storage could be something.
@mnivoliez

mnivoliez May 9, 2018

I though that it would be only one arcball camera (I got no reference about a game having several arcball camera other that split screen game). The exemple is planned, but at 1h30 am, it was difficult to explain to my partner I was still working on code ^^'
The restricted storage could be something.

@torkleyy torkleyy added status: working status: ready and removed status: ready status: working labels May 9, 2018

@torkleyy

torkleyy reviewed May 10, 2018
View changes

Honestly I think it's not obvious enough that you also need to add the FlyControlTag. We need to document the requirements of the ArcBallCamera.

examples/arc_ball_camera/README.md
@@ -0,0 +1,2 @@
+# Fly Camera Example

This comment has been minimized.

Show comment
Hide comment
@torkleyy

torkleyy May 10, 2018

Member

Rather than a dedicated readme it would actually be more important to add it to the overview.
@torkleyy

torkleyy May 10, 2018

Member

Rather than a dedicated readme it would actually be more important to add it to the overview.
amethyst_controls/src/components.rs
@@ -9,3 +9,16 @@ pub struct FlyControlTag;
impl Component for FlyControlTag {
type Storage = NullStorage<FlyControlTag>;
}
+
+#[derive(Debug)]
+pub struct ArcBallCameraTag {

This comment has been minimized.

Show comment
Hide comment
@torkleyy

torkleyy May 10, 2018

Member

Could you also derive Clone for this component?
@torkleyy

torkleyy May 10, 2018

Member

Could you also derive Clone for this component?

@torkleyy torkleyy added status: stalled status: working and removed status: ready status: stalled labels May 12, 2018

@MrMinimal

MrMinimal suggested changes May 12, 2018
View changes

Found a couple of typos, thanks for the PR!

amethyst_controls/src/components.rs
@@ -9,3 +9,19 @@ pub struct FlyControlTag;
impl Component for FlyControlTag {
type Storage = NullStorage<FlyControlTag>;
}
+
+/// Add this to a camera to which you have already add the FlyControlTag to add a arc ball

This comment has been minimized.

Show comment
Hide comment
@MrMinimal

MrMinimal May 12, 2018

Contributor

Rephrase
@MrMinimal

MrMinimal May 12, 2018

Contributor

Rephrase

This comment has been minimized.

Show comment
Hide comment
@MrMinimal

MrMinimal May 12, 2018

Contributor

I'd go with "To add an arc ball behaviour, add this to a camera which already has the FlyControlTag added."
@MrMinimal

MrMinimal May 12, 2018

Contributor

I'd go with "To add an arc ball behaviour, add this to a camera which already has the FlyControlTag added."
amethyst_controls/src/components.rs
+
+/// Add this to a camera to which you have already add the FlyControlTag to add a arc ball
+/// behaviour to the camera.
+/// Please not that this component require the ArcBallControlSystem to work.

This comment has been minimized.

Show comment
Hide comment
@MrMinimal

MrMinimal May 12, 2018

Contributor

note, requires
@MrMinimal

MrMinimal May 12, 2018

Contributor

note, requires
amethyst_controls/src/components.rs
+
+impl Component for ArcBallControlTag {
+ // we can use HashMapStorage here because, according to the specs doc, this storage should be
+ // use when the component is use with few entity, I think there will rarely more than one

This comment has been minimized.

Show comment
Hide comment
@MrMinimal

MrMinimal May 12, 2018

Contributor

used
@MrMinimal

MrMinimal May 12, 2018

Contributor

used
amethyst_controls/src/systems.rs
+/// The system that manages the arc ball movement;
+/// In essence, the system will allign the camera with its target while keeping the distance to it
+/// and while keeping the orientation of the camera.
+/// To modify the orientation of the camera in according with the mouse input, please use the

This comment has been minimized.

Show comment
Hide comment
@MrMinimal

MrMinimal May 12, 2018

Contributor

in accordance with
@MrMinimal

MrMinimal May 12, 2018

Contributor

in accordance with
mnivoliez
Issue #593 
As a arc ball mecanism was wanted, a component indicated the target and
a system making the calculus for it was needed.

Tose two elements are now reality but need testing. One way to do so is
to provide a solid exemple of the mecanism. Also, maybe some tests could
be implemented.

Change the system to just correct distance between cam and target. The
free rotation will take care of the rest. Thanks to @jojolepro for the
heads up.

Add the example but I may not fully understand how the systems works
since it seem not to move.

Add missing systems, still no result, the scene stay still, or at least
seems to.

Get the switch back. Tomorrow I shall look into the quaternion and
determination of new position based on it.

The example now works fine.

Add better documentation.

forgot to change component name everywhere.... (name is more consistent now)

Better documentation and correct some errors in documentation

Fix typo
9a8bee2
@torkleyy

This comment has been minimized.

Show comment
Hide comment
@torkleyy

torkleyy May 12, 2018

Member

@mnivoliez Please rebase this PR onto develop, it has two conflicts.
Member

torkleyy commented May 12, 2018

@mnivoliez Please rebase this PR onto develop, it has two conflicts.

mnivoliez added some commits May 8, 2018

mnivoliez
Issue #593 
As a arc ball mecanism was wanted, a component indicated the target and
a system making the calculus for it was needed.

Tose two elements are now reality but need testing. One way to do so is
to provide a solid exemple of the mecanism. Also, maybe some tests could
be implemented.

Change the system to just correct distance between cam and target. The
free rotation will take care of the rest. Thanks to @jojolepro for the
heads up.

Add the example but I may not fully understand how the systems works
since it seem not to move.

Add missing systems, still no result, the scene stay still, or at least
seems to.

Get the switch back. Tomorrow I shall look into the quaternion and
determination of new position based on it.

The example now works fine.

Add better documentation.

forgot to change component name everywhere.... (name is more consistent now)

Better documentation and correct some errors in documentation

Fix typo
f833677
mnivoliez
Merge branch 'feature/#593-add-arc-ball-camera' of github.com:mnivoli… 
…ez/amethyst into feature/#593-add-arc-ball-camera
Loading status checks…
ff6fea9
@Rhuagh

Rhuagh approved these changes May 12, 2018
View changes

I believe this could be done using just the transform parenting system, and the free rotation system, but I'll accept it anyways for clarity.

@Rhuagh

This comment has been minimized.

Show comment
Hide comment
@Rhuagh

Rhuagh May 12, 2018

Member

If the PR is ready for merge, please change the title.
Member

Rhuagh commented May 12, 2018

If the PR is ready for merge, please change the title.
amethyst_controls/src/components.rs
@@ -9,3 +9,19 @@ pub struct FlyControlTag;
impl Component for FlyControlTag {
type Storage = NullStorage<FlyControlTag>;
}
+
+/// Add this to a camera to which you have already add the FlyControlTag to add a arc ball

This comment has been minimized.

Show comment
Hide comment
@MrMinimal

MrMinimal May 12, 2018

Contributor

I'd go with "To add an arc ball behaviour, add this to a camera which already has the FlyControlTag added."
@MrMinimal

MrMinimal May 12, 2018

Contributor

I'd go with "To add an arc ball behaviour, add this to a camera which already has the FlyControlTag added."

@mnivoliez mnivoliez changed the title from [WIP] Feature/#593 add arc ball camera to Feature/#593 add arc ball camera May 12, 2018

@Rhuagh Rhuagh added status: ready and removed status: working labels May 12, 2018

@Rhuagh

This comment has been minimized.

Show comment
Hide comment
@Rhuagh

Rhuagh May 12, 2018

Member

bors r+
Member

Rhuagh commented May 12, 2018

bors r+
@bors

This comment has been minimized.

Show comment
Hide comment
@bors

bors bot May 12, 2018

Contributor

👎 Rejected by code reviews
Contributor

bors bot commented May 12, 2018

👎 Rejected by code reviews
@MrMinimal

MrMinimal approved these changes May 12, 2018
View changes

Let's get this on the road! 😃

@Rhuagh Rhuagh assigned jojolepro May 12, 2018

@torkleyy

torkleyy approved these changes May 13, 2018
View changes

Looks good, thank you! 👍

@Rhuagh

This comment has been minimized.

Show comment
Hide comment
@Rhuagh

Rhuagh May 13, 2018

Member

@jojolepro please merge if you're ok with this.
Member

Rhuagh commented May 13, 2018

@jojolepro please merge if you're ok with this.
@torkleyy

This comment has been minimized.

Show comment
Hide comment
@torkleyy

torkleyy May 14, 2018

Member

bors r+
Member

torkleyy commented May 14, 2018

bors r+

bors bot added a commit that referenced this pull request May 14, 2018

@bors[bot] mnivoliez
Merge #700 
700: Feature/#593 add arc ball camera r=torkleyy a=mnivoliez

Here is my work on issue #593 .
For now, I have set up a system and component checking distance between the camera and the target, the free rotation system should take care of the rotation itself.

There is at this point no test, I'll do that as soon as possible.
Also, I do not know if it's the case but in the FPS/TPS case, the camera should be child to the player and evolve in player space (in the case of TPS, we should also check for other collidable object).

<!-- Reviewable:start -->
---
This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/amethyst/amethyst/700)
<!-- Reviewable:end -->


Co-authored-by: mnivoliez <mathieu.nivoliez@laposte.net>
Loading status checks…
a895416
@bors

This comment has been minimized.

Show comment
Hide comment
@bors

bors bot May 14, 2018

Contributor

Build succeeded
Contributor

bors bot commented May 14, 2018

Build succeeded

@bors bors bot merged commit 06d580a into amethyst:develop May 14, 2018
3 checks passed

3 checks passed

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

jojolepro reviewed May 14, 2018 • edited
View changes

I'm a little late but 👍
Thanks!

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