Reimplements Many `Projection` Methods in Rust #179

Dheatly23 · 2023-03-13T13:53:16Z

Closes #143

Note: create_fit_aabb is not yet exists, should it be a stub function?

lilizoey · 2023-03-13T14:32:40Z

This looks largely good! I'm not too familiar with the actual math here (thus why i didn't do this initially) but overall seems good.

I did notice that some of the code looks very similar to the c++ code, is this just a consequence of there being fairly standard algorithms for this stuff, or did you look at and copy some code? It's not an issue either way, it's just good to know if you did copy and to what degree, for copyright reasons. (we can copy code from godot but doing so to a substantial degree would require us to properly add MIT license attribution).

There are still some functions not fully reimplemented in rust, so this wouldn't fully close #143, but it's a very good start on getting that done.

You are missing tests, could you add some unit-tests for code we can run entirely from rust, as well as integration tests to at least confirm our implementation works approximately the same as godot's? (feel free to add other integration tests that make sense too, but keep in mind that we can't really catch errors from godot at the moment)

Look at basis.rs for an example of some unit tests, and basis_test.rs. In particular you can use basis_equiv as a base for how to write a test that checks that the two implementations work the same. (dont forget to add the license header when you make a new file).

create_fit_aabb doesn't exist yeah, you can add one that just calls into godot for it, but you cant reimplement it in rust yet because we havent reimplemented Aabb.

Dheatly23 · 2023-03-13T14:43:22Z

I mostly use OpenGL's description of perspective and orthogonal matrix. It's pretty standard anyways. Sometimes there is equivalent function in glam that i can reuse.

As for create_*_hmd, well tbh it's pure magic. As far as i know, there isn't obvious reference to what it does. I don't know, maybe one can find the algorithm 🤷‍♂️

I might make unittests tomorrow.

Dheatly23 · 2023-03-18T06:41:43Z

I think it's ready for review for now.

The only missing tests are create_perspective_hmd and create_for_hmd. Biggest problem implementing those is the sheer number of parameters, making it really hard to test. Maybe if we could use library like proptest or fuzzing it would make future tests much easier.

Bromeon

Thanks a lot! 👍
The PR looks mostly good, if you could squash your comments and answer the minor comments, that would be nice!

Regarding fuzzing, I agree it's something we should consider, I brought up the idea of property-based testing in the past. Do you have experience with propetest in particular?

Bromeon · 2023-03-19T15:21:23Z

godot-core/src/builtin/projection.rs

+        self.cols[3].w == 1.0
+
+        // XXX: Test the entire last row?
+        // The argument is that W should not mixed with any other dimensions.
+        // But if the only operation is projection and affine, it suffice
+        // to check if input W is nullified (v33 is zero).
+        // (self.cols[0].w == 0.0) && (self.cols[1].w == 0.0) && (self.cols[2] == 0.0) && (self.cols[3].w == 1.0)


How does Godot handle this?
In case of doubt, I'd probably adopt its behavior, or what do you think? 🤔

It's currently identical to Godot's. I just proposing for maybe a more thorough test (?) But it risk diverging with Godot.

