Skip to content

Remove Matrices #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.

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

Closed
wants to merge 465 commits into from
Closed

Remove Matrices #760

wants to merge 465 commits into from

Conversation

devshgraphicsprogramming
Copy link
Member

Description

Testing

TODO list:

Comment on lines 22 to 42
template<typename T>
inline T radians(NBL_CONST_REF_ARG(T) degrees)
{
static_assert(
is_floating_point<T>::value,
"This code expects the type to be either a double or a float."
);

return degrees * PI<T>() / T(180);
}

template<typename T>
inline T degrees(NBL_CONST_REF_ARG(T) radians)
{
static_assert(
is_floating_point<T>::value,
"This code expects the type to be either a double or a float."
);

return radians * T(180) / PI<T>();
}
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we have numbers ot something like that for PI and friends.

Also the _HLSL_VERSION case should forward to the inline SPIR-V from the std450 extended set

Comment on lines 52 to 53
DEFINE_MUL_QUATERNION_BY_SCALAR_OPERATOR(uint32_t)
DEFINE_MUL_QUATERNION_BY_SCALAR_OPERATOR(uint64_t)
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this is a mistake, only provide mul with T

AnastaZIuk added a commit that referenced this pull request Oct 25, 2024
AnastaZIuk added a commit that referenced this pull request Oct 25, 2024
…H (ambiguity dependent type issues), reference #760
AnastaZIuk added a commit to Devsh-Graphics-Programming/Nabla-Examples-and-Tests that referenced this pull request Oct 25, 2024
@Przemog1 Przemog1 changed the base branch from master to cameraz November 4, 2024 13:28

namespace nbl
{
namespace hlsl
{

// TODO: if `IdentityFloat32_t3x4` and `IdentityFloat32_t3x4` constexprs are ok, then I can expand them into templated struct, not doing it untill the concept is approved
//template<typename T, uint32_t N, uint32_t M>
//struct IdentityMatrix
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

add

template<typename T>
concept MatrixType = requires 
{
  typename T::Base::row_type;
  typename T::Base::col_type;
  typename T::Base::value_type;
} && std::is_base_of_v<glm::mat<T::Base::row_type::length(), T::Base::col_type::length(), typename T::Base::value_type>, typename T::Base>;

to matrix.hlsl just after definition (but ask @devshgraphicsprogramming if it should be named MatrixType)

then create diagonal utility

template<MatrixType Matrix>
constexpr inline Matrix diagonal(float scalar)
{
  return Matrix(scalar);
}

you can also template the scalar actually and make it value_type of the Matrix, now note that

  • matrix<T, N, M>(scalar) creates a diagonal matrix with scalar put on the diagonal
  • if your N == M then matrix<T, N, M>(1.f) is the identity matrix you need

so following diagonal create identity function (not struct!), you should be able to

using any_square_matrix_t = float32_t3x3;
constexpr auto bruh = hlsl::identity<any_square_matrix_t>()

have constraints on the template parameter, let's allow only for square matrices

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

but this is not C++ only file

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

so we can't use concepts..

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

so we can't use concepts..

hehe, you can! 492a0ad

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  typename T::Base::row_type;
  typename T::Base::col_type;
  typename T::Base::value_type;

this is specific to GLM and won't pass in plain HLSL

//{
//
//};
NBL_CONSTEXPR hlsl::float32_t3x4 IdentityFloat32_t3x4 =
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lets not hardcode and follow this comment

//};
NBL_CONSTEXPR hlsl::float32_t3x4 IdentityFloat32_t3x4 =
hlsl::float32_t3x4(hlsl::float32_t4(1, 0, 0, 0), hlsl::float32_t4(0, 0, 1, 0), hlsl::float32_t4(0, 0, 1, 0));
NBL_CONSTEXPR hlsl::float32_t4x4 IdentityFloat32_t4x4 =
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lets not hardcode and follow this comment

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

template<typename T>
matrix<T, 4, 4> getMatrix3x4As4x4(const matrix<T, 3, 4>& mat)
inline core::vectorSIMDf transformVector(const matrix<T, 4, 4>& mat, const core::vectorSIMDf& vec)
Copy link
Member

@AnastaZIuk AnastaZIuk Nov 4, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

shouldn't we also remove SIMD vectors?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

will do in other pr

matrix<T, 4, 4> getMatrix3x4As4x4(const matrix<T, 3, 4>& mat)
inline core::vectorSIMDf transformVector(const matrix<T, 4, 4>& mat, const core::vectorSIMDf& vec)
{
core::vectorSIMDf output;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I will remove every occurance of the simd vector when I work on it, no need to spam xd

namespace transformation_matrix_utils_impl
{
template<typename T>
inline T determinant_helper(const matrix<T, 3, 3>& mat, vector<T, 3>& r1crossr2)
Copy link
Member

@AnastaZIuk AnastaZIuk Nov 4, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

please document that your returned determinant is yielded by scalar triple product

const vector<T, 3> zaxis = glm::normalize(position - target);
const vector<T, 3> xaxis = glm::normalize(hlsl::cross(upVector, zaxis));
const vector<T, 3> yaxis = hlsl::cross(zaxis, xaxis);
static_assert(N >= 3 && M >= 3);
Copy link
Member

@AnastaZIuk AnastaZIuk Nov 4, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

instead of doing static_assert lets have it in requires clause

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can't do since it is not C++ only file

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

you can, NBL_FUNC_REQUIRES

const vector<T, 3>& target,
const vector<T, 3>& upVector)
template<typename T, uint32_t N, uint32_t M>
inline matrix<T, 3, 3> getSub3x3TransposeCofactors(const matrix<T, N, M>& mat)
Copy link
Member

@AnastaZIuk AnastaZIuk Nov 4, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

please document it returns adjugate (transpose of the cofactor matrix) of sub 3x3 mat matrix

NBL_FORCE_INLINE T dot(const T& a, const T& b);
NBL_FORCE_INLINE T dot(const T& a, const T& b)
{
static_assert(!(std::is_same_v<T, hlsl::float32_t2> || std::is_same_v<T, hlsl::float32_t3> || std::is_same_v<T, hlsl::float32_t4>));
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

move to requires clause

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can't do since it is not C++ only file

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

you can, NBL_FUNC_REQUIRES

Fletterio and others added 27 commits January 17, 2025 14:40
Signed-off-by: Ali Cheraghi <[email protected]>
Signed-off-by: Ali Cheraghi <[email protected]>
Fix false positive aspect mask error raising
video: free command buffers when destroyed
@Przemog1 Przemog1 closed this Feb 7, 2025
@Przemog1 Przemog1 deleted the rm_core_mat_vec branch February 7, 2025 10:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

10 participants