I hope Core module is the right category for this?
When running the tests the test geo_node_mesh_test_sample_uv_surface sometimes fails. When debugging this I traced it back to the call:
line 167: dst_typed[i] = {};
In the file source/blender/nodes/geometry/nodes/node_geo_sample_uv_surface.cc .
This calls the default constructor of the current type the template is instantiated with. In this specific test it’s a derived class of ColorRGBA .
The class ColorRGBA has an empty constructor, it’s defined as:
ColorRGBA() = default;
Which is equivalent to:
ColorRGBA()
{
}
This does not initialize the member variable r,g,b and a, leading to undefined values in the original assignment to dst_type[i] .
I assume the default constructor for ColorRGBA is defined to not initialize the member variable on purpose, to prevent unnecessarily paying the cost of initializing when allocating large arrays of ColorRGBA’s which will be written to shortly after.
Should we just add initializing constructors to all types that are used as attributes? Or should we use another way to get a well defined default value in a templated setting like this?
Created a bugreport so this is not forgotten about:
https://developer.blender.org/T103432
Array of colors will filled by zero only in case if you pass to array constructor zero color
Array<ColorGeometry4f> colors(size, {});
No it isn’t. Not on ubuntu gcc 11.3. And not per the spec.
The C++ spec sais that
‘’’
ColorRGBA() = default;
‘’’
Is equivalent to
‘’’
ColorRGBA::ColorRGBA()
{
}
‘’’
Which does not initialize the member variables.
I don’t know if the constructor of Array maybe zeroes out the memory before use?
The array constructor should not do this unless a 2nd argument is passed.
So yes, if there is a strange behavior here, then it is in the color class.
After more looking into it:
As I understand the spec the =default constructor makes ColorRGBA ‘trivially constructible’. So there is a difference, because an empty constructor gets called (and does nothing) but ‘trivially constructible’ (if I understand it correctly) ,means no constructor is called at all. This can lead to different results, because the first one ‘pollutes’ the stack with function call stuff while the second one does not.
But still the members are uninitialized.
1 Like
In this picture, the colored areas = the result of the work 167 line.
So yes, it looks like you’re right, the colors were never initialized to zero…
Which is strange.
The spec is very unclear (to me) but all small test programs I created (so far) did actually initialize the member variables to zero.
I recreated the exact same class derivation:
#include <stdio.h>
template <typename T, typename T2>
class A
{
public:
constexpr A() = default;
A(T _a, T _b)
: a(_a), b(_b)
{
}
T a;
T b;
T2 c;
};
template <typename T> class B final
: public A<float, T>
{
public:
constexpr B() : A<float, T>()
{
}
};
int main(int argc, char **argv)
{
{
unsigned x = 0xDEAFBEEF;
printf("%x \n",x);
}
{
B<int> test2 = {};
printf(" %f %f %d\n", test2.a, test2.b, test2.c);
}
}
Running this in valgrind does not show uninitialized values.
If I replace the A() = default; with A() { } ir does show uninitialized values. So apparently using =default; should cause the memory to be zeroed. But in this specific case it doesn’t work.
If I replace
colorRGBA() = default;
with
ColorRGBA()
: r(), g(), b(), a()
{
}
The problem goes away. But at the cost of an extra function call for each initialisation of ColorRGBA, which I guess we don’t want.
A better solution by @deadpin:
replace the empty constructor of ColorSceneLinear4f with a default constructor:
diff --git a/source/blender/blenlib/BLI_color.hh b/source/blender/blenlib/BLI_color.hh
index b1b9aeb17f1..0256cec667c 100644
--- a/source/blender/blenlib/BLI_color.hh
+++ b/source/blender/blenlib/BLI_color.hh
@@ -152,9 +152,7 @@ BLI_INLINE ColorTheme4<uint8_t> BLI_color_convert_to_theme4b(const ColorTheme4<f
template<eAlpha Alpha>
class ColorSceneLinear4f final : public ColorRGBA<float, eSpace::SceneLinear, Alpha> {
public:
- constexpr ColorSceneLinear4f<Alpha>() : ColorRGBA<float, eSpace::SceneLinear, Alpha>()
- {
- }
+ constexpr ColorSceneLinear4f<Alpha>() = default;
constexpr ColorSceneLinear4f<Alpha>(const float *rgba)
: ColorRGBA<float, eSpace::SceneLinear, Alpha>(rgba)
Should theoretically be no different from what was there before, but this does work.
2 Likes