I see. Could you maybe change "XXX" to "TODO" and mention what you just replied (that the current impl matches Godot's)?

Bromeon · 2023-03-19T15:25:47Z

godot-core/src/builtin/projection.rs

+                                let left = (left_i as real) * 0.5 - 5.0;
+                                let right = (right_i as real) * 0.5 - 5.0;
+                                let bottom = (bottom_i as real) * 0.5 - 5.0;
+                                let top = (top_i as real) * 0.5 - 5.0;
+                                let near = (near_i as real) * 0.5 - 5.0;
+                                let far = (far_i as real) * 0.5 - 5.0;


Maybe you could extract the (x as real) * 0.5 - 5.0 into a small closure or function.

Probably, but better solution would be proper fuzzing. I'll refactor it.

Bromeon · 2023-03-19T15:28:10Z

bors try

bors · 2023-03-19T15:44:12Z

try

Build succeeded:

full-ci

Dheatly23 · 2023-03-20T06:07:02Z

Regarding fuzzing, I agree it's something we should consider, I brought up the idea of property
based testing in the past. Do you have experience with propetest in particular?

I haven't tried fuzz testing yet. Looking up potential libraries, it seem proptest is most promising. It looks easy to configure and has shrinking functionality to pinpoint minimal counterexample. Another candidate is quickcheck.

Bromeon

Looks mostly good. Few comments left, and could you squash commits to 1?

Bromeon · 2023-03-20T09:07:40Z

godot-core/src/builtin/projection.rs

+        self.cols[3].w == 1.0
+
+        // XXX: Test the entire last row?
+        // The argument is that W should not mixed with any other dimensions.
+        // But if the only operation is projection and affine, it suffice
+        // to check if input W is nullified (v33 is zero).
+        // (self.cols[0].w == 0.0) && (self.cols[1].w == 0.0) && (self.cols[2] == 0.0) && (self.cols[3].w == 1.0)


I see. Could you maybe change "XXX" to "TODO" and mention what you just replied (that the current impl matches Godot's)?

Bromeon · 2023-03-20T09:10:21Z

godot-core/src/builtin/projection.rs

+                    RMat4::from_cols_array(&[
+                        1.0 / x,
+                        0.0,
+                        0.0,
+                        0.0,
+                        0.0,
+                        1.0 / y,
+                        0.0,
+                        0.0,
+                        0.0,
+                        0.0,
+                        1.0 / z,
+                        0.0,
+                        0.0,
+                        0.0,
+                        0.0,
+                        1.0 / w,
+                    ]),


Maybe that's a case for #[rustfmt::skip] and 4-by-4 matrix notation for readability. I think this has been done elsewhere already.

Maybe use a separate variable:

if det.abs() > 1e-6 { #[rustfmt::skip] let expected = RMat4::from_cols_array(&[ // 4 rows, 4 columns ]); assert_eq_approx!(proj.inverse(), expected);

Bromeon · 2023-03-20T09:11:46Z

godot-core/src/builtin/projection.rs

+    /// Test `create_orthogonal_aspect` method.
+    #[test]
+    fn test_ortho_aspect() {


There's no reason to abbreviate here, it just makes it harder to associate with the method.

Suggested change

/// Test `create_orthogonal_aspect` method.

#[test]

fn test_ortho_aspect() {

/// Test `create_orthogonal_aspect` method.

#[test]

fn test_orthogonal_aspect() {

Bromeon · 2023-03-20T09:14:09Z

godot-core/src/builtin/projection.rs

+
+                                assert!(
+                                    Projection::create_orthogonal(left, right, bottom, top, near, far).is_orthogonal(),
+                                    "Projection should be orthogonal! (left={left} right={right} bottom={bottom} top={top} near={near} far={far})",


Minor nitpick, the screaming ! in the message is not needed 😉

Bromeon

Thanks for the updates, and the great work in this PR!

Using RMat4::from_cols_array_2d for square matrix literals is even better than my #[rustfmt::skip] suggestion! 👍

bors r+

bors · 2023-03-21T20:21:18Z

Build succeeded:

full-ci

Dheatly23 changed the title ~~Reimplements Projection in Rust~~ Reimplements Many Projection Methods in Rust Mar 13, 2023

Bromeon reviewed Mar 19, 2023

View reviewed changes

bors bot added a commit that referenced this pull request Mar 19, 2023

Try #179:

dde53c2

Bromeon added c: core Core components quality-of-life No new functionality, but improves ergonomics/internals labels Mar 19, 2023

Bromeon reviewed Mar 20, 2023

View reviewed changes

Implements Many Projection Methods in Rust

13d4611

Dheatly23 force-pushed the feature/projection-impl branch from a4cbe9c to 13d4611 Compare March 21, 2023 05:58

Bromeon approved these changes Mar 21, 2023

View reviewed changes

bors bot merged commit dbce0d9 into godot-rust:master Mar 21, 2023

lilizoey mentioned this pull request Jun 14, 2023

Tracking issue for builtin feature-completeness #310

Closed

33 tasks

Dheatly23 deleted the feature/projection-impl branch March 12, 2024 07:05

Uh oh!

Reimplements Many Projection Methods in Rust #179

Reimplements Many Projection Methods in Rust #179

Uh oh!

Conversation

Dheatly23 commented Mar 13, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

lilizoey commented Mar 13, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Dheatly23 commented Mar 13, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Dheatly23 commented Mar 18, 2023

Uh oh!

Bromeon left a comment

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!

Bromeon commented Mar 19, 2023

Uh oh!

bors bot commented Mar 19, 2023

try

Uh oh!

Dheatly23 commented Mar 20, 2023

Uh oh!

Bromeon left a comment

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!

Bromeon left a comment

Choose a reason for hiding this comment

Uh oh!

bors bot commented Mar 21, 2023

Uh oh!

Uh oh!

Reimplements Many `Projection` Methods in Rust #179

Reimplements Many `Projection` Methods in Rust #179

Dheatly23 commented Mar 13, 2023 •

edited

Loading

lilizoey commented Mar 13, 2023 •

edited

Loading

Dheatly23 commented Mar 13, 2023 •

edited

Loading