-
-
Notifications
You must be signed in to change notification settings - Fork 2.2k
math: fix vec2,3,4 project not using the right formulas #25813
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.
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
base: master
Are you sure you want to change the base?
Conversation
… functions added documentation for some methods and fixed project and abs vec probably a lot more work
Refactor project function to handle zero vector case and calculate projection properly
spytheman
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please add tests for the changed .project methods, to prevent future regressions.
Added tests for vec3 projection functionality, including basic projection and projection onto a zero vector.
|
You have to also run |
|
Lets wait for my test(s). |
|
My research. @spytheman Yes, the current implementation is incorrect. I've checked everything and I'm not approving the merge. I have several sources to prove that his implementation is not correct. @gw-dev101 Thanks for working on this. I appreciate it. You got confused in the details and parameters, fix the code, comments, tests and come back with new commit(s). |
|
That's the main issue with trying to make a pr at 2am |
|
Not a waste of time - it brought an issue up that not many were even thinking about. You also don't need to make a second PR - just push modifications to this one. |
|
Is anyone working on this issue currently ? I can try to fix some check the weren't successful if nobody is on it already |
|
I do not understand why, but it seems to be a issue import math.vec
fn main() {
v1 := vec.vec2(3.0, 4.0) // magnitude 5 vector
v2 := vec.vec2(5.0, 6.0) // magnitude ~7.81 vector
proj := v1.project(v2)
solution := vec.vec2(4.68, 6.24)
denom := v1.dot(v1)
scale := v2.dot(v1) / denom
proj2 := vec.Vec2[f64]{v1.x * scale, v1.y * scale}
println('x: $proj.x $proj2.x $solution.x')
println('y: $proj.y $proj2.y $solution.y')
println('Vec: $proj $proj2 $solution')
}Returns x: proj: 5.0 proj2: 4.68 solution: 4.68
y: proj: 6.0 proj2: 6.24 solution: 6.24
Vec: proj: vec.Vec2[f64]{
x: 5.0
y: 6.0
} proj2: vec.Vec2[f64]{
x: 4.68
y: 6.24
} solution: vec.Vec2[f64]{
x: 4.68
y: 6.24
}when using pub fn (v Vec2[T]) project(u Vec2[T]) Vec2[T] {
denom := T(v.dot(v))
if denom <= vec_epsilon {
return Vec2[T]{T(0.0), T(0.0)}
}
scale := u.dot(v) / denom
return Vec2[T]{v.x * scale, v.y * scale}
}While it should have been the same result with the function and with the equivalent code used in the main |
|
The generic may not be involved, by defining a function directly in the file: fn project_intern[T](v vec.Vec2[T], u vec.Vec2[T]) vec.Vec2[T] {
denom := v.dot(v)
if denom <= vec.vec_epsilon {
return vec.Vec2[T]{T(0.0), T(0.0)}
}
scale := u.dot(v) / denom
return vec.Vec2[T]{v.x * scale, v.y * scale}
}It works as entended: proj3 := project_intern(v1, v2)returns proj3: vec.Vec2[f64]{
x: 4.68
y: 6.24
} |
|
@gw-dev101 pushed wrong commits and we are waiting for him. |
It isn't an issue, I was juste using the wrong compilation of V, so it didn't uses the version of math.vec I was working on |
|
There is a fix for This is a candidate for comparison. commit 28a0a03663fa359da954816d0affbaa3fc0163b8 (HEAD -> vec_project)
Author: Mike <[email protected]>
Date: Tue Nov 25 20:39:48 2025 +0200
math.vec: fix
diff --git a/vlib/math/vec/vec3.v b/vlib/math/vec/vec3.v
index 6f82d231e..5e72616f1 100644
--- a/vlib/math/vec/vec3.v
+++ b/vlib/math/vec/vec3.v
@@ -261,7 +261,7 @@ pub fn (v Vec3[T]) perpendicular(u Vec3[T]) Vec3[T] {
// project returns the projected vector.
pub fn (v Vec3[T]) project(u Vec3[T]) Vec3[T] {
- percent := v.dot(u) / u.dot(v)
+ percent := v.dot(u) / u.dot(u)
return u.mul_scalar(percent)
} |
This doesn't work for u : (0; 0; 0) If you project any vector on the null vector, it must return a null vector or a nan vector ? |
i see |
|
Si it is intended that It feel strange because, if you project into a vector that doesn't existe, the line passing by this vectors also doesn't exist I am sorry if I am not clear enought |
|
that was my assumption too |
@Linklancien @gw-dev101 - |
Makes sense, I will change accordigly |
|
@Linklancien @gw-dev101 python's |
|
unreal engine one does return the zero vector |
import numpy as np
v = np.array([1.0, 0.0, 0.0])
u = np.array([0.0, 0.0, 0.0])
projection = u * (np.dot(v, u) / np.dot(u, u))
print(projection)which generates a warning and |
I am ok with returning a nan fn test_vec2_project_zero_vector() {
v1 := vec.vec2(0.0, 0.0)
v2 := vec.vec2(5.0, 6.0)
proj := v1.project(v2)
println(proj)
assert proj.x == nan
assert proj.y == nan
}When using this code it doesn't work, do you know a way ? |
i am using math.is_nan() |
|
NaNs are weird in that by definition they are fn test_vec2_project_zero_vector() {
v1 := vec.vec2(0.0, 0.0)
v2 := vec.vec2(5.0, 6.0)
proj := v1.project(v2)
println(proj)
assert proj.x != proj.x
assert proj.y != proj.y
}this should work |
This indeed work, maybe math.is_nan() is more explicit ? |
|
a quick check tell me that math.isnan() is doing exactly that so yeah |
test_vec2_project_zero_vector test_vec2_project_onto_zero
Wrong copy paste
|
Remember: Marking something |
|
i sure hope marking something inline never make it |
|
No, the compiler can completely ignore it. It will automatically inline things that are worthwhile to inline, and refuse to inline things that aren't. Adding that marker just tells the compiler you would like for it to be inlined, but as I said, it can ignore it. |
|
We can probably fix it on the V side by inlining it ourselves :) |
|
on the v side isnan(x) is basically return x!=xso it should inline pretty nicely |
|
All the test files must passed with this push, it least it does on my PC |
addresses #25811 by replacing some of the formulas with the correct ones
(the old one only worked when both vectors had the same magnitude)
i think there is still some more work to be done on the module before it is satifactory