Nalgebra Migration

[dotellie]

Update:
Everything seems to be working at this point. All the examples work and I haven't found any instance of the performance being considerably worse than it was before. That is to say, this PR is very much ready at this point after a few reviews.

Previous content:
It's finally here! Well almost. Things build for the most part but not everything works yet. Some examples have been converted and work (almost) perfectly but some still need some love. Of particular note is the renderable example where the positions seem to be a bit off for some reason. While the entire engine builds, I'm not 100% sure everything was actually converted correctly, hence why the boxes aren't ticked off yet.

TODO:

• Rendering 2D
• Rendering 3D
• Rendering UI
• Loading prefabs (mostly complete although positions don't seem right)
• Animation
• Audio positioning
• Convert all examples and prefabs
• Fix the book
• Update all documentation
• Performance testing
• General regression testing

---

This change is 

[sebcrozet]

I've made a few more suggestions that fix the fly_camera and gltf demos.

Note that the remark about the new_normalize also applies to the calls to Quaternion::new_normalize though maybe you already have the guarantee that they will never be zero.

Unfortunately github does not support multi-line suggestions that you can merge automatically.

[sebcrozet]

I've made a few more suggestions that fix the fly_camera and gltf demos.

Note that the remark about the new_normalize also applies to the calls to Quaternion::new_normalize though maybe you already have the guarantee that they will never be zero.

Unfortunately github does not support multi-line suggestions that you can merge automatically.

[torkleyy]

Should we use nalgebra-glm for exampels & docs?

[torkleyy]

Should we use nalgebra-glm for exampels & docs?

[dotellie]

Everything is almost working properly now! If anyone wants to help out, the only two examples that aren't working currently is the sprites_ordered example. I have no idea why this is however as the pong examples are working without problem. (@azriel91 Perhaps you could be able to figure it out?)

[torkleyy]

@magnonellie sprites_ordered is the example that needs a rewrite anyway IIRC.

[azriel91]

yo, certainly! Shall have a go at it tomorrow

[dotellie]

@sebcrozet @jojolepro @Moxinilian @Rhuagh @torkleyy If any of you would like to review this that would be appreciated! ❤️

[jojolepro]

jojolepro-8% cargo run --example appendix_a
    Finished dev [unoptimized + debuginfo] target(s) in 0.35s                                                                                              
     Running `target/debug/examples/appendix_a`
[ERROR][amethyst_config] Failed to load config file '/shareddata/share/Prog/Rust/amethystnalgebra/examples/appendix_a/resources/config.ron': 7:19: Expected array

gltf example, puffy looks bugged (even without animation). Also the animations are still glitched when pressing space fast, not sure if that was fixed yet or not. Probably not related to this pr.

jojolepro-8% cargo run --example pong_tutorial_02 (same in 03)

[WARN][amethyst_renderer::pass::sprite::interleaved] Sprite sheet not loaded for sprite_render: `SpriteRender { sprite_sheet: Handle { id: 0 }, sprite_number: 0, flip_horizontal: true, flip_vertical: false }`.

cargo run --example sprites_ordered - where they that small before?

That's it for the tests 👍 nicely done

Most of the issues mentionned might be

1. Because you need to rebase?
2. Because they are bugs that are already present and should have an issue each

[torkleyy]

Sorry, need to use Reviewable for such big PRs.

Massive work @magnonellie! One can see you refactored this very carefully.

The only thing I've noticed so far are unresolved questions regarding nalgebra-glm:

• Why are we using it?
• Where should it be used?
• What should the examples do?

Reviewed 12 of 50 files at r1, 3 of 11 files at r2, 12 of 38 files at r5, 21 of 31 files at r7.
Reviewable status: 48 of 75 files reviewed, 5 unresolved discussions (waiting on @magnonellie and @azriel91)

---

amethyst_animation/src/transform.rs, line 51 at r7 (raw file):

            Rotation => SamplerPrimitive::Vec4({
                let c = self.rotation().as_ref().coords;
                [c.w, c.x, c.y, c.z]

I think we should document this layout.

---

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

        let z = get_input_axis_simple(&self.forward_input_axis, &input);

        if let Some(dir) = Unit::try_new(Vector3::new(x, y, z), 1.0e-6) {

Why is Unit needed here? Isn't (0, 0, 0) just as valid?

---

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

pub struct Transform {
    /// Translation + rotation value
    pub iso: Isometry3<f32>,

Hmm, I'm a bit unhappy with exposing it to the user this way.

---

amethyst_core/src/transform/components/transform.rs, line 21 at r7 (raw file):

    /// Checks whether each `f32` of the `GlobalTransform` is finite (not NaN or inf).
    pub fn is_finite(&self) -> bool {
        !self.0.as_slice().iter().any(|f| !f32::is_finite(*f))

should use .all

---

book/src/math.md, line 10 at r7 (raw file):

we'll redirect you to the excellent [nalgebra website][na] where you can find the
documentation for both of these projects. If you haven't used nalgebra before,
we highly recommend you start out with [nalgebra-glm][nag] as it's somewhat easier

Can you provide us with the motivation to expose both? Is it substiantially easier to justify two APIs?

[torkleyy]

Sorry, need to use Reviewable for such big PRs.

Massive work @magnonellie! One can see you refactored this very carefully.

The only thing I've noticed so far are unresolved questions regarding nalgebra-glm:

• Why are we using it?
• Where should it be used?
• What should the examples do?

Reviewed 12 of 50 files at r1, 3 of 11 files at r2, 12 of 38 files at r5, 21 of 31 files at r7.
Reviewable status: 48 of 75 files reviewed, 5 unresolved discussions (waiting on @magnonellie and @azriel91)

---

amethyst_animation/src/transform.rs, line 51 at r7 (raw file):

            Rotation => SamplerPrimitive::Vec4({
                let c = self.rotation().as_ref().coords;
                [c.w, c.x, c.y, c.z]

I think we should document this layout.

---

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

        let z = get_input_axis_simple(&self.forward_input_axis, &input);

        if let Some(dir) = Unit::try_new(Vector3::new(x, y, z), 1.0e-6) {

Why is Unit needed here? Isn't (0, 0, 0) just as valid?

---

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

pub struct Transform {
    /// Translation + rotation value
    pub iso: Isometry3<f32>,

Hmm, I'm a bit unhappy with exposing it to the user this way.

---

amethyst_core/src/transform/components/transform.rs, line 21 at r7 (raw file):

    /// Checks whether each `f32` of the `GlobalTransform` is finite (not NaN or inf).
    pub fn is_finite(&self) -> bool {
        !self.0.as_slice().iter().any(|f| !f32::is_finite(*f))

should use .all

---

book/src/math.md, line 10 at r7 (raw file):

we'll redirect you to the excellent [nalgebra website][na] where you can find the
documentation for both of these projects. If you haven't used nalgebra before,
we highly recommend you start out with [nalgebra-glm][nag] as it's somewhat easier

Can you provide us with the motivation to expose both? Is it substiantially easier to justify two APIs?

[dotellie]

@jojolepro I'm fairly certain all of those issues are present in master too. Could you double check that perhaps?

[dotellie]

Reviewable status: 48 of 75 files reviewed, 5 unresolved discussions (waiting on @torkleyy and @azriel91)

---

amethyst_animation/src/transform.rs, line 51 at r7 (raw file):

Previously, torkleyy (Thomas Schaller) wrote…

I think we should document this layout.

Document in what way do you mean exactly? The problem here is that nalgebra stores the values as [x, y, z, w] while most other APIs (including the new functions in nalgebra) expect [w, x, y, z].

---

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

Previously, torkleyy (Thomas Schaller) wrote…

Why is Unit needed here? Isn't (0, 0, 0) just as valid?

I think that (0, 0, 0) isn't normalized actually as the length isn't 1. Unit is needed because move_along_local expects a Unit<Vector3>. We should probably ping @sebcrozet here though as they were the one who made this change. 😅

---

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

Previously, torkleyy (Thomas Schaller) wrote…

Hmm, I'm a bit unhappy with exposing it to the user this way.

I can only agree. I would have personally liked to have it be abstracted away behind functions but I was unsure how that would go down with the rest of the community. What would you have liked to see here instead?

---

amethyst_core/src/transform/components/transform.rs, line 21 at r7 (raw file):

Previously, torkleyy (Thomas Schaller) wrote…

should use .all

I just added a unit test which contains the following and passes with the current code:

let mut transform = GlobalTransform::default();
assert!(transform.is_finite());

transform.0.fill_row(2, std::f32::NAN);
assert!(!transform.is_finite());

---

book/src/math.md, line 10 at r7 (raw file):

Previously, torkleyy (Thomas Schaller) wrote…

Can you provide us with the motivation to expose both? Is it substiantially easier to justify two APIs?

I can't really provide a reason to be honest. I myself prefer to just use the nalgebra API but I can imagine that many users will think that the rustdoc problems are too severe and would be very happy to use nalgebra glm as an alternative. If you take a look through the nalgebra-glm docs and compare them to the nalgebra docs, you can see that everything is laid out in a much more understandable fashion with links to other functions you may want to use etc.

[dotellie]

Reviewable status: 48 of 75 files reviewed, 5 unresolved discussions (waiting on @torkleyy and @azriel91)

---

amethyst_animation/src/transform.rs, line 51 at r7 (raw file):

Previously, torkleyy (Thomas Schaller) wrote…

I think we should document this layout.

Document in what way do you mean exactly? The problem here is that nalgebra stores the values as [x, y, z, w] while most other APIs (including the new functions in nalgebra) expect [w, x, y, z].

---

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

Previously, torkleyy (Thomas Schaller) wrote…

Why is Unit needed here? Isn't (0, 0, 0) just as valid?

I think that (0, 0, 0) isn't normalized actually as the length isn't 1. Unit is needed because move_along_local expects a Unit<Vector3>. We should probably ping @sebcrozet here though as they were the one who made this change. 😅

---

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

Previously, torkleyy (Thomas Schaller) wrote…

Hmm, I'm a bit unhappy with exposing it to the user this way.

I can only agree. I would have personally liked to have it be abstracted away behind functions but I was unsure how that would go down with the rest of the community. What would you have liked to see here instead?

---

amethyst_core/src/transform/components/transform.rs, line 21 at r7 (raw file):

Previously, torkleyy (Thomas Schaller) wrote…

should use .all

I just added a unit test which contains the following and passes with the current code:

let mut transform = GlobalTransform::default();
assert!(transform.is_finite());

transform.0.fill_row(2, std::f32::NAN);
assert!(!transform.is_finite());

---

book/src/math.md, line 10 at r7 (raw file):

Previously, torkleyy (Thomas Schaller) wrote…

Can you provide us with the motivation to expose both? Is it substiantially easier to justify two APIs?

I can't really provide a reason to be honest. I myself prefer to just use the nalgebra API but I can imagine that many users will think that the rustdoc problems are too severe and would be very happy to use nalgebra glm as an alternative. If you take a look through the nalgebra-glm docs and compare them to the nalgebra docs, you can see that everything is laid out in a much more understandable fashion with links to other functions you may want to use etc.

[dotellie]

Reviewable status: 77 of 79 files reviewed, 16 unresolved discussions (waiting on @azriel91, @torkleyy, and @magnonellie)

---

amethyst_core/Cargo.toml, line 19 at r9 (raw file):

Previously, azriel91 (Azriel Hoh) wrote…

This doesn't look like it's used anymore

Removed.

---

amethyst_core/src/orientation.rs, line 25 at r9 (raw file):

Previously, azriel91 (Azriel Hoh) wrote…

I lack understanding here, haven't seen so much math in a while 😅. In transform.rs you mentioned:

nalgebra expects (w, x, y, z) upon creation but stores the values as (x, y, z, w)

So I assume that would be for a Vec4. A Matrix3 has 3x3, and I'm having trouble matching column numbers with 0~2.
Might be something to do with these euler angles and pitch/yaw/roll

教えて下さい!

I don't quite understand your question, but I've decided to remove this file completely as it doesn't serve any purpose currently and I'm unsure if I got the math right 😛

---

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

Previously, torkleyy (Thomas Schaller) wrote…

👍 Please remove the no_run in that case.

Done.

---

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

Previously, torkleyy (Thomas Schaller) wrote…

Are you planning on re-adding this test?

Done.

---

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

Previously, azriel91 (Azriel Hoh) wrote…

pedantic: vectors -> vector's (same for all the other adds/sets)

Done.

---

amethyst_renderer/src/cam.rs, line 261 at r9 (raw file):

Previously, azriel91 (Azriel Hoh) wrote…

I can't get this code to work!
It says I'm missing left"
but it's there!! See!?! SEE!!?!?!!

*hours later*

the error lies!

Copy paste strikes again 😝

---

amethyst_renderer/src/cam.rs, line 395 at r9 (raw file):

Previously, azriel91 (Azriel Hoh) wrote…

ditto 😛. how did you manage to change aspect and fovy without realizing the others are broken 🤔

almost makes me wanna write a macro

I honestly have no idea. This was just so tedious to write that I think I spaced out while doing it 😅

[dotellie]

Reviewable status: 77 of 79 files reviewed, 16 unresolved discussions (waiting on @azriel91, @torkleyy, and @magnonellie)

---

amethyst_core/Cargo.toml, line 19 at r9 (raw file):

Previously, azriel91 (Azriel Hoh) wrote…

This doesn't look like it's used anymore

Removed.

---

amethyst_core/src/orientation.rs, line 25 at r9 (raw file):

Previously, azriel91 (Azriel Hoh) wrote…

I lack understanding here, haven't seen so much math in a while 😅. In transform.rs you mentioned:

nalgebra expects (w, x, y, z) upon creation but stores the values as (x, y, z, w)

So I assume that would be for a Vec4. A Matrix3 has 3x3, and I'm having trouble matching column numbers with 0~2.
Might be something to do with these euler angles and pitch/yaw/roll

教えて下さい!

I don't quite understand your question, but I've decided to remove this file completely as it doesn't serve any purpose currently and I'm unsure if I got the math right 😛

---

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

Previously, torkleyy (Thomas Schaller) wrote…

👍 Please remove the no_run in that case.

Done.

---

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

Previously, torkleyy (Thomas Schaller) wrote…

Are you planning on re-adding this test?

Done.

---

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

Previously, azriel91 (Azriel Hoh) wrote…

pedantic: vectors -> vector's (same for all the other adds/sets)

Done.

---

amethyst_renderer/src/cam.rs, line 261 at r9 (raw file):

Previously, azriel91 (Azriel Hoh) wrote…

I can't get this code to work!
It says I'm missing left"
but it's there!! See!?! SEE!!?!?!!

*hours later*

the error lies!

Copy paste strikes again 😝

---

amethyst_renderer/src/cam.rs, line 395 at r9 (raw file):

Previously, azriel91 (Azriel Hoh) wrote…

ditto 😛. how did you manage to change aspect and fovy without realizing the others are broken 🤔

almost makes me wanna write a macro

I honestly have no idea. This was just so tedious to write that I think I spaced out while doing it 😅

[dotellie]

Reviewable status: 14 of 79 files reviewed, 16 unresolved discussions (waiting on @azriel91 and @torkleyy)

---

amethyst_audio/src/systems/audio.rs, line 66 at r9 (raw file):

Previously, azriel91 (Azriel Hoh) wrote…

I think this should now be:

let left_ear_position = listener_transform.transform_point(&listener.left_ear);
let right_ear_position = listener_transform.transform_point(&listener.right_ear);

Done.

[dotellie]

Reviewable status: 14 of 79 files reviewed, 16 unresolved discussions (waiting on @azriel91 and @torkleyy)

---

amethyst_audio/src/systems/audio.rs, line 66 at r9 (raw file):

Previously, azriel91 (Azriel Hoh) wrote…

I think this should now be:

let left_ear_position = listener_transform.transform_point(&listener.left_ear);
let right_ear_position = listener_transform.transform_point(&listener.right_ear);

Done.

[azriel91]

Reviewed 3 of 11 files at r11, 1 of 1 files at r12, 22 of 38 files at r14, 1 of 3 files at r15, 37 of 39 files at r16, 4 of 4 files at r17.
Reviewable status: all files reviewed, 9 unresolved discussions (waiting on @torkleyy)

---

amethyst_core/src/transform/components/transform.rs, line 21 at r7 (raw file):

self.0.as_slice().iter().all(|f| f32::is_finite(*f))

reminder 🎗

[azriel91]

Reviewed 3 of 11 files at r11, 1 of 1 files at r12, 22 of 38 files at r14, 1 of 3 files at r15, 37 of 39 files at r16, 4 of 4 files at r17.
Reviewable status: all files reviewed, 9 unresolved discussions (waiting on @torkleyy)

---

amethyst_core/src/transform/components/transform.rs, line 21 at r7 (raw file):

self.0.as_slice().iter().all(|f| f32::is_finite(*f))

reminder 🎗

[dotellie]

Reviewable status: all files reviewed, 9 unresolved discussions (waiting on @torkleyy)

---

amethyst_core/src/transform/components/transform.rs, line 21 at r7 (raw file):

Previously, azriel91 (Azriel Hoh) wrote…

self.0.as_slice().iter().all(|f| f32::is_finite(*f))

reminder 🎗

Done.

[dotellie]

Reviewable status: all files reviewed, 9 unresolved discussions (waiting on @torkleyy)

---

amethyst_core/src/transform/components/transform.rs, line 21 at r7 (raw file):

Previously, azriel91 (Azriel Hoh) wrote…

self.0.as_slice().iter().all(|f| f32::is_finite(*f))

reminder 🎗

Done.

[dotellie]

@sebcrozet @torkleyy Re-review perhaps? 😅

[jojolepro]

Very nice PR! I got a few nitpicks ^^

I'll be here pretty much all day tomorow, so I can approve quickly once its fixed. When everyone that is blocking it has approved we can merge :)

(Then I got 3k lines of code to fix, then I need to migrate everything to nphysics because amethyst_rhusics will be 100% broken) lol

Good news is that I can start making things that depend on physics that I can merge in amethyst later ^^

[jojolepro]

Very nice PR! I got a few nitpicks ^^

I'll be here pretty much all day tomorow, so I can approve quickly once its fixed. When everyone that is blocking it has approved we can merge :)

(Then I got 3k lines of code to fix, then I need to migrate everything to nphysics because amethyst_rhusics will be 100% broken) lol

Good news is that I can start making things that depend on physics that I can merge in amethyst later ^^

[jojolepro]

So if this stays like that, I'll need to make a PR for the global transform decomposition almost as soon as this is merged.

[dotellie]

I'm not sure what you're referring to.

[sebcrozet]

Great work @magnonellie! It looks good to me now.

[sebcrozet]

Great work @magnonellie! It looks good to me now.

[torkleyy]

Nearly there 👍

Reviewed 1 of 13 files at r9, 8 of 38 files at r14, 8 of 39 files at r16.
Reviewable status: 62 of 80 files reviewed, 18 unresolved discussions (waiting on @azriel91, @torkleyy, @jojolepro, and @magnonellie)

---

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

Previously, magnonellie (Ellie) wrote…

See my comment above for why isometry is needed for performance. I don't think the user should need to know much about isometries however which is why I decided to keep the old structure for prefabs.

I find that a bit strange given that iso is a public field. That means the user will see iso on the field but translation in the RON file, which is more confusing than any consistent method.

[torkleyy]

Nearly there 👍

Reviewed 1 of 13 files at r9, 8 of 38 files at r14, 8 of 39 files at r16.
Reviewable status: 62 of 80 files reviewed, 18 unresolved discussions (waiting on @azriel91, @torkleyy, @jojolepro, and @magnonellie)

---

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

Previously, magnonellie (Ellie) wrote…

See my comment above for why isometry is needed for performance. I don't think the user should need to know much about isometries however which is why I decided to keep the old structure for prefabs.

I find that a bit strange given that iso is a public field. That means the user will see iso on the field but translation in the RON file, which is more confusing than any consistent method.

[dotellie]

Reviewable status: 62 of 80 files reviewed, 18 unresolved discussions (waiting on @azriel91, @torkleyy, and @jojolepro)

---

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

Previously, torkleyy (Thomas Schaller) wrote…

I find that a bit strange given that iso is a public field. That means the user will see iso on the field but translation in the RON file, which is more confusing than any consistent method.

Ah I see. I did actually want to make all fields private but I wasn't sure if everyone would be on board with that. I think I'll go for it though since now is probably a good time to do it. 😅

[dotellie]

Reviewable status: 62 of 80 files reviewed, 18 unresolved discussions (waiting on @azriel91, @torkleyy, and @jojolepro)

---

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

Previously, torkleyy (Thomas Schaller) wrote…

I find that a bit strange given that iso is a public field. That means the user will see iso on the field but translation in the RON file, which is more confusing than any consistent method.

Ah I see. I did actually want to make all fields private but I wasn't sure if everyone would be on board with that. I think I'll go for it though since now is probably a good time to do it. 😅

[torkleyy]

Reviewed 1 of 1 files at r18, 13 of 15 files at r19, 1 of 3 files at r20, 6 of 6 files at r21, 1 of 1 files at r22.
Reviewable status: all files reviewed, 17 unresolved discussions (waiting on @jojolepro)

[torkleyy]

Reviewed 1 of 1 files at r18, 13 of 15 files at r19, 1 of 3 files at r20, 6 of 6 files at r21, 1 of 1 files at r22.
Reviewable status: all files reviewed, 17 unresolved discussions (waiting on @jojolepro)

[torkleyy]

Let's merge this!

bors r+

[bors]

Build succeeded

• continuous-integration/travis-ci/push

[brendanzab]

Woo, congratulations folks! This makes me so happy and excited! 👏 🎉 😍