-
Notifications
You must be signed in to change notification settings - Fork 67
New quaternion in HLSL #960
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
| static this_t create() | ||
| { | ||
| this_t q; | ||
| q.data = data_type(0.0, 0.0, 0.0, 1.0); | ||
| return q; | ||
| } |
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.
i think the create overloads need to have their argument types templated with NBL_FUNC_REQUIRES because DXC will screw us over with implicit conversions
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.
ping
| const AsUint negationMask = hlsl::bit_cast<AsUint>(totalPseudoAngle) & AsUint(0x80000000u); | ||
| const data_type adjEnd = hlsl::bit_cast<scalar_type>(hlsl::bit_cast<AsUint>(end.data) ^ negationMask); | ||
|
|
||
| this_t retval; | ||
| retval.data = hlsl::mix(start.data, adjEnd, fraction); |
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 unit test/check it somehow (re-derive) I'm not sure that when totalPseudoAngle<0 the result is meant to be S+(-E-S)*fraction and not `S+(E-S)*(-fraction)
P.S. btw my recent learnings about GPU architectures have made me aware that there's more FP32 cores than INT32 on Nvidia so write a comment to benchmark the sign xor flip against just *sign(totalPseudoAngle)
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.
@keptsecret did you re-derive ?
| mat[1][1] = scalar_type(0.5) - mat[1][1]; | ||
| mat[2][2] = scalar_type(0.5) - mat[2][2]; | ||
| mat *= scalar_type(2.0); | ||
| return hlsl::transpose(mat); // TODO: double check transpose? |
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.
yes please double check the tranpose, best add some unit tests to example 22 where random vectors get rotated by random quaternions, and you check the result is same as making a mat3 out of the quaternion and mul(matrix,inputVector)
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.
@Przemog1 can advise/help
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.
sure, also example 22 doesn't compile on master currently, for now just comment out the code that doesn't work
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.
if you took this from old C++ code involving matrixSIMD, the operator[] gave rows, so if the tranpose was there in the old C++ it needs to be here cause HLSL gives rows too
But you can ofc rewrite this function (keep the old one for reference in the unit test) to work on a row basis.
or you can get rid of the whole * vector3_type( 1.0,-1.0, 1.0); thing, and just write out the swizzle element by element
…rm variants for lerp/flerp
| // angle: Rotation angle expressed in radians. | ||
| // axis: Rotation axis, must be normalized. | ||
| static this_t create(scalar_type angle, const vector3_type axis) | ||
| static this_t create(const vector3_type axis, scalar_type angle) |
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.
add a const float uniformScale = 1.f argument which basically multiples the whole q.data
| static bool __isEqual(const scalar_type a, const scalar_type b) | ||
| { | ||
| return hlsl::max(a/b, b/a) <= scalar_type(1e-4); | ||
| } | ||
| static bool __dotIsZero(const vector3_type a, const vector3_type b) | ||
| { | ||
| const scalar_type ab = hlsl::dot(a, b); | ||
| return hlsl::abs(ab) <= scalar_type(1e-4); | ||
| } |
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.
this needs go somewhere in a common header, can't have it in every class
| // only orthogonal and uniform scale mats can be converted | ||
| bool valid = __dotIsZero(m[0], m[1]); | ||
| valid = __dotIsZero(m[1], m[2]) && valid; | ||
| valid = __dotIsZero(m[0], m[2]) && valid; | ||
|
|
||
| const matrix_type m_T = hlsl::transpose(m); | ||
| const scalar_type dotCol0 = hlsl::dot(m_T[0],m_T[0]); | ||
| const scalar_type dotCol1 = hlsl::dot(m_T[1],m_T[1]); | ||
| const scalar_type dotCol2 = hlsl::dot(m_T[2],m_T[2]); | ||
| valid = __isEqual(dotCol0, dotCol1) && valid; | ||
| valid = __isEqual(dotCol1, dotCol2) && valid; | ||
| valid = __isEqual(dotCol0, dotCol2) && valid; |
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.
you need to pack this up into nbl::hlsl::math::linalg struct RuntimeTraits<SquareMatrix> which has bool members for:
- invertible/nonZeroDeterminant
- orthogonal
- uniform scale
- orthonormal
| const scalar_type neg_m00 = bit_cast<scalar_type>(bit_cast<AsUint>(m00)^0x80000000u); | ||
| const scalar_type neg_m11 = bit_cast<scalar_type>(bit_cast<AsUint>(m11)^0x80000000u); |
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.
you can use unary - no problem here, will be just as fast, but more legible
| } | ||
| } | ||
|
|
||
| retval.data = hlsl::normalize(retval.data); |
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.
uniform scale needs to be restored from the matrix, so its the inversesqrt(dot(m.anyColumn,m.anyColumn))
| } | ||
|
|
||
| static this_t lerp(const this_t start, const this_t end, const scalar_type fraction, const scalar_type totalPseudoAngle) | ||
| static this_t unnormLerp(const this_t start, const this_t end, const scalar_type fraction, const scalar_type totalPseudoAngle) |
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.
you want to assert both start and end have unit length, otherwise algo doesn't work
| static this_t flerp(const this_t start, const this_t end, const scalar_type fraction) | ||
| static this_t unnormFlerp(const this_t start, const this_t end, const scalar_type fraction) | ||
| { | ||
| const scalar_type pseudoAngle = hlsl::dot(start.data,end.data); |
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.
you want to assert both start and end have unit length, otherwise algo doesn't work
| vector3_type transformVector(const vector3_type v, const bool assumeNoScale=false) NBL_CONST_MEMBER_FUNC | ||
| { | ||
| scalar_type scale = hlsl::length(data); | ||
| scalar_type scale = hlsl::mix(hlsl::length(data), scalar_type(1.0), assumeNoScale); | ||
| vector3_type direction = data.xyz; | ||
| return v * scale + hlsl::cross(direction, v * data.w + hlsl::cross(direction, v)) * scalar_type(2.0); |
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.
almost correct, if you have a scale:
| Ds x (Vsw + Ds x V)| = s^2 |D x (Vw + D x V)|
you should compute the scale as scaleRcp = inverseSqrt(dot(data,data)), and then do:
vector3_type modV = v*2.f*scaleRcp;
return v/scaleRcp + hlsl::cross(direction, modV * data.w + hlsl::cross(direction,modV));
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.
the choice of the location of the 2.f multiplier is up to you really.
| { | ||
| this_t retval; | ||
| math::truncated_quaternion<T> retval; | ||
| retval.data = hlsl::normalize(q.data); |
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.
truncated quaternion is normalized by definition (last component is dropped), this should be a NOOP
| static inline math::truncated_quaternion<T> cast(const math::quaternion<T> q) | ||
| { | ||
| math::truncated_quaternion<T> t; |
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.
assert the input quaternion has unit scale
|
will probably need a new PR once you merge |
No description provided.