Proper matrix transformations for sprites #19

skyne98 · 2020-03-23T22:37:33Z

Solves #16.
This is the first introduction of proper matrix transformations for the sprite rendering. It includes scale, translation and rotation with origin. Also a new, simple example is included.

Please note, this pull request has a lot of cleaning to do, so I am waiting for your first review!

cloudhead · 2020-03-29T15:15:24Z

Thanks! I will take a look soon.

cloudhead · 2020-03-29T15:16:32Z

To merge this, the first thing we would have to do is take out the ultraviolet dependency -- if there's anything missing from the built-in math library, I'm happy to add it in there.

skyne98 · 2020-04-23T23:02:10Z

Removed the ultraviolet dependency, had to add a matrix rotation function to the default implementation as well (copied it from cgmath in a simplified manner). Everything seems to work, however, when you enable the cgmath feature flag, then things are not compiling anymore, even inside the core libraries this pull request doesn't touch. Any ideas on that matter? Is it ok on this stage?

Also, I think it might be a good option to incorporate ultraviolet itself as a math library as it seems to be the fastest one at the moment, according to my benchmarks.

zicklag · 2020-04-24T00:18:03Z

You might want to keep an eye out for nalgebra now, too, because with nalgebra 0.21 and the new SIMD optimizations it is competitive with ultraviolet according to the posted benchmarks:

https://rustsim.org/blog/2020/03/23/simd-aosoa-in-nalgebra/

nalgebra's biggest disadvantage to me seems to be it's complication in the documentation and such, but nalgebra-glm seems like it helps with that a lot. The post above has a good comparison between nalgebra and ultraviolet.

Personally I could go with either and I don't have a particular suggestion. They both are interesting for their own reasons. :)

skyne98 · 2020-04-24T07:37:27Z

@zicklag, hmm, interesting, in some benchmarks it is more than 2 or 3 times faster than the ultraviolet counterpart. Also, giving some options to decide between more precise 64-bit floats and faster 16-bit floats might be a good idea, however, I might imagine there are more low-hanging optimisations rgx can achieve before we will need 16-bit floats.

cloudhead · 2020-04-26T12:11:56Z

Thanks! I've pushed a fix to master regarding the cgmath compilation, maybe this will solve the problem.

With regards to third party algebra libraries, rgx used to depend on cgmath, but when it went unmaintained, I decided to copy the parts I needed into rgx, make certain improvements, and make the dependency optional. nalgebra is too big of a dependency to pull in for simple 2D math. As for all the other libraries, they are good, but they come and go, so I'd rather wait and see which ones are still around in a year.

cloudhead · 2020-04-26T12:16:36Z

src/kit/sprite2d/mod.rs

        h: u32,
        src: Rect<f32>,
-        dst: Rect<f32>,
+        dst: Vector2<f32>,


I wonder why you've changed the API here from a rect to a position + scale? This seems like an unnecessary change and will break a lot of things.

In theory, Rect should be sufficient in this situation as it is basically a combination of two 2d vectors itself. However, I think it's a little clearer for an average user to have them separate as it explicitly tells you the roles of both vectors. Also, this way of handling position + scale seems to be more common among the engines used by lots of game developers (especially 2d-oriented): Love2d, XNA, Game Maker.

In the end, I think it makes sense to have both APIs available, as some users coming from Unity or even XNA as well might find the Rect API more intuitive. This approach will also allow things to not break, as you have mentioned above.

Yeah, I understand that it's a more common API, but this change is unrelated to adding rotation to sprites, and is a breaking change.

The advantage of the rect is when a specific area needs to be filled with a sprite (eg. the screen), it's easier than having to calculate the required scale.

There's a lot of code out there that's going to have to be changed if the API is changed like this. If we only add angle, it's an easy fix, but changing from a Rect to a Vector pair is going to be pretty annoying.

Sure, I will add back the rect API, but I think we can keep both, in the end. What do you think about this?

Hmm, that could be confusing. I would rather have a builder-like API as we do for shape2d in the long term. Something like: batch.add(Sprite::new(...).scale(4.0).position([4,4]).repeat(...)..), otherwise it's a lot of parameters to remember positionally, no?

cloudhead · 2020-04-26T12:18:47Z

src/kit/sprite2d/mod.rs

+                -src.height() * origin.y * scale.y,
+                0.0,
+            ));
+            let rotation = Matrix4::from_angle_z(*angle * 3.14 / 180.0);


There is a PI constant here: https://doc.rust-lang.org/std/f32/consts/constant.PI.html

My bad, didn't know that, will replace it in the next commit.

src/math/algebra.rs

skyne98 · 2020-05-01T21:31:21Z

@cloudhead, did the changes you mentioned, can you have a look?

cloudhead · 2020-05-05T20:18:28Z

src/kit/sprite2d/mod.rs

+            let vec5 = Vector3::new(0.0, 1.0, 1.0);
+            let vec5 = transformation * vec5;
+            let vec6 = Vector3::new(1.0, 1.0, 1.0);
+            let vec6 = transformation * vec6;


You can iterate over a Vec<Vertex> and apply the transformation on each Vector2, it would be a lot cleaner :)

This could still use some work.

cloudhead · 2020-05-05T20:20:20Z

@cloudhead, did the changes you mentioned, can you have a look?

