[Ready] Fly camera!

[jojolepro]

Explore the world with that fancy camera!
Wow, much rotation!

Jokes aside, this adds a fly camera, an example that shows how to use it, and some useful functions to change the cursor behaviour.

If someone manages to add a constraint to rotation in a PR, this could use it to prevent the camera from going upside down.

Also fixes a bug in transform::move_local that wasn't actually taking rotation into account.

---

This change is 

[jojolepro]

Forgot to format, will take a while for me to update rustfmt and rust.

[jojolepro]

Forgot to format, will take a while for me to update rustfmt and rust.

[jojolepro]

formatted, ready for review

[jojolepro]

formatted, ready for review

[Rhuagh]

space after ,

[jojolepro]

done

[Rhuagh]

Remove comment.

[jojolepro]

done

[Rhuagh]

don't use specs-stuff in function signatures where not needed, you could take &InputHandler..> here. Also, this removes the ability to use a different type for the Axis/Action in InputHandler, we should not force <String, String> in our core functionality

[jojolepro]

done

[Rhuagh]

Why two separate systems?

[jojolepro]

More system = more parallelisation = better performance + ideomatic?
Also its a separation of features, you might want to use the camera rotation without the movement of it (first person camera, kind of)

[jojolepro]

I think this is what should have been done everywhere in fact. A ton of small systems instead of a couple of big ones.

[Rhuagh]

Newlines. here and below.

[jojolepro]

done

[Rhuagh]

Incorrect doc.

[jojolepro]

done

[Rhuagh]

Should provide a bundle for adding a fly camera.

[jojolepro]

done

[torkleyy]

Reviewed 6 of 10 files at r1, 5 of 7 files at r2.
Review status: 11 of 13 files reviewed at latest revision, 17 unresolved discussions.

---

