Skip to content

Fixing bug of fn rotated in Vector3 #760

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
bors merged 2 commits into from Sep 12, 2021
Merged

Fixing bug of fn rotated in Vector3 #760

bors merged 2 commits into from Sep 12, 2021

Conversation

@jo32
Copy link
Contributor

@jo32 jo32 commented Jul 16, 2021

@jo32 jo32 mentioned this pull request Jul 16, 2021
Closed
@Bromeon
Copy link
Member

Bromeon commented Aug 7, 2021

Thanks!

This seems to be a bug in Basis::mul() rather than Vector3::rotated(). Intuitively, both basis * vector and basis.xform(vector) should do the same thing.

Indeed they do, if I run the following in GDScript:

var axis = Vector3.UP
var v = Vector3(37.51756, 20.39467, 49.96816)
var phi = -0.4927880786382844
var basis = Basis(axis, phi)

print("v.rotated(axis, phi) = ", v.rotated(axis, phi))
print("basis * v            = ", basis * v)
print("basis.xform(v)       = ", basis.xform(v))

Output:

v.rotated(axis, phi) = (9.414476, 20.39467, 61.77177)
basis * v            = (9.414476, 20.39467, 61.77177)
basis.xform(v)       = (9.414476, 20.39467, 61.77177)

Could you fix impl Mul<Vector3> for Basis instead? It should reuse the xform() implementation, there's no point in duplicating code.

@Bromeon Bromeon added bug c: core Component: core (mod core_types, object, log, init, ...) labels Aug 7, 2021
@Bromeon
Copy link
Member

Bromeon commented Aug 23, 2021

@jo32 let me know what you prefer: either fix basis * vector3 operator yourself, or if you don't have the time, I'll gladly add a commit on top of yours, so we can merge this 🙂

@Bromeon Bromeon linked an issue Aug 25, 2021 that may be closed by this pull request
Different output from Vector3::rotated in godot-rust vs GDScript #751
Closed
@Bromeon Bromeon added this to the 0.10 milestone Aug 25, 2021
@jo32
Copy link
Contributor Author

jo32 commented Sep 7, 2021

@Bromeon Thank you for your insights! Sorry for the late response and I am currently working on another urgent project for job so I have little time to continue this fix right now. I have added you to the fork repo and it would be so kind if you can help with it.

@Bromeon
Copy link
Member

Bromeon commented Sep 12, 2021

bors r+

bors bot added a commit that referenced this pull request Sep 12, 2021
@bors @Bromeon
Merge #760
fdbcdab 
760: Fixing bug of fn rotated in Vector3 r=Bromeon a=jo32

based on related code in godot: https://github.com/godotengine/godot/blob/554c776e08c9ee35fa9e2677e02f4005c11ddbc0/core/math/vector3.cpp#L36

Co-authored-by: Jan Haller <[email protected]>
@bors
Copy link
Contributor

bors bot commented Sep 12, 2021

Build failed:

@Bromeon @jo32
Fix Basis * Vector3
a22b094 
Fixes also Vector3::rotated(). Add unit test.

Co-authored-by: jo32 <[email protected]>
@Bromeon
Copy link
Member

Bromeon commented Sep 12, 2021

Rebased on latest master.
bors r+

bors bot added a commit that referenced this pull request Sep 12, 2021
@bors @Bromeon
Merge #760
302fe14 
760: Fixing bug of fn rotated in Vector3 r=Bromeon a=jo32

based on related code in godot: https://github.com/godotengine/godot/blob/554c776e08c9ee35fa9e2677e02f4005c11ddbc0/core/math/vector3.cpp#L36

Co-authored-by: Jan Haller <[email protected]>
@bors
Copy link
Contributor

bors bot commented Sep 12, 2021

Build failed:

@Bromeon
Disable clippy lint 'self-named-constructors' for Null::null()
27e57f6 
The following warning (as error) was generated:

-------------------------------------------------------
error: constructor `null` has the same name as the type
    --> gdnative-core\src\object.rs:1023:5
     |
1023 | /     pub fn null() -> Self {
1024 | |         Null(PhantomData)
1025 | |     }
     | |_____^
     |
     = note: `-D clippy::self-named-constructors` implied by `-D clippy::style`
     = help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#self_named_constructors
-------------------------------------------------------
@Bromeon
Copy link
Member

Bromeon commented Sep 12, 2021

Updated to Clippy lints from stable toolchain 1.55
bors r+

@bors
Copy link
Contributor

bors bot commented Sep 12, 2021

Build succeeded:

@bors bors bot merged commit 831958c into godot-rust:master Sep 12, 2021
@Bromeon
Copy link
Member

Bromeon commented Sep 12, 2021

Thanks for the help @jo32! I mentioned you as co-author in the commit.
You can delete the fork or remove me from it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

bug c: core Component: core (mod core_types, object, log, init, ...)

Projects

None yet

Milestone

v0.10.0

Development

Successfully merging this pull request may close these issues.

Different output from Vector3::rotated in godot-rust vs GDScript

2 participants

@jo32 @Bromeon