Upgrade and use cgmath #462

Xaeroxe · Oct 26, 2017

This upgrades us to cgmath 0.15, uses it where appropriate, and enables the mint feature for it.

You may have noticed that the renderer actually lost cgmath types in this PR, this was a necessary sacrifice to upgrade to 0.15. 0.15 is the only version released with mint support. Our current gfx version uses cgmath 0.14 so the gfx trait implementations were not compatible with cgmath 0.15.

Closes #452
Closes #200
Closes #236

Rhuagh · Oct 26, 2017

Rhuagh reviewed Oct 26, 2017

View changes

Rhuagh · Oct 26, 2017

amethyst_core/src/transform/components/local_transform.rs

@@ -1,28 +1,38 @@
 //! Local transform component.
 
-use cgmath::{Deg, InnerSpace, Matrix3, Matrix4, Point3, Quaternion, Rotation, Rotation3, Vector3};
+use cgmath::{


Weird formatting

Rhuagh · Oct 26, 2017

amethyst_core/src/transform/components/local_transform.rs

     /// Translation/position vector [x, y, z]
-    pub translation: [f32; 3],
+    pub translation: Point3<f32>,


This should be Vector, translation is a movement quantity, but a positional quantity

And it's a relative translation from the parent and not an absolute position

I can make good arguments in my head for both, so I'll use Vector as you've suggested.

I think the most compelling part is that it's a relative quantity, not absolute, in LocalTransform

torkleyy · Oct 26, 2017

torkleyy reviewed Oct 26, 2017

View changes

torkleyy · Oct 26, 2017

amethyst_renderer/src/light.rs

@@ -31,14 +30,14 @@ pub struct DirectionalLight {
     /// Color of the light in RGBA8 format.
     pub color: Rgba,
     /// Direction that the light is pointing.
-    pub direction: Vector3<f32>,
+    pub direction: [f32; 3],


Wasn't the goal consistency? So could we maybe just upgrade to gfx 0.17?

0.17 isn't on crates :(

Although I'll add a Todo for when we do upgrade our gfx

Apparently the most recent version of rustfmt-nightly says we were wrong on all of our formatting.

Xaeroxe · Oct 26, 2017

Apparently the most recent version of rustfmt-nightly says we were wrong on all of our formatting.

Hope ya'll are ready for some rebases :/

Xaeroxe · Oct 26, 2017

Hope ya'll are ready for some rebases :/

Using our cgmath now requires a couple features to be enabled by the person depending on cgmath, should we pub extern cgmath somewhere so they can easily get the right version and features? (I think yes so I've already added it to the PR but I want to hear what others think)

Xaeroxe · Oct 26, 2017

Using our cgmath now requires a couple features to be enabled by the person depending on cgmath, should we pub extern cgmath somewhere so they can easily get the right version and features? (I think yes so I've already added it to the PR but I want to hear what others think)

r? @Rhuagh @torkleyy

Xaeroxe · Oct 26, 2017

r? @Rhuagh @torkleyy

Rhuagh · Oct 26, 2017

Rhuagh reviewed Oct 26, 2017

View changes

Rhuagh · Oct 26, 2017

amethyst_core/src/transform/components/transform.rs

-            [0.0, 0.0, 1.0, 0.0],
-            [0.0, 0.0, 0.0, 1.0],
-        ])
+        Transform(


Matrix4::one() ?

Rhuagh · Oct 26, 2017

Rhuagh approved these changes Oct 26, 2017

View changes

Rhuagh · Oct 26, 2017

Rhuagh reviewed Oct 26, 2017

View changes

Rhuagh · Oct 26, 2017

amethyst_core/src/orientation.rs

-            forward: [1.0, 0.0, 0.0],
-            right: [0.0, -1.0, 0.0],
-            up: [0.0, 0.0, 1.0],
+            forward: [1.0, 0.0, 0.0].into(),


In theory, you could use Vector3::unit_x() -Vector3::unit_y() etc, but might not be a point tbh

Rhuagh · Oct 26, 2017

amethyst_core/src/transform/components/local_transform.rs

@@ -31,16 +33,16 @@ impl LocalTransform {
     /// Combined with the parent's global `Transform` component it gives
     /// the global (or world) matrix for the current entity.
     #[inline]
-    pub fn matrix(&self) -> [[f32; 4]; 4] {
+    pub fn matrix(&self) -> Matrix4<f32> {
         let quat: Matrix3<f32> = Quaternion::from(self.rotation).into();
         let scale: Matrix3<f32> = Matrix3::<f32> {


Matrix3::from_diagonal(self.scale)

Rhuagh · Oct 26, 2017

amethyst_core/src/transform/components/local_transform.rs

-            translation: [0.0, 0.0, 0.0],
-            rotation: [1.0, 0.0, 0.0, 0.0],
-            scale: [1.0, 1.0, 1.0],
+            translation: [0.0, 0.0, 0.0].into(),


Vector3::zero(), Quaternion::one(), Vector3::from_value(1.)

Rhuagh · Oct 26, 2017

amethyst_core/src/transform/systems.rs

-        transform.translation = [0.0, 0.0, 0.0];
-        transform.rotation = [1.0, 0.0, 0.0, 0.0];
+        transform.translation = Vector3::zero();
+        transform.rotation = Quaternion::new(1.0, 0.0, 0.0, 0.0);


Quaternion::one()

Rhuagh · Oct 26, 2017

examples/03_renderable/main.rs

         for (_, transform) in (&camera, &mut transforms).join() {
             // rotate the camera, using the origin as a pivot point
             transform.translation = delta_rot
-                .rotate_point(Point3::from(transform.translation))
-                .into();
+                .rotate_point(Point3::from_vec(transform.translation)).to_vec();


.rotate_vector(transform.translation)

Rhuagh · Oct 26, 2017

examples/05_assets/main.rs

-        trans.translation = [-5.0, 0.0, 0.0];
-        trans.scale = [2.0, 2.0, 2.0];
+        trans.translation = Vector3::new(-5.0, 0.0, 0.0);
+        trans.scale = [2.0; 3].into();


in theory, Vector3::from_value(2.), but not necessary.

Rebased and addressed @Rhuagh

Xaeroxe · Oct 26, 2017

Rebased and addressed @Rhuagh

Rhuagh · Oct 26, 2017

Rhuagh approved these changes Oct 26, 2017

View changes

@torkleyy Since my current design sort of goes against the grain on our current crate structure allow me to explain myself:

cgmath is compiled with a particular version and set of features, it's of the utmost importance that that version and set of features are used consistently throughout not only our user's games but also our internal ecosystem. Otherwise, types that seemingly should be compatible, suddenly aren't. So, in order to guarantee synchronization and reduce the amount of times we have to specify these versions and features all amethyst crates and downstream pull in a cgmath that's re-exported from amethyst_core. This should allow us to drastically reduce the amount of turmoil caused by future upgrades and feature changes.

Xaeroxe · Oct 26, 2017

@torkleyy Since my current design sort of goes against the grain on our current crate structure allow me to explain myself:

cgmath is compiled with a particular version and set of features, it's of the utmost importance that that version and set of features are used consistently throughout not only our user's games but also our internal ecosystem. Otherwise, types that seemingly should be compatible, suddenly aren't. So, in order to guarantee synchronization and reduce the amount of times we have to specify these versions and features all amethyst crates and downstream pull in a cgmath that's re-exported from amethyst_core. This should allow us to drastically reduce the amount of turmoil caused by future upgrades and feature changes.

I couldn't spot any logical issues, it's just the implied strategy regarding our math API that I'm not so sure about.

torkleyy · Oct 27, 2017

I couldn't spot any logical issues, it's just the implied strategy regarding our math API that I'm not so sure about.

@torkleyy care to elaborate?

Xaeroxe · Oct 27, 2017

@torkleyy care to elaborate?

Well, I think the arguments on all sides are clear, we've gone over them a couple of times. It seems we can't find any full agreement, so..

torkleyy · Oct 27, 2017

Well, I think the arguments on all sides are clear, we've gone over them a couple of times. It seems we can't find any full agreement, so..

torkleyy · Oct 27, 2017

torkleyy approved these changes Oct 27, 2017

View changes

..take this as my approval together with the note that at least two other members should approve this before we proceed.

This is the only way we can make progress.

torkleyy · Oct 27, 2017

This is the only way we can make progress.

r? @Aceeri

Xaeroxe · Oct 27, 2017

r? @Aceeri

omni-viral · Oct 28, 2017

omni-viral approved these changes Oct 28, 2017

View changes

bors r+

Xaeroxe · Oct 28, 2017

bors r+

So why don't we let @Aceeri approve this?

torkleyy · Oct 28, 2017

So why don't we let @Aceeri approve this?

bors r-

torkleyy · Oct 28, 2017

bors r-

Canceled

bors · Oct 28, 2017

Canceled

Doesn't seem like @Aceeri is going to review this in the near future, so I have to merge this now.

bors r+

torkleyy · Oct 28, 2017

Doesn't seem like @Aceeri is going to review this in the near future, so I have to merge this now.

bors r+

Build succeeded

bors · Oct 28, 2017

Build succeeded

Xaeroxe added project: audio project: core project: gltf project: main crate project: rendering type: improvement labels Oct 26, 2017

Xaeroxe requested review from Rhuagh, Aceeri and torkleyy Oct 26, 2017

Xaeroxe referenced this pull request Oct 26, 2017
Closed
[Discussion] Use cgmath as our official math library? #452

torkleyy added status: ready type: RFC labels Oct 27, 2017

torkleyy referenced this pull request Oct 28, 2017
Closed
Plan for 0.6 release #249

bors bot merged commit 2602e43 into amethyst:develop Oct 28, 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 deleted the Xaeroxe:cgmath branch Oct 28, 2017

Aceeri added this to the 0.6.0 milestone Nov 28, 2017

amethyst/amethyst

Join GitHub today

Upgrade and use cgmath #462

Conversation

Xaeroxe commented Oct 26, 2017 • edited Edited 2 times Xaeroxe edited Oct 26, 2017 (most recent) Xaeroxe edited Oct 26, 2017

Xaeroxe added project: audio project: core project: gltf project: main crate project: rendering type: improvement labels Oct 26, 2017

Xaeroxe requested review from Rhuagh, Aceeri and torkleyy Oct 26, 2017

Xaeroxe referenced this pull request Oct 26, 2017

Rhuagh reviewed Oct 26, 2017 View changes

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

torkleyy reviewed Oct 26, 2017 View changes

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

Xaeroxe commented Oct 26, 2017

This comment has been minimized.

Xaeroxe commented Oct 26, 2017

This comment has been minimized.

Xaeroxe commented Oct 26, 2017 • edited Edited 3 times Xaeroxe edited Oct 26, 2017 (most recent) Xaeroxe edited Oct 26, 2017 Xaeroxe edited Oct 26, 2017

This comment has been minimized.

Xaeroxe commented Oct 26, 2017

Rhuagh reviewed Oct 26, 2017 View changes

This comment has been minimized.

Rhuagh approved these changes Oct 26, 2017 View changes

Rhuagh reviewed Oct 26, 2017 View changes

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

Xaeroxe commented Oct 26, 2017

Rhuagh approved these changes Oct 26, 2017 View changes

Xaeroxe added some commits Oct 26, 2017

This comment has been minimized.

Xaeroxe commented Oct 26, 2017

torkleyy added status: ready type: RFC labels Oct 27, 2017

This comment has been minimized.

torkleyy commented Oct 27, 2017

This comment has been minimized.

Xaeroxe commented Oct 27, 2017

This comment has been minimized.

torkleyy commented Oct 27, 2017

torkleyy approved these changes Oct 27, 2017 View changes

This comment has been minimized.

torkleyy commented Oct 27, 2017

This comment has been minimized.

Xaeroxe commented Oct 27, 2017

torkleyy referenced this pull request Oct 28, 2017

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

This comment has been minimized.

Xaeroxe commented Oct 28, 2017

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

This comment has been minimized.

torkleyy commented Oct 28, 2017

This comment has been minimized.

torkleyy commented Oct 28, 2017

This comment has been minimized.

bors bot Oct 28, 2017

Canceled

bors bot commented Oct 28, 2017

Canceled

This comment has been minimized.

torkleyy commented Oct 28, 2017

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

This comment has been minimized.

bors bot Oct 28, 2017

Build succeeded

bors bot commented Oct 28, 2017

Build succeeded

Hide details View details bors bot merged commit 2602e43 into amethyst:develop Oct 28, 2017 3 checks passed

3 checks passed

Xaeroxe deleted the Xaeroxe:cgmath branch Oct 28, 2017

Aceeri added this to the 0.6.0 milestone Nov 28, 2017

Xaeroxe commented Oct 26, 2017 •

edited

Edited 2 times

Xaeroxe edited Oct 26, 2017 (most recent)

Xaeroxe edited Oct 26, 2017

Rhuagh reviewed Oct 26, 2017

View changes

torkleyy reviewed Oct 26, 2017

View changes

Xaeroxe commented Oct 26, 2017 •

edited

Edited 3 times

Xaeroxe edited Oct 26, 2017 (most recent)

Xaeroxe edited Oct 26, 2017

Xaeroxe edited Oct 26, 2017

Rhuagh reviewed Oct 26, 2017

View changes

Rhuagh approved these changes Oct 26, 2017

View changes

Rhuagh reviewed Oct 26, 2017

View changes

Rhuagh approved these changes Oct 26, 2017

View changes

torkleyy approved these changes Oct 27, 2017

View changes

omni-viral approved these changes Oct 28, 2017

View changes

bors bot merged commit `2602e43` into amethyst:develop Oct 28, 2017
3 checks passed