Proposal-1778: Updating Transform.rotated() docs and test cases. #43694

AlexanderPruss · 2020-11-19T15:56:47Z

For proposal 1778: godotengine/godot-proposals#1788

Following the discussion in #43601, it was decided to just adjust the documentation of Transform.rotated() instead of adding a new function.

If this is approved, we can probably close the previous PR and the linked proposal.

Xrayez · 2020-11-19T18:09:15Z

tests/test_math.h

+ Transform rotated_transform = transform.rotated(Vector3(0, 1, 0), Math_PI);
+ REQUIRE(rotated_transform.is_equal_approx(expected));
+
+ // Make sure that the rotated transform isn't sharing references with the original transform.
+ REQUIRE(&rotated_transform.basis != &transform.basis);
+ REQUIRE(&rotated_transform.origin != &transform.origin);


While the first REQUIRE is reasonable, the successive assertions depend on the previous one. Better do this:

Suggested change

Transform rotated_transform = transform.rotated(Vector3(0, 1, 0), Math_PI);

REQUIRE(rotated_transform.is_equal_approx(expected));

// Make sure that the rotated transform isn't sharing references with the original transform.

REQUIRE(&rotated_transform.basis != &transform.basis);

REQUIRE(&rotated_transform.origin != &transform.origin);

Transform rotated_transform = transform.rotated(Vector3(0, 1, 0), Math_PI);

REQUIRE(rotated_transform.is_equal_approx(expected));

// Make sure that the rotated transform isn't sharing references with the original transform.

CHECK(&rotated_transform.basis != &transform.basis);

CHECK(&rotated_transform.origin != &transform.origin);

This way, both "basis" and "origin" will be tested (without immediately exiting the test case if the first one fails).

Same for the test case below.

Also, not sure about referencing with &, is this required?

Seems reasonable, I'll change the latter REQUIREs to CHECKs.

As far as referencing with &, this may just be paranoia due to my lack of C++ experience. But technically the == operator can be overloaded, right?

Looking more closely now, in this case & will take a pointer, so this will always "falsely" succeed because this compares pointers rather than values, because as far as I know rotated_transform is a modified copy of transform, so & is not suitable in this case.

So this should rather be:

Suggested change

Transform rotated_transform = transform.rotated(Vector3(0, 1, 0), Math_PI);

REQUIRE(rotated_transform.is_equal_approx(expected));

// Make sure that the rotated transform isn't sharing references with the original transform.

REQUIRE(&rotated_transform.basis != &transform.basis);

REQUIRE(&rotated_transform.origin != &transform.origin);

Transform rotated_transform = transform.rotated(Vector3(0, 1, 0), Math_PI);

REQUIRE(rotated_transform.is_equal_approx(expected));

// Make sure that the rotated transform isn't sharing references with the original transform.

CHECK(rotated_transform.basis != transform.basis);

CHECK(rotated_transform.origin != transform.origin);

But technically the == operator can be overloaded, right?

Yes, this is done for a lot of existing core types in Godot, so usually you don't have to worry about this and just do equality checks. If those fail due to floating point accumulation, is_equal_approx is also implemented for built-in types.

I think the pointer memory addresses are what I wanted to check; I thought I could make sure that transform.rotated() returns an object that doesn't share important references with the old object. I.e. that the following is the case -

Transform transform = Transform();
Transform rotated = transform.rotated(...);
transform.origin.x += 1;
//rotated's origin should still be on the origin, not on (1,0,0)

But that's maybe not necessary, I think I'll just remove those assertions. Though in general, as a new Godot user, I'm not always clear on whether a function will modify the existing object or return a new object.

aaronfranke

This needs to be rebased because Transform was renamed to Transform3D.

aaronfranke · 2021-08-29T14:35:26Z

doc/classes/Transform.xml

@@ -168,7 +168,8 @@
 <argument index="1" name="phi" type="float">
 </argument>
 <description>
- Rotates the transform around the given axis by the given angle (in radians), using matrix multiplication. The axis must be a normalized vector.
+ Returns a new transform created by rotating around the given axis by the given angle (in radians). The axis must be a normalized vector.
+ The axis of rotation goes through (0, 0, 0) in local space, so the transform's origin will also rotate. If you want to change the transform's orientation while keeping it in place, consider calling [code]rotated[/code] on the transform's basis instead.


While technically this should be implied from the current text, for clarity's sake we should mention "relative to the parent" somewhere here.

aaronfranke · 2021-08-29T14:39:39Z

tests/test_math.h

+ expected.basis.set_axis(2, Vector3(0, 0, -1));
+
+ Transform rotated_transform = transform.rotated(Vector3(0, 1, 0), Math_PI);
+ REQUIRE(rotated_transform.is_equal_approx(expected));


Most of the code in the test cases is very nice. But two things:

These need to go in a test_transform_3d.h file.

Why are you using REQUIRE when the other tests use CHECK_MESSAGE?

I'll move the code over into a new file and make sure it compiles after the renamings, and change REQUIRE to CHECK_MESSAGE. I'll keep the assertion the same.

`#pragma once` was used in a few files, yet we settled on using traditional include guards instead. The PooledList template comment was also moved to allow editors such as Visual Studio Code to display the comment when hovering PooledList. `app.h` was renamed to `app_uwp.h` to be less generic for the include guard.

…odotengine#52915

Replace `#pragma once` by traditional include guards for consistency

…_fix

…p_fix

@AnilBK

New contributors added to AUTHORS: @AnilBK, @Jummit Thanks to all contributors and donors for making Godot possible!

…t_update

Fix incorrect offsets of tooltip content in `CodeEdit`

AlexanderPruss · 2021-10-01T16:00:20Z

