Page MenuHome

BLI: Refactor vector types & functions to use templates
ClosedPublic

Authored by Clément Foucault (fclem) on Jan 10 2022, 8:37 PM.

Details

Summary

This patch implements the vector types (i.e:float2) by making heavy
usage of templating. All vector functions are now outside of the vector
classes (inside the blender::math namespace) and are not vector size
dependent for the most part.

In the ongoing effort to make shaders less GL centric, we are aiming
to share more code between GLSL and C++ to avoid code duplication.

Motivations:
  • We are aiming to share UBO and SSBO structures between GLSL and C++. This means we will use many of the existing vector types and others we currently don't have (uintX, intX). All these variations were asking for many more code duplication.
  • Deduplicate existing code which is duplicated for each vector size.
  • We also want to share small functions. Which means that vector functions should be static and not in the class namespace.
  • Reduce friction to use these types in new projects due to their incompleteness.
  • The current state of the BLI_(float|double|mpq)(2|3|4).hh is a bit of a let down. Most clases are incomplete, out of sync with each others with different codestyles, and some functions that should be static are not (i.e: float3::reflect()).
Upsides:
  • Still support .x, .y, .z, .w for readability.
  • Compact, readable and easilly extendable.
  • All of the vector functions are available for all the vectors types and can be restricted to certain types. Also template specialization let us define exception for special class (like mpq).
  • With optimization ON, the compiler unroll the loops and performance is the same.
Downsides:
  • Might impact debugability. Though I would arge that the bugs are rarelly caused by the vector class itself (since the operations are quite trivial) but by the type conversions.
  • Might impact compile time. I did not saw a significant impact since the usage is not really widespread.
  • Functions needs to be rewritten to support arbitrary vector length. For instance, one can't call len_squared_v3v3 in math::length_squared() and call it a day.
  • Type cast does not work with the template version of the math:: vector functions. Meaning you need to manually cast float * and (float *)[3] to float3 for the function calls. i.e: math::distance_squared(float3(nearest.co), positions[i]);
  • Some parts might loose in readability: float3::dot(v1.normalized(), v2.normalized()) becoming math::dot(math::normalize(v1), math::normalize(v2)) But I propose, when appropriate, to use using namespace blender::math; on function local or file scope to increase readability. dot(normalize(v1), normalize(v2))
Consideration:
  • Include back .length() method. It is quite handy and is more C++ oriented.
  • I considered the GLM library as a candidate for replacement. It felt like too much for what we need and would be difficult to extend / modify to our needs.
  • I used Macros to reduce code in operators declaration and potential copy paste bugs. This could reduce debugability and could be reverted.
  • This touches delaunay_2d.cc and the intersection code. I would like to know @Howard Trickey (howardt) opinion on the matter.
  • The noexcept on the copy constructor of mpq(2|3) is being removed. But according to @Jacques Lucke (JacquesLucke) it is not a real problem for now.

I would like to give a huge thanks to @Jacques Lucke (JacquesLucke) who helped during this
and pushed me to reduce the duplication further.

Diff Detail

Repository
rB Blender
Branch
tmp-vector-template
Build Status
Buildable 19874
Build 19874: arc lint + arc unit

Event Timeline

Clément Foucault (fclem) requested review of this revision.Jan 10 2022, 8:37 PM
Clément Foucault (fclem) created this revision.

On a design level this all makes sense to me, I only skimmed the code but don't see an issue.

This revision is now accepted and ready to land.Jan 10 2022, 9:51 PM

The changes to the mpq2/3 files and the delaunay code look fine to me, assuming this all compiles and the unit tests pass (I didn't test myself).

Kicked off the bots, https://builder.blender.org/admin/#/builders/18/builds/258

they are still running, but looks like windows has some minor issues building.

  • Fix template specialization error
  • Use using instead of typedef inside of vec_base
  • Set uint_type as void if type is larger than 64bits
  • Merge branch 'master' into tmp-vector-template

This does look nice! I did like the length, length_squared, and normalized methods, I wonder if it would be possible to keep those? Though if it's important to the design not to have them, obviously I can adapt.

source/blender/blenlib/BLI_memory_utils.hh
567

Is it intentional to remove this? It was just recently added.

This does look nice! I did like the length, length_squared, and normalized methods, I wonder if it would be possible to keep those? Though if it's important to the design not to have them, obviously I can adapt.

I like the idea of having only one way to do things and keeping all non operators outside of the class. It is not really that important to the design, but I think we should stick to one rule so that no one is tempted to add more random methods to the class. Ultimately leading to the discrepancy we have now. I'm even considering moving the is_zero and is_any_zero outside of the class. The hash() method is here only so that the type can be used as key for hashmaps.

The only two thing I can think of in the defense of keeping length() and length_squared() inside the class (as alias) is that they can be considered as properties of the vector and avoiding the math:: namespace specification. On the other hand, normalized() is a bit odd semantically.

But this is all quite pedantic in my opinion.

source/blender/blenlib/BLI_memory_utils.hh
567

It is moved to BLI_utildefines.h as per request of @Jacques Lucke (JacquesLucke). But I agree this should have been done in master outside of this commit.

On a design and use level is a huge +1 :)