Yup, we're getting there! Don't forget to fix the examples when you've finalized the library code.

skyne98 · 2020-07-16T23:15:15Z

@cloudhead, finally got some time away from work and studying - so here are a couple of commits that should solve all the discussed issues.

cloudhead

Nice progress, there are still some issues though 🧐

src/kit/sprite2d/mod.rs

cloudhead · 2020-07-21T06:27:35Z

src/kit/sprite2d/mod.rs

+    origin: Vector2<f32>
+) -> Sprite {
+    Sprite::new(src).position(pos).scale(scale).origin(origin)
 }


These two methods (sprite_pos and sprite_origin) are not needed.

You mean the function calls, not the functions themselves?

No I mean the functions themselves, since we have the builder.

cloudhead · 2020-07-21T06:32:42Z

src/kit/sprite2d/mod.rs

+    pub fn angle(mut self, angle: f32) -> Self {
+        self.angle = angle;
+        self
+    }


This API should look the same as for shape2d. See here: https://github.com/cloudhead/rgx/blob/master/src/kit/shape2d/mod.rs#L173

Yeah, my choice of the name is not great, I agree, will change it. However, do you think passing the rotation origin each time you want to rotate the sprite is a good idea? In my mind, people will expect it to work as it does in any other modern game engine or library - set origin once and then expect all the rotations to happen around that origin point.
Also, I guess, it is more performant to avoid copying 60% more information each time you want to change the angle.

This isn't a game engine though, it's a graphics library. The game entity can store its origin and then pass it in when it needs to be rendered. The extra copy of the origin will not impact performance.

cloudhead · 2020-07-21T06:33:21Z

src/lib.rs

@@ -1,4 +1,5 @@
 #![deny(clippy::all)]
+#![feature(stmt_expr_attributes)]


What's this?

I believe at some point I used attributes for expressions somewhere, I guess it should not be needed anymore, I will investigate 😄

cloudhead · 2020-07-21T06:34:22Z

src/math/algebra.rs

 //! Most of the code in this module was borrowed from the `cgmath` package.

 pub use num_traits::{cast, Float, One, Zero};
+use num_traits::real::Real;


Is this used?

I will have a look, thanks for pointing that out!

cloudhead · 2020-07-21T06:38:46Z

Additionally, https://github.com/cloudhead/rgx/pull/19#discussion_r420379790 was not addressed properly.

cloudhead · 2020-08-06T11:34:45Z

src/kit/sprite2d/mod.rs

+    pub pos: Vector2<f32>,
+    pub angle: f32,
+    pub scale: Vector2<f32>,
+    pub origin: Vector2<f32>,


Origin should be Point2

So should pos actually.

Rename the functions, formatting

skyne98 added 2 commits February 12, 2020 16:16

Added crude rotation and scaling

0e7ac1c

Implement customizable origin, fix examples, add origin example

650cf1f

skyne98 marked this pull request as ready for review April 23, 2020 22:59

Remove the ultraviolet dependency, add rotation to default math

e076404

cloudhead reviewed Apr 26, 2020

View reviewed changes

src/math/algebra.rs Show resolved Hide resolved

cloudhead reviewed Apr 29, 2020

View reviewed changes

src/math/algebra.rs Outdated Show resolved Hide resolved

cloudhead reviewed Apr 29, 2020

View reviewed changes

src/math/algebra.rs Outdated Show resolved Hide resolved

cloudhead reviewed Apr 29, 2020

View reviewed changes

src/math/algebra.rs Outdated Show resolved Hide resolved

Fix constant, remove unused function

25b1f22

cloudhead reviewed May 5, 2020

View reviewed changes

skyne98 added 2 commits July 17, 2020 00:59

Fix the sprite builder, add rectangle option, fix examples

eae0b31

Fix the examples for sprites

6242c2c

Merge remote-tracking branch 'upstream/master'

2d910d3

cloudhead requested changes Jul 21, 2020

View reviewed changes

cloudhead reviewed Aug 6, 2020

View reviewed changes

skyne98 added 2 commits January 26, 2021 21:37

Rename the functions, formatting

2adcf6e

Merge pull request #1 from skyne98/profiler

d9b57e3

Rename the functions, formatting

		@@ -1,4 +1,5 @@
		#![deny(clippy::all)]
		#![feature(stmt_expr_attributes)]

Proper matrix transformations for sprites #19

Are you sure you want to change the base?

Proper matrix transformations for sprites #19

Uh oh!

Conversation

skyne98 commented Mar 23, 2020

Uh oh!

cloudhead commented Mar 29, 2020

Uh oh!

cloudhead commented Mar 29, 2020

Uh oh!

skyne98 commented Apr 23, 2020

Uh oh!

zicklag commented Apr 24, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

skyne98 commented Apr 24, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

cloudhead commented Apr 26, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

skyne98 commented May 1, 2020

Uh oh!

cloudhead May 5, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cloudhead commented May 5, 2020

Uh oh!

skyne98 commented Jul 16, 2020

Uh oh!

cloudhead left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cloudhead commented Jul 21, 2020

zicklag commented Apr 24, 2020 •

edited

Loading

skyne98 commented Apr 24, 2020 •

edited

Loading

cloudhead May 5, 2020 •

edited

Loading