After the rebase this PR's commit history has suddenly gotten a bit bigger. Oh dear. I'll have to see what has to happen to fix this, but I won't get to it tonight I'm afraid.

Don't rebase while on sick leave, lesson learned

aaronfranke · 2021-10-01T16:45:37Z

It's because you didn't do a rebase, you did a merge. Here are instructions on how to rebase:

https://docs.godotengine.org/en/latest/community/contributing/pr_workflow.html#the-interactive-rebase

Calinou · 2021-12-08T21:52:31Z

@AlexanderPruss Could you look into rebasing this pull request to make sure only 2a920a2 (#43694) is included? If this proves to be too difficult, you can recreate a new pull request.

Calinou · 2022-05-13T21:52:52Z

Superseded by #61008 and #61013. Thanks for the contribution nonetheless!

AlexanderPruss requested a review from a team as a code owner November 19, 2020 15:56

Calinou added documentation enhancement topic:tests labels Nov 19, 2020

Calinou added this to the 4.0 milestone Nov 19, 2020

Xrayez reviewed Nov 19, 2020

View reviewed changes

AlexanderPruss mentioned this pull request Aug 29, 2021

Add a local rotation function to Transform3D #43601

Closed

aaronfranke requested changes Aug 29, 2021

View reviewed changes

YeldhamDev and others added 22 commits September 23, 2021 21:32

Fix arrow pointing in the wrong direction in the Dock Position popup

f289b13

Expose TabContainer's tab hidding for scripts

7ce02b6

Added set_active(true) to all body direct state force apply/impulses. g…

e8efe62

…odotengine#52915

Merge pull request godotengine#52988 from Calinou/remove-pragma-once

cc23d6d

Replace `#pragma once` by traditional include guards for consistency

Merge pull request godotengine#52994 from YeldhamDev/dock_popup_arrow…

10801b9

…_fix

Add selection getter methods to LineEdit

4e0552a

Merge pull request godotengine#52939 from bruvzg/remove_v_kern

d267777

Merge pull request godotengine#52967 from danger-dan/dev_vehicle_slee…

fb4fadf

…p_fix

Update AUTHORS and DONORS list

252ae43

New contributors added to AUTHORS: @AnilBK, @Jummit Thanks to all contributors and donors for making Godot possible!

Merge pull request godotengine#38414 from theoway/Fix-GraphNode_offse…

b8a6eb6

…t_update

Add a special case for 0-time interpolations

5c1195e

Merge pull request godotengine#52430 from AnilBK/vector2-replacements

32ab77e

Merge pull request godotengine#53003 from KoBeWi/tween_0()

5e4a712

Refactor and move easing equations

11e7963

Merge pull request godotengine#50165 from KoBeWi/take_it_easing

53306cb

Add Delete All button to ItemList editor

f7e152b

Fix incorrect offsets of tooltip content in CodeEdit

e70a0d9

Fix incorrect offsets of tooltip content in `CodeEdit`

Fix tab renaming not updating its size in Tabs

edf3d22

Remove binds from Signal.connect

7bf5fc7

[Net] Enable PacketPeer native extensions.

b9e6cc8

[Net] StreamPeer GDExtension.

5f61b2c

aaronfranke removed request for a team October 1, 2021 16:58

This was referenced May 13, 2022

Document performing local rotation with Transform2D and Transform3D #61008

Closed

Add rotation unit tests for Transform3D #61013

Merged

Calinou closed this May 13, 2022

Calinou removed request for a team May 13, 2022 21:52

Calinou added the archived label May 13, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Proposal-1778: Updating Transform.rotated() docs and test cases. #43694

Proposal-1778: Updating Transform.rotated() docs and test cases. #43694

AlexanderPruss commented Nov 19, 2020 •

edited

Loading

Xrayez Nov 19, 2020 •

edited

Loading

AlexanderPruss Nov 19, 2020

Xrayez Nov 19, 2020 •

edited

Loading

AlexanderPruss Nov 22, 2020 •

edited

Loading

aaronfranke left a comment

aaronfranke Aug 29, 2021

aaronfranke Aug 29, 2021 •

edited

Loading

AlexanderPruss Oct 1, 2021

AlexanderPruss commented Oct 1, 2021

aaronfranke commented Oct 1, 2021

Calinou commented Dec 8, 2021 •

edited

Loading

Calinou commented May 13, 2022

Proposal-1778: Updating Transform.rotated() docs and test cases. #43694

Proposal-1778: Updating Transform.rotated() docs and test cases. #43694

Conversation

AlexanderPruss commented Nov 19, 2020 • edited Loading

Xrayez Nov 19, 2020 • edited Loading

Choose a reason for hiding this comment

AlexanderPruss Nov 19, 2020

Choose a reason for hiding this comment

Xrayez Nov 19, 2020 • edited Loading

Choose a reason for hiding this comment

AlexanderPruss Nov 22, 2020 • edited Loading

Choose a reason for hiding this comment

aaronfranke left a comment

Choose a reason for hiding this comment

aaronfranke Aug 29, 2021

Choose a reason for hiding this comment

aaronfranke Aug 29, 2021 • edited Loading

Choose a reason for hiding this comment

AlexanderPruss Oct 1, 2021

Choose a reason for hiding this comment

AlexanderPruss commented Oct 1, 2021

aaronfranke commented Oct 1, 2021

Calinou commented Dec 8, 2021 • edited Loading

Calinou commented May 13, 2022

AlexanderPruss commented Nov 19, 2020 •

edited

Loading

Xrayez Nov 19, 2020 •

edited

Loading

Xrayez Nov 19, 2020 •

edited

Loading

AlexanderPruss Nov 22, 2020 •

edited

Loading

aaronfranke Aug 29, 2021 •

edited

Loading

Calinou commented Dec 8, 2021 •

edited

Loading