Some comments in the implementation side.

source/blender/blenkernel/intern/object_dupli.cc
73–79

Either stick to using-what-needed or using the nanespace. The Former is preferred.
I see similar comment applicable in other files.

P.S. Other thing to consider is to put functionality of these .cc files into the blender namespace (that's where they belong to anyway). NOT saying it should be mixed with this patch.

source/blender/blenlib/BLI_math_vec_types.hh
97

What's the issue with templates?

241–253

Will having those as a templated functions and passing operation as a functor unroll to the same code?
Probably @Jacques Lucke (JacquesLucke) have experience with such things :)

Wouldn't mind knowinfg answers to my couple of questions, and wouldn't min Jacques going over as the code as well before the patch is officially in the landable status.

This revision now requires review to proceed.Jan 11 2022, 9:36 AM
Jacques Lucke (JacquesLucke) requested changes to this revision.Jan 11 2022, 11:58 AM

Overall looks great, but got a few smaller comments.

I also prepared compiler explorer so that we can see what code gcc generates given this math library: https://godbolt.org/z/PWvjrv1ET (you can change the function at the very bottom).

source/blender/blenkernel/intern/object_dupli.cc
73–79

Agreed, don't just add using namespace blender; everywhere.

source/blender/blenlib/BLI_double2.hh
27

I think all of these basic types could just be declared at the bottom of BLI_math_vec_types.hh now. No need to keep all these small headers around.
The headers for mpq can stay for now though.

source/blender/blenlib/BLI_math_vec_types.hh
241–253

Well, gcc with -O2 might not.. With -O3 it would unroll and vectorize it.

You can test how expressions are compiled with gcc here: https://godbolt.org/z/PWvjrv1ET
Can't test clang or msvc right now unfortunately.

I don't think that's blocking though, it's something we can look into separately. The compilers are certainly able to optimize these loops.

495

Add asserts that b is not zero.

518

Use early return instead of result variable. Same below.

source/blender/blenlib/BLI_math_vector.hh
55

Does this work instead?
std::is_floating_point_v<typename T::base_type> || std::is_same_v<typename T::base_type, mpq_class>

59

Use std::is_floating_point_v.

62

Use std::is_integral_v.

This revision now requires changes to proceed.Jan 11 2022, 11:58 AM
Clément Foucault (fclem) marked 12 inline comments as done.
  • Use early return instead of result variable.
  • Move is_zero and is_any_zero to blender::math namespace.
  • Add asserts that modulo second argument is not zero.
  • Simplify template macros
  • Deduplicate cross_poly implementation
  • Remove file scope using namespace blender;
  • Group vector types declaration into BLI_math_vec_types.hh and BLI_math_vec_mpq_types.hh
  • Change comment to be more meaningful.
  • Merge branch 'master' into tmp-vector-template
Jacques Lucke (JacquesLucke) accepted this revision.EditedJan 11 2022, 4:56 PM
source/blender/blenlib/BLI_double3.hh
34

This file and BLI_double2.hh can be removed as well.

From quick reading seems good!

This revision is now accepted and ready to land.Jan 11 2022, 5:05 PM
  • Remove BLI_double2|3.hh files
  • Fix type conversion error on MSVC (thanks @Jacques Lucke (JacquesLucke) for that)
  • Add clamp() to templated functions
  • Add constructor tests
  • Make mixed vec4 type constructor works with different vector types
Jacques Lucke (JacquesLucke) added inline comments.
source/blender/blenlib/tests/BLI_math_vec_types_test.cc
150 ↗(On Diff #46921)

Missing newline.

Clément Foucault (fclem) marked an inline comment as done.
  • Fix missing new line