amethyst_core/src/transform/components/local_transform.rs, line 57 at r2 (raw file):

    #[inline]
    pub fn move_local(&mut self, axis: Vector3<f32>, amount: f32) -> &mut Self {
        if axis.magnitude() == 0.0 {

Use squared magnitude?

---

amethyst_utils/src/fly_camera.rs, line 13 at r2 (raw file):

    speed: f32,
    /// The name of the input axis to locally move in the x coordinates.
    move_x_name: Option<&'a str>,

Should be generic over the axis key of the input manager.x

---

amethyst_utils/src/fly_camera.rs, line 36 at r2 (raw file):

    fn get_axis(name: Option<&'a str>, input: &Fetch<InputHandler<String, String>>) -> f32 {
        if let Some(n) = name {

name.and_then(|n| input.axis_value(n)).unwrap_or(0.0)

---

amethyst_utils/src/fly_camera.rs, line 41 at r2 (raw file):

            }
        }
        return 0.0;

No need for an explicit return

---

amethyst_utils/src/fly_camera.rs, line 60 at r2 (raw file):

        let dir = Vector3::new(x, y, z);

        for (_, transform) in (&camera, &mut transform).join() {

Do we want to move all cameras? I'd say pass an entity to the system.

---

amethyst_utils/src/fly_camera.rs, line 67 at r2 (raw file):

Previously, jojolepro wrote…

I think this is what should have been done everywhere in fact. A ton of small systems instead of a couple of big ones.

Yeah, usually splitting makes sense, but here both will block each other since they need access to WriteStorage<Transform>

---

amethyst_utils/src/lib.rs, line 2 at r2 (raw file):

extern crate amethyst_core;
extern crate amethyst_input;

I'm unsure if this fly_camera should go into amethyst_utils. I could imagine amethyst_input or amethyst_renderer depending on utils in the future. A recursive dependency is not permitted however. We need to consider putting this feature into another crate.

---

amethyst_utils/src/mouse.rs, line 21 at r2 (raw file):

    msg.send_command(move |win| {
        if let Err(err) = win.set_cursor_state(state) {
            eprintln!("Unable to change the cursor state! Error: {:?}", err);

Should use error!

---

amethyst_utils/src/mouse.rs, line 37 at r2 (raw file):

/// The system that locks the mouse to the center of the screen. Useful for first person camera.
pub struct MouseCenterLockSystem;

In my experience this kind of manually setting the mouse position to the middle of the screen doesn't go well. What does this offer over MouseCursor::Grab?

---

examples/fly_camera/README.md, line 1 at r2 (raw file):

# Fly Camera Example

Missing description

---

examples/fly_camera/resources/input.ron, line 18 at r2 (raw file):

    actions: {
    },
)

Missing newline

---

Comments from Reviewable

[torkleyy]

Reviewed 6 of 10 files at r1, 5 of 7 files at r2.
Review status: 11 of 13 files reviewed at latest revision, 17 unresolved discussions.

---

amethyst_core/src/transform/components/local_transform.rs, line 57 at r2 (raw file):

    #[inline]
    pub fn move_local(&mut self, axis: Vector3<f32>, amount: f32) -> &mut Self {
        if axis.magnitude() == 0.0 {

Use squared magnitude?

---

amethyst_utils/src/fly_camera.rs, line 13 at r2 (raw file):

    speed: f32,
    /// The name of the input axis to locally move in the x coordinates.
    move_x_name: Option<&'a str>,

Should be generic over the axis key of the input manager.x

---

amethyst_utils/src/fly_camera.rs, line 36 at r2 (raw file):

    fn get_axis(name: Option<&'a str>, input: &Fetch<InputHandler<String, String>>) -> f32 {
        if let Some(n) = name {

name.and_then(|n| input.axis_value(n)).unwrap_or(0.0)

---

amethyst_utils/src/fly_camera.rs, line 41 at r2 (raw file):

            }
        }
        return 0.0;

No need for an explicit return

---

amethyst_utils/src/fly_camera.rs, line 60 at r2 (raw file):

        let dir = Vector3::new(x, y, z);

        for (_, transform) in (&camera, &mut transform).join() {

Do we want to move all cameras? I'd say pass an entity to the system.

---

amethyst_utils/src/fly_camera.rs, line 67 at r2 (raw file):

Previously, jojolepro wrote…

I think this is what should have been done everywhere in fact. A ton of small systems instead of a couple of big ones.

Yeah, usually splitting makes sense, but here both will block each other since they need access to WriteStorage<Transform>

---

amethyst_utils/src/lib.rs, line 2 at r2 (raw file):

extern crate amethyst_core;
extern crate amethyst_input;

I'm unsure if this fly_camera should go into amethyst_utils. I could imagine amethyst_input or amethyst_renderer depending on utils in the future. A recursive dependency is not permitted however. We need to consider putting this feature into another crate.

---

amethyst_utils/src/mouse.rs, line 21 at r2 (raw file):

    msg.send_command(move |win| {
        if let Err(err) = win.set_cursor_state(state) {
            eprintln!("Unable to change the cursor state! Error: {:?}", err);

Should use error!

---

amethyst_utils/src/mouse.rs, line 37 at r2 (raw file):

/// The system that locks the mouse to the center of the screen. Useful for first person camera.
pub struct MouseCenterLockSystem;

In my experience this kind of manually setting the mouse position to the middle of the screen doesn't go well. What does this offer over MouseCursor::Grab?

---

examples/fly_camera/README.md, line 1 at r2 (raw file):

# Fly Camera Example

Missing description

---

examples/fly_camera/resources/input.ron, line 18 at r2 (raw file):

    actions: {
    },
)

Missing newline

---

Comments from Reviewable

[jojolepro]

amethyst_utils/src/mouse.rs, line 37 at r2 (raw file):

/// The system that locks the mouse to the center of the screen. Useful for first person camera.
pub struct MouseCenterLockSystem;
In my experience this kind of manually setting the mouse position to the middle of the screen doesn't go >well. What does this offer over MouseCursor::Grab?

MouseGrab locks the mouse inside of the windows, but doesn't force it to be centered.

amethyst_utils/src/fly_camera.rs, line 67 at r2 (raw file):

Previously, jojolepro wrote…
I think this is what should have been done everywhere in fact. A ton of small systems instead of a >couple of big ones.

Yeah, usually splitting makes sense, but here both will block each other since they need access to >WriteStorage

I still prefer having System separated. Even if both block each other on the Transform, some other systems will have access to resources that become free after the first system is done. ScreenDimensions is an example.

I fixed everything else in both reviews. Thanks for reviewing! :) 💯

[jojolepro]

amethyst_utils/src/mouse.rs, line 37 at r2 (raw file):

/// The system that locks the mouse to the center of the screen. Useful for first person camera.
pub struct MouseCenterLockSystem;
In my experience this kind of manually setting the mouse position to the middle of the screen doesn't go >well. What does this offer over MouseCursor::Grab?

MouseGrab locks the mouse inside of the windows, but doesn't force it to be centered.

amethyst_utils/src/fly_camera.rs, line 67 at r2 (raw file):

Previously, jojolepro wrote…
I think this is what should have been done everywhere in fact. A ton of small systems instead of a >couple of big ones.

Yeah, usually splitting makes sense, but here both will block each other since they need access to >WriteStorage

I still prefer having System separated. Even if both block each other on the Transform, some other systems will have access to resources that become free after the first system is done. ScreenDimensions is an example.

I fixed everything else in both reviews. Thanks for reviewing! :) 💯

[torkleyy]

Thanks for addressing. Please make sure to comment "Done." (shortcut key: "d") to my review comments so I can acknowledge them.

---

Reviewed 4 of 10 files at r3.
Review status: 10 of 16 files reviewed at latest revision, 24 unresolved discussions.

---

amethyst_renderer/src/bundle.rs, line 50 at r3 (raw file):

    ) -> Result<DispatcherBuilder<'a, 'b>> {
        world.add_resource(AmbientColor(Rgba::from([0.01; 3])));
        if !world.res.has_value(ResourceId::new::<WindowMessages>()) {

Should use Entry API.

---

amethyst_utils/src/bundles.rs, line 58 at r3 (raw file):

    ) -> Result<DispatcherBuilder<'a, 'b>> {
        world.register::<FlyCameraTag>();
        if !world.res.has_value(ResourceId::new::<WindowMessages>()) {

Should use Entry API.

---

amethyst_utils/src/bundles.rs, line 63 at r3 (raw file):

        let mut msg = world.write_resource::<WindowMessages>();
        grab_cursor(&mut msg);
        set_mouse_cursor_none(&mut msg);

Should be in the bundle documentation.

---

amethyst_utils/src/fly_camera.rs, line 15 at r3 (raw file):

impl Component for FlyCameraTag {
    type Storage = VecStorage<FlyCameraTag>;

Should use a NullStorage.

---

amethyst_utils/src/fly_camera.rs, line 20 at r3 (raw file):

/// The system that manages the camera movement.
/// Generic parameters are the parameters for the InputHandler.
pub struct FlyCameraMovementSystem<A, B> {

Why do we need B here?

---

amethyst_utils/src/fly_camera.rs, line 22 at r3 (raw file):

pub struct FlyCameraMovementSystem<A, B> {
    /// The movement speed of the camera in units per second.
    pub speed: f32,

You cannot access fields of a system unfortunately.

---

amethyst_utils/src/fly_camera.rs, line 121 at r3 (raw file):

            let offset_x = half_x - posx as f32;
            let offset_y = half_y - posy as f32;
            for (transform, _) in (&mut transform, &tag).join() {

Still, do we want to fly all of them at once?

---

Comments from Reviewable

[torkleyy]

Thanks for addressing. Please make sure to comment "Done." (shortcut key: "d") to my review comments so I can acknowledge them.

---

Reviewed 4 of 10 files at r3.
Review status: 10 of 16 files reviewed at latest revision, 24 unresolved discussions.

---

amethyst_renderer/src/bundle.rs, line 50 at r3 (raw file):

    ) -> Result<DispatcherBuilder<'a, 'b>> {
        world.add_resource(AmbientColor(Rgba::from([0.01; 3])));
        if !world.res.has_value(ResourceId::new::<WindowMessages>()) {

Should use Entry API.

---

amethyst_utils/src/bundles.rs, line 58 at r3 (raw file):

    ) -> Result<DispatcherBuilder<'a, 'b>> {
        world.register::<FlyCameraTag>();
        if !world.res.has_value(ResourceId::new::<WindowMessages>()) {

Should use Entry API.

---

amethyst_utils/src/bundles.rs, line 63 at r3 (raw file):

        let mut msg = world.write_resource::<WindowMessages>();
        grab_cursor(&mut msg);
        set_mouse_cursor_none(&mut msg);

Should be in the bundle documentation.

---

amethyst_utils/src/fly_camera.rs, line 15 at r3 (raw file):

impl Component for FlyCameraTag {
    type Storage = VecStorage<FlyCameraTag>;

Should use a NullStorage.

---

amethyst_utils/src/fly_camera.rs, line 20 at r3 (raw file):

/// The system that manages the camera movement.
/// Generic parameters are the parameters for the InputHandler.
pub struct FlyCameraMovementSystem<A, B> {

Why do we need B here?

---

amethyst_utils/src/fly_camera.rs, line 22 at r3 (raw file):

pub struct FlyCameraMovementSystem<A, B> {
    /// The movement speed of the camera in units per second.
    pub speed: f32,

You cannot access fields of a system unfortunately.

---

amethyst_utils/src/fly_camera.rs, line 121 at r3 (raw file):

            let offset_x = half_x - posx as f32;
            let offset_y = half_y - posy as f32;
            for (transform, _) in (&mut transform, &tag).join() {

Still, do we want to fly all of them at once?

---

Comments from Reviewable

[jojolepro]

Review status: 10 of 16 files reviewed at latest revision, 24 unresolved discussions.

---

amethyst_core/src/transform/components/local_transform.rs, line 57 at r2 (raw file):

Previously, torkleyy (Thomas Schaller) wrote…

Use squared magnitude?

Done.

---

amethyst_utils/src/fly_camera.rs, line 13 at r2 (raw file):

Previously, torkleyy (Thomas Schaller) wrote…

Should be generic over the axis key of the input manager.x

Done.

---

amethyst_utils/src/fly_camera.rs, line 35 at r2 (raw file):

Previously, jojolepro wrote…

done

Done.

---

amethyst_utils/src/fly_camera.rs, line 36 at r2 (raw file):

Previously, torkleyy (Thomas Schaller) wrote…

name.and_then(|n| input.axis_value(n)).unwrap_or(0.0)

Done.

---

amethyst_utils/src/fly_camera.rs, line 41 at r2 (raw file):

Previously, torkleyy (Thomas Schaller) wrote…

No need for an explicit return

Done.

---

amethyst_utils/src/fly_camera.rs, line 60 at r2 (raw file):

Previously, torkleyy (Thomas Schaller) wrote…

Do we want to move all cameras? I'd say pass an entity to the system.

Done.

---

amethyst_utils/src/fly_camera.rs, line 67 at r2 (raw file):

Previously, torkleyy (Thomas Schaller) wrote…

Yeah, usually splitting makes sense, but here both will block each other since they need access to WriteStorage<Transform>

replied as comment

---

amethyst_utils/src/fly_camera.rs, line 15 at r3 (raw file):

Previously, torkleyy (Thomas Schaller) wrote…

Should use a NullStorage.

Done.

---

amethyst_utils/src/fly_camera.rs, line 20 at r3 (raw file):

Previously, torkleyy (Thomas Schaller) wrote…

Why do we need B here?

Second type argument of InputHandler<A,B>. I did put a doc line about it.

---

amethyst_utils/src/fly_camera.rs, line 22 at r3 (raw file):

Previously, torkleyy (Thomas Schaller) wrote…

You cannot access fields of a system unfortunately.

Done.

---

amethyst_utils/src/fly_camera.rs, line 121 at r3 (raw file):

Previously, torkleyy (Thomas Schaller) wrote…

Still, do we want to fly all of them at once?

Only the one tagged will. Its up to the user to specify which entities have that tag.

---

amethyst_utils/src/lib.rs, line 2 at r2 (raw file):

Previously, torkleyy (Thomas Schaller) wrote…

I'm unsure if this fly_camera should go into amethyst_utils. I could imagine amethyst_input or amethyst_renderer depending on utils in the future. A recursive dependency is not permitted however. We need to consider putting this feature into another crate.

It could be move if we ever get a cycle dependency.

---

amethyst_utils/src/mouse.rs, line 21 at r2 (raw file):

Previously, torkleyy (Thomas Schaller) wrote…

Should use error!

Done.

---

amethyst_utils/src/mouse.rs, line 37 at r2 (raw file):

Previously, torkleyy (Thomas Schaller) wrote…

In my experience this kind of manually setting the mouse position to the middle of the screen doesn't go well. What does this offer over MouseCursor::Grab?

replied as comment

---

examples/fly_camera/README.md, line 1 at r2 (raw file):

Previously, torkleyy (Thomas Schaller) wrote…

Missing description

Done.

---

examples/fly_camera/resources/input.ron, line 18 at r2 (raw file):

Previously, torkleyy (Thomas Schaller) wrote…

Missing newline

Done.

---

Comments from Reviewable

[jojolepro]

Review status: 10 of 16 files reviewed at latest revision, 24 unresolved discussions.

---

amethyst_core/src/transform/components/local_transform.rs, line 57 at r2 (raw file):

Previously, torkleyy (Thomas Schaller) wrote…

Use squared magnitude?

Done.

---

amethyst_utils/src/fly_camera.rs, line 13 at r2 (raw file):

Previously, torkleyy (Thomas Schaller) wrote…

Should be generic over the axis key of the input manager.x

Done.

---

amethyst_utils/src/fly_camera.rs, line 35 at r2 (raw file):

Previously, jojolepro wrote…

done

Done.

---

amethyst_utils/src/fly_camera.rs, line 36 at r2 (raw file):

Previously, torkleyy (Thomas Schaller) wrote…

name.and_then(|n| input.axis_value(n)).unwrap_or(0.0)

Done.

---

amethyst_utils/src/fly_camera.rs, line 41 at r2 (raw file):

Previously, torkleyy (Thomas Schaller) wrote…

No need for an explicit return

Done.

---

amethyst_utils/src/fly_camera.rs, line 60 at r2 (raw file):

Previously, torkleyy (Thomas Schaller) wrote…

Do we want to move all cameras? I'd say pass an entity to the system.

Done.

---

amethyst_utils/src/fly_camera.rs, line 67 at r2 (raw file):

Previously, torkleyy (Thomas Schaller) wrote…

Yeah, usually splitting makes sense, but here both will block each other since they need access to WriteStorage<Transform>

replied as comment

---

amethyst_utils/src/fly_camera.rs, line 15 at r3 (raw file):

Previously, torkleyy (Thomas Schaller) wrote…

Should use a NullStorage.

Done.

---

amethyst_utils/src/fly_camera.rs, line 20 at r3 (raw file):

Previously, torkleyy (Thomas Schaller) wrote…

Why do we need B here?

Second type argument of InputHandler<A,B>. I did put a doc line about it.

---

amethyst_utils/src/fly_camera.rs, line 22 at r3 (raw file):

Previously, torkleyy (Thomas Schaller) wrote…

You cannot access fields of a system unfortunately.

Done.

---

amethyst_utils/src/fly_camera.rs, line 121 at r3 (raw file):

Previously, torkleyy (Thomas Schaller) wrote…

Still, do we want to fly all of them at once?

Only the one tagged will. Its up to the user to specify which entities have that tag.

---

amethyst_utils/src/lib.rs, line 2 at r2 (raw file):

Previously, torkleyy (Thomas Schaller) wrote…

I'm unsure if this fly_camera should go into amethyst_utils. I could imagine amethyst_input or amethyst_renderer depending on utils in the future. A recursive dependency is not permitted however. We need to consider putting this feature into another crate.

It could be move if we ever get a cycle dependency.

---

amethyst_utils/src/mouse.rs, line 21 at r2 (raw file):

Previously, torkleyy (Thomas Schaller) wrote…

Should use error!

Done.

---

amethyst_utils/src/mouse.rs, line 37 at r2 (raw file):

Previously, torkleyy (Thomas Schaller) wrote…

In my experience this kind of manually setting the mouse position to the middle of the screen doesn't go well. What does this offer over MouseCursor::Grab?

replied as comment

---

examples/fly_camera/README.md, line 1 at r2 (raw file):

Previously, torkleyy (Thomas Schaller) wrote…

Missing description

Done.

---

examples/fly_camera/resources/input.ron, line 18 at r2 (raw file):

Previously, torkleyy (Thomas Schaller) wrote…

Missing newline

Done.

---

Comments from Reviewable

[jojolepro]

Review status: 9 of 16 files reviewed at latest revision, 24 unresolved discussions.

---

amethyst_renderer/src/bundle.rs, line 50 at r3 (raw file):

Previously, torkleyy (Thomas Schaller) wrote…

Should use Entry API.

Done.

---

amethyst_utils/src/bundles.rs, line 58 at r3 (raw file):

Previously, torkleyy (Thomas Schaller) wrote…

Should use Entry API.

Done.

---

amethyst_utils/src/bundles.rs, line 63 at r3 (raw file):

Previously, torkleyy (Thomas Schaller) wrote…

Should be in the bundle documentation.

Done.

---

Comments from Reviewable

[jojolepro]

Review status: 9 of 16 files reviewed at latest revision, 24 unresolved discussions.

---

amethyst_renderer/src/bundle.rs, line 50 at r3 (raw file):

Previously, torkleyy (Thomas Schaller) wrote…

Should use Entry API.

Done.

---

amethyst_utils/src/bundles.rs, line 58 at r3 (raw file):

Previously, torkleyy (Thomas Schaller) wrote…

Should use Entry API.

Done.

---

amethyst_utils/src/bundles.rs, line 63 at r3 (raw file):

Previously, torkleyy (Thomas Schaller) wrote…

Should be in the bundle documentation.

Done.

---

Comments from Reviewable

[jojolepro]

Fixed all. Thanks for review ;)
Nice Entry api, by the way.

[jojolepro]

Fixed all. Thanks for review ;)
Nice Entry api, by the way.

[torkleyy]

Reviewed 1 of 10 files at r3, 1 of 3 files at r4.
Review status: 11 of 16 files reviewed at latest revision, 11 unresolved discussions.

---

amethyst_utils/src/fly_camera.rs, line 20 at r3 (raw file):

Previously, jojolepro wrote…

Second type argument of InputHandler<A,B>. I did put a doc line about it.

But you never store B, you can simply put the type param on the methods.

---

amethyst_utils/src/fly_camera.rs, line 121 at r3 (raw file):

Previously, jojolepro wrote…

Only the one tagged will. Its up to the user to specify which entities have that tag.

Problem is if multiple entities have that tag.

---

amethyst_utils/src/lib.rs, line 2 at r2 (raw file):

Previously, jojolepro wrote…

It could be move if we ever get a cycle dependency.

Still, I don't think this is the right place.

---

examples/fly_camera/resources/input.ron, line 18 at r2 (raw file):

Previously, jojolepro wrote…

Done.

Err.. I don't think so.

---

Comments from Reviewable

[torkleyy]

Reviewed 1 of 10 files at r3, 1 of 3 files at r4.
Review status: 11 of 16 files reviewed at latest revision, 11 unresolved discussions.

---

amethyst_utils/src/fly_camera.rs, line 20 at r3 (raw file):

Previously, jojolepro wrote…

Second type argument of InputHandler<A,B>. I did put a doc line about it.

But you never store B, you can simply put the type param on the methods.

---

amethyst_utils/src/fly_camera.rs, line 121 at r3 (raw file):

Previously, jojolepro wrote…

Only the one tagged will. Its up to the user to specify which entities have that tag.

Problem is if multiple entities have that tag.

---

amethyst_utils/src/lib.rs, line 2 at r2 (raw file):

Previously, jojolepro wrote…

It could be move if we ever get a cycle dependency.

Still, I don't think this is the right place.

---

examples/fly_camera/resources/input.ron, line 18 at r2 (raw file):

Previously, jojolepro wrote…

Done.

Err.. I don't think so.

---

Comments from Reviewable

[jojolepro]

Review status: 11 of 16 files reviewed at latest revision, 11 unresolved discussions.

---

amethyst_utils/src/fly_camera.rs, line 20 at r3 (raw file):

Previously, torkleyy (Thomas Schaller) wrote…

But you never store B, you can simply put the type param on the methods.

How do you want to do that? I need to keep the B generic parameter all the way from the user's main.rs file to be able to fetch the right InputHandler<A,B>. I figured storing it as a PhantomData would do just that?

---

amethyst_utils/src/fly_camera.rs, line 121 at r3 (raw file):

Previously, torkleyy (Thomas Schaller) wrote…

Problem is if multiple entities have that tag.

1. Users might want to have multiple fly camera. You have to keep in mind that someone might want to make a remote turret by only using the fly camera's rotation system. In that case, it might be useful to use tags.
2. If the user only wants one fly camera, then only add one tag? The systems are changing (transform,tag), they completely ignore the Camera component.

---

amethyst_utils/src/lib.rs, line 2 at r2 (raw file):

Previously, torkleyy (Thomas Schaller) wrote…

Still, I don't think this is the right place.

Where do you want it then?

---

examples/fly_camera/resources/input.ron, line 18 at r2 (raw file):

Previously, torkleyy (Thomas Schaller) wrote…

Err.. I don't think so.

blame rustfmt

---

Comments from Reviewable

[jojolepro]

Review status: 11 of 16 files reviewed at latest revision, 11 unresolved discussions.

---

amethyst_utils/src/fly_camera.rs, line 20 at r3 (raw file):

Previously, torkleyy (Thomas Schaller) wrote…

But you never store B, you can simply put the type param on the methods.

How do you want to do that? I need to keep the B generic parameter all the way from the user's main.rs file to be able to fetch the right InputHandler<A,B>. I figured storing it as a PhantomData would do just that?

---

amethyst_utils/src/fly_camera.rs, line 121 at r3 (raw file):

Previously, torkleyy (Thomas Schaller) wrote…

Problem is if multiple entities have that tag.

1. Users might want to have multiple fly camera. You have to keep in mind that someone might want to make a remote turret by only using the fly camera's rotation system. In that case, it might be useful to use tags.
2. If the user only wants one fly camera, then only add one tag? The systems are changing (transform,tag), they completely ignore the Camera component.

---

amethyst_utils/src/lib.rs, line 2 at r2 (raw file):

Previously, torkleyy (Thomas Schaller) wrote…

Still, I don't think this is the right place.

Where do you want it then?

---

examples/fly_camera/resources/input.ron, line 18 at r2 (raw file):

Previously, torkleyy (Thomas Schaller) wrote…

Err.. I don't think so.

blame rustfmt

---

Comments from Reviewable

[Rhuagh]

Reviewed 3 of 10 files at r1, 3 of 7 files at r2, 7 of 10 files at r3, 3 of 3 files at r4.
Review status: all files reviewed at latest revision, 5 unresolved discussions.

---

amethyst_utils/src/fly_camera.rs, line 25 at r4 (raw file):

    speed: f32,
    /// The name of the input axis to locally move in the x coordinates.
    move_x_name: Option<A>,

Would it be possible to give these better names? They don't really encode movement along the global x/y/z axis, they're along the local axes. Something like right_input_axis etc?

---

Comments from Reviewable

[Rhuagh]

Reviewed 3 of 10 files at r1, 3 of 7 files at r2, 7 of 10 files at r3, 3 of 3 files at r4.
Review status: all files reviewed at latest revision, 5 unresolved discussions.

---

amethyst_utils/src/fly_camera.rs, line 25 at r4 (raw file):

    speed: f32,
    /// The name of the input axis to locally move in the x coordinates.
    move_x_name: Option<A>,

Would it be possible to give these better names? They don't really encode movement along the global x/y/z axis, they're along the local axes. Something like right_input_axis etc?

---

Comments from Reviewable

[Rhuagh]

Review status: all files reviewed at latest revision, 5 unresolved discussions.

---

amethyst_utils/src/fly_camera.rs, line 20 at r3 (raw file):

Previously, jojolepro wrote…

How do you want to do that? I need to keep the B generic parameter all the way from the user's main.rs file to be able to fetch the right InputHandler<A,B>. I figured storing it as a PhantomData would do just that?

@torkleyy He needs to have the B generic type here because the System impl below needs it to be able to Fetch<InputHandler<A, B>>

---

Comments from Reviewable

[Rhuagh]

Review status: all files reviewed at latest revision, 5 unresolved discussions.

---

amethyst_utils/src/fly_camera.rs, line 20 at r3 (raw file):

Previously, jojolepro wrote…

How do you want to do that? I need to keep the B generic parameter all the way from the user's main.rs file to be able to fetch the right InputHandler<A,B>. I figured storing it as a PhantomData would do just that?

@torkleyy He needs to have the B generic type here because the System impl below needs it to be able to Fetch<InputHandler<A, B>>

---

Comments from Reviewable

[Xaeroxe]

Review status: all files reviewed at latest revision, 6 unresolved discussions.

---

examples/fly_camera/main.rs, line 33 at r4 (raw file):

        //trans.scale = [1.0; 3].into();

        //trans.move_local(Vector3::new(1.0,0.0,-1.0),5.0);

Please don't leave commented out code in the PR, at least not without a stated reason. It comes off looking sloppy.

---

examples/fly_camera/resources/input.ron, line 18 at r2 (raw file):

Previously, jojolepro wrote…

blame rustfmt

rustfmt doesn't touch ron files.

---

Comments from Reviewable

[Xaeroxe]

Review status: all files reviewed at latest revision, 6 unresolved discussions.

---

examples/fly_camera/main.rs, line 33 at r4 (raw file):

        //trans.scale = [1.0; 3].into();

        //trans.move_local(Vector3::new(1.0,0.0,-1.0),5.0);

Please don't leave commented out code in the PR, at least not without a stated reason. It comes off looking sloppy.

---

examples/fly_camera/resources/input.ron, line 18 at r2 (raw file):

Previously, jojolepro wrote…

blame rustfmt

rustfmt doesn't touch ron files.

---

Comments from Reviewable

[jojolepro]

Review status: 12 of 16 files reviewed at latest revision, 6 unresolved discussions.

---

amethyst_utils/src/fly_camera.rs, line 25 at r4 (raw file):

Previously, Rhuagh (Simon Rönnberg) wrote…

Would it be possible to give these better names? They don't really encode movement along the global x/y/z axis, they're along the local axes. Something like right_input_axis etc?

Done.

---

examples/fly_camera/main.rs, line 33 at r4 (raw file):

Previously, Xaeroxe (Jacob Kiesel) wrote…

Please don't leave commented out code in the PR, at least not without a stated reason. It comes off looking sloppy.

Done.

---

examples/fly_camera/resources/input.ron, line 18 at r2 (raw file):

Previously, Xaeroxe (Jacob Kiesel) wrote…

rustfmt doesn't touch ron files.

should be fixed now...

---

Comments from Reviewable

[jojolepro]

Review status: 12 of 16 files reviewed at latest revision, 6 unresolved discussions.

---

amethyst_utils/src/fly_camera.rs, line 25 at r4 (raw file):

Previously, Rhuagh (Simon Rönnberg) wrote…

Would it be possible to give these better names? They don't really encode movement along the global x/y/z axis, they're along the local axes. Something like right_input_axis etc?

Done.

---

examples/fly_camera/main.rs, line 33 at r4 (raw file):

Previously, Xaeroxe (Jacob Kiesel) wrote…

Please don't leave commented out code in the PR, at least not without a stated reason. It comes off looking sloppy.

Done.

---

examples/fly_camera/resources/input.ron, line 18 at r2 (raw file):

Previously, Xaeroxe (Jacob Kiesel) wrote…

rustfmt doesn't touch ron files.

should be fixed now...

---

Comments from Reviewable

[jojolepro]

@torkleyy reply to my answer whenever you get time please. Its been two days. Thanks!

[jojolepro]

@torkleyy reply to my answer whenever you get time please. Its been two days. Thanks!

[jojolepro]

Once people are happy with how the code is, I'll rebase.

[jojolepro]

Once people are happy with how the code is, I'll rebase.

[jojolepro]

Review status: 8 of 20 files reviewed at latest revision, 6 unresolved discussions.

---

amethyst_utils/src/lib.rs, line 2 at r2 (raw file):

Previously, jojolepro wrote…

Where do you want it then?

Done.

---

Comments from Reviewable

[jojolepro]

Review status: 8 of 20 files reviewed at latest revision, 6 unresolved discussions.

---

amethyst_utils/src/lib.rs, line 2 at r2 (raw file):

Previously, jojolepro wrote…

Where do you want it then?

Done.

---

Comments from Reviewable

[jojolepro]

Allright, I made a new crate after talking with people on gitter and thinking about it more. I makes sense to have a controls crate, since I'll be adding more control types in the future (fps, third person, vehicle, aircraft, etc..)

This should be ready for a new (and hopefully final) review.

[jojolepro]

Allright, I made a new crate after talking with people on gitter and thinking about it more. I makes sense to have a controls crate, since I'll be adding more control types in the future (fps, third person, vehicle, aircraft, etc..)

This should be ready for a new (and hopefully final) review.

[Xaeroxe]

---

Reviewed 1 of 6 files at r5, 11 of 13 files at r6, 2 of 2 files at r7.
Review status: all files reviewed at latest revision, 5 unresolved discussions.

---

Comments from Reviewable

[Xaeroxe]

---

Reviewed 1 of 6 files at r5, 11 of 13 files at r6, 2 of 2 files at r7.
Review status: all files reviewed at latest revision, 5 unresolved discussions.

---

Comments from Reviewable

[Rhuagh]

Reviewed 1 of 6 files at r5, 11 of 13 files at r6, 2 of 2 files at r7.
Review status: all files reviewed at latest revision, 6 unresolved discussions.

---

amethyst_controls/src/systems.rs, line 10 at r7 (raw file):

use std::marker::PhantomData;

use super::*;

Not a fan of glob imports, especially when it's for a single type.

---

examples/fly_camera/main.rs, line 8 at r7 (raw file):

use amethyst::assets::Loader;
use amethyst::config::Config;
use amethyst::controls::*;

Glob import.

---

examples/fly_camera/resources/input.ron, line 18 at r2 (raw file):

Previously, jojolepro wrote…

should be fixed now...

Nope

---

Comments from Reviewable

[Rhuagh]

Reviewed 1 of 6 files at r5, 11 of 13 files at r6, 2 of 2 files at r7.
Review status: all files reviewed at latest revision, 6 unresolved discussions.

---

amethyst_controls/src/systems.rs, line 10 at r7 (raw file):

use std::marker::PhantomData;

use super::*;

Not a fan of glob imports, especially when it's for a single type.

---

examples/fly_camera/main.rs, line 8 at r7 (raw file):

use amethyst::assets::Loader;
use amethyst::config::Config;
use amethyst::controls::*;

Glob import.

---

examples/fly_camera/resources/input.ron, line 18 at r2 (raw file):

Previously, jojolepro wrote…

should be fixed now...

Nope

---

Comments from Reviewable

[torkleyy]

Didn't review it a second time yet, but I like the direction and thank you for moving the code :)

[torkleyy]

Didn't review it a second time yet, but I like the direction and thank you for moving the code :)

[jojolepro]

On my computer.

On git.

Though, it shows fine in git diff, so I guess this problem is fixed.

[jojolepro]

On my computer.

On git.

Though, it shows fine in git diff, so I guess this problem is fixed.

[torkleyy]

Reviewed 1 of 12 files at r3, 9 of 13 files at r6, 1 of 2 files at r7, 4 of 4 files at r8.
Review status: all files reviewed at latest revision, 5 unresolved discussions.

---

amethyst_utils/src/lib.rs, line 2 at r2 (raw file):

Previously, jojolepro wrote…

Done.

I don't think this is done, we still depend on Amethyst crates other than amethyst_core.

---

amethyst_utils/src/mouse.rs, line 1 at r8 (raw file):

use amethyst_renderer::WindowMessages;

Move this file to amethyst_renderer?

---

examples/pong/resources/input.ron, line 15 at r8 (raw file):

    },
)

Missing newline

---

Comments from Reviewable

[torkleyy]

Reviewed 1 of 12 files at r3, 9 of 13 files at r6, 1 of 2 files at r7, 4 of 4 files at r8.
Review status: all files reviewed at latest revision, 5 unresolved discussions.

---

amethyst_utils/src/lib.rs, line 2 at r2 (raw file):

Previously, jojolepro wrote…

Done.

I don't think this is done, we still depend on Amethyst crates other than amethyst_core.

---

amethyst_utils/src/mouse.rs, line 1 at r8 (raw file):

use amethyst_renderer::WindowMessages;

Move this file to amethyst_renderer?

---

examples/pong/resources/input.ron, line 15 at r8 (raw file):

    },
)

Missing newline

---

Comments from Reviewable

[jojolepro]

Review status: 13 of 21 files reviewed at latest revision, 5 unresolved discussions.

---

amethyst_controls/src/systems.rs, line 10 at r7 (raw file):

Previously, Rhuagh (Simon Rönnberg) wrote…

Not a fan of glob imports, especially when it's for a single type.

Done. The only glob import left is in the bundle file. As this file usually touches most of the crate, I assume it is okay.

---

amethyst_renderer/src/mouse.rs, line 1 at r8 (raw file):

Previously, torkleyy (Thomas Schaller) wrote…

Move this file to amethyst_renderer?

Done.

---

amethyst_utils/src/lib.rs, line 2 at r2 (raw file):

Previously, torkleyy (Thomas Schaller) wrote…

I don't think this is done, we still depend on Amethyst crates other than amethyst_core.

Done.

---

examples/fly_camera/main.rs, line 8 at r7 (raw file):

Previously, Rhuagh (Simon Rönnberg) wrote…

Glob import.

Done.

---

examples/fly_camera/resources/input.ron, line 18 at r2 (raw file):

Previously, Rhuagh (Simon Rönnberg) wrote…

Nope

I really hope this file coming from hell is fixed now :P

---

examples/pong/resources/input.ron, line 15 at r8 (raw file):

Previously, torkleyy (Thomas Schaller) wrote…

Missing newline

Done.

---

Comments from Reviewable

[jojolepro]

Review status: 13 of 21 files reviewed at latest revision, 5 unresolved discussions.

---

amethyst_controls/src/systems.rs, line 10 at r7 (raw file):

Previously, Rhuagh (Simon Rönnberg) wrote…

Not a fan of glob imports, especially when it's for a single type.

Done. The only glob import left is in the bundle file. As this file usually touches most of the crate, I assume it is okay.

---

amethyst_renderer/src/mouse.rs, line 1 at r8 (raw file):

Previously, torkleyy (Thomas Schaller) wrote…

Move this file to amethyst_renderer?

Done.

---

amethyst_utils/src/lib.rs, line 2 at r2 (raw file):

Previously, torkleyy (Thomas Schaller) wrote…

I don't think this is done, we still depend on Amethyst crates other than amethyst_core.

Done.

---

examples/fly_camera/main.rs, line 8 at r7 (raw file):

Previously, Rhuagh (Simon Rönnberg) wrote…

Glob import.

Done.

---

examples/fly_camera/resources/input.ron, line 18 at r2 (raw file):

Previously, Rhuagh (Simon Rönnberg) wrote…

Nope

I really hope this file coming from hell is fixed now :P

---

examples/pong/resources/input.ron, line 15 at r8 (raw file):

Previously, torkleyy (Thomas Schaller) wrote…

Missing newline

Done.

---

Comments from Reviewable

[torkleyy]

Reviewed 1 of 14 files at r6, 9 of 9 files at r9.
Review status: all files reviewed at latest revision, 2 unresolved discussions.

---

Comments from Reviewable

[torkleyy]

Reviewed 1 of 14 files at r6, 9 of 9 files at r9.
Review status: all files reviewed at latest revision, 2 unresolved discussions.

---

Comments from Reviewable

[Rhuagh]

Reviewed 2 of 14 files at r6, 2 of 5 files at r8, 9 of 9 files at r9.
Review status: all files reviewed at latest revision, 2 unresolved discussions.

---

examples/fly_camera/main.rs, line 8 at r9 (raw file):

use amethyst::assets::Loader;
use amethyst::config::Config;
use amethyst::controls::{FlyControlTag,FlyControlBundle};

Space, run fmt pls

---

Comments from Reviewable

[Rhuagh]

Reviewed 2 of 14 files at r6, 2 of 5 files at r8, 9 of 9 files at r9.
Review status: all files reviewed at latest revision, 2 unresolved discussions.

---

examples/fly_camera/main.rs, line 8 at r9 (raw file):

use amethyst::assets::Loader;
use amethyst::config::Config;
use amethyst::controls::{FlyControlTag,FlyControlBundle};

Space, run fmt pls

---

Comments from Reviewable

[Rhuagh]

---

Reviewed 1 of 9 files at r9, 2 of 2 files at r10.
Review status: all files reviewed at latest revision, 2 unresolved discussions.

---

Comments from Reviewable

[Rhuagh]

---

Reviewed 1 of 9 files at r9, 2 of 2 files at r10.
Review status: all files reviewed at latest revision, 2 unresolved discussions.

---

Comments from Reviewable

[Rhuagh]

Please squash the commits a bit

[Rhuagh]

Please squash the commits a bit

[jojolepro]

I think I broke my commit message, but there you go ;)

[jojolepro]

I think I broke my commit message, but there you go ;)

[torkleyy]

Please run rustfmt and squash the commits, then merge.

bors r+

---

Reviewed 1 of 2 files at r10, 11 of 11 files at r11.
Review status: all files reviewed at latest revision, all discussions resolved.

---

Comments from Reviewable

[torkleyy]

Please run rustfmt and squash the commits, then merge.

bors r+

---

Reviewed 1 of 2 files at r10, 11 of 11 files at r11.
Review status: all files reviewed at latest revision, all discussions resolved.

---

Comments from Reviewable

[bors]

Build succeeded

• continuous-integration/appveyor/branch
• continuous-integration/travis-ci/push

[bors]

Build succeeded

• continuous-integration/appveyor/branch
• continuous-integration/travis-ci/push