min() is bugged in vector2.h

[HardCoreCodin]

Problem

The following two functions in the "vector2.h" header are bugged.

1. vector2 add(vector2 a, vector2 b)
2. vector2 add(vector2 a, float b)

The first has the bug, the second uses the first so gets bugged as well.
Looks like a copy/pate bug that just got left in there, trivial to fix (not bothering with a PR)

Expected behavior:
The minimum value between the 2 arguments should be in the result, for both components

Actual behavior:
The result contains just the first component of the first argument, and the second component of the second argument - regardless.

Steps to Reproduce

Write a simple shader like that uses this function from the vector2.h header:

#include "vector2.h"

shader Texture(
    output color value = 0,
    vector2 a = {1, 0},
    vector2 b = {0, 1})
{
    vector2 c = min(a, b);
    value = color(c.x, c.y, 0.0);
}

Result should be black, not yellow.

Versions

all (I suppose)

[AlexMWells]

src/shaders/vector2.h

vector2 min(vector2 a, vector2 b)
{
    return vector2 (min(a.x, a.x),
                    min(b.y, b.y));
}

which is clearly broken, it should be

vector2 min(vector2 a, vector2 b)
{
    return vector2 (min(a.x, b.x),
                    min(a.y, b.y));
}

More over there should be some tests added to exercise all these operations.

[AlexMWells]

Code review also shows that dividing by an integer is bugged.
Reproducer:

#include "vector2.h"

vector2 current__operator__div__(vector2 a, int b)
{
    float b_inv = 1/b; // nothing promoted to float, so b_inv will be 0|-0 for all b values other than 1|-1
    return a * vector2(b_inv, b_inv);
}

vector2 fixed__operator__div__(vector2 a, int b)
{
    float b_inv = 1.0/b;
    return a * vector2(b_inv, b_inv);
}

shader do_div(
    output color value = 0,
    vector2 a = {2.5, 1},
    int b = 5)
{
    vector2 c = a/b; // incorrect
//    vector2 c = __operator__div__(a,b); // incorrect
//    vector2 c = current__operator__div__(a,b); // incorrect
//    vector2 c = fixed__operator__div__(a,b); // correct
    value = color(c.x, c.y, 0.0);
}

[AlexMWells]

Also dividing by integer for vector4 is bugged in the same manner:

#include "vector4.h"

vector4 current__operator__div__(vector4 a, int b)
{
    float b_inv = 1/b; // nothing promoted to float, so b_inv will be 0|-0 for all b values other than 1|-1
    return a * vector4(b_inv, b_inv, b_inv, b_inv);
}

vector4 fixed__operator__div__(vector4 a, int b)
{
    float b_inv = 1.0/b;
    return a * vector4(b_inv, b_inv, b_inv, b_inv);
}

shader do_div4(
    output color value = 0,
    vector4 a = {3.5, 2, 1, 0.5},
    int b = 5)
{
    vector4 c = a/b; // incorrect
//    vector4 c = __operator__div__(a,b); // incorrect
//    vector4 c = current__operator__div__(a,b); // incorrect
//    vector4 c = fixed__operator__div__(a,b); // correct
    value = color(c.x, c.y, c.z + c.w);
}

[AlexMWells]

Even more troubling, code review shows that Color2 * Color2 is broken:
Currently:

color2 __operator__mul__(color2 a, color2 b)
{
    return color2(a.r * a.r, b.a * b.a);
}

When it should be

color2 __operator__mul__(color2 a, color2 b)
{
    return color2(a.r * b.r, a.a * b.a);
}

[AlexMWells]

Dividing by integer for color2 also exhibits same bug, reproducer:

#include "color2.h"

color2 current__operator__div__(color2 a, int b)
{
    float b_inv = 1/b; // nothing promoted to float, so b_inv will be 0|-0 for all b values other than 1|-1
    return a * color2(b_inv, b_inv);
    // bug in color2*color2 operator means the result will be color2(a.x*a.x, b_inv*binv)
}

color2 fixed__operator__div__(color2 a, int b)
{
    float b_inv = 1.0/b; 
	// do to different bug in color2*color2 we can't use that operator
	//    return a * color2(b_inv, b_inv);
	// so expand the per component multiply
    return color2(a.r * b_inv, a.a * b_inv);
}

shader do_color2_div(
    output color value = 0,
    color2 a = {2.5, 1},
    int b = 5)
{
    color2 c = a/b; // incorrect
//    color2 c = __operator__div__(a,b); // incorrect
//    color2 c = current__operator__div__(a,b); // incorrect
//    color2 c = fixed__operator__div__(a,b); // correct
    value = color(c.r, c.a, 0.0);
}

[AlexMWells]

Also color2 + color2 is bugged.
existing broken version:

color2 __operator__add__(color2 a, color2 b)
{
    return color2(a.r + a.r, b.a + b.a);
}

Should be:

color2 __operator__add__(color2 a, color2 b)
{
    return color2(a.r + b.r, a.a + b.a);
}

[lgritz]

Oof, this is all pretty bad. Does this mean that nobody actually uses these classes?

[AlexMWells]

int - color2 and is incorrect, reproducer/fix:

#include "color2.h"

color2 current__operator__sub__(int a, color2 b)
{
    return b - color2(a, a);
}

color2 fixed__operator__sub__(int a, color2 b)
{
    return color2(a, a) - b;
}

shader do_int_sub_color2(
    output color value = 0,
    color2 b = {0.75, 0.25},
    int a = 1)
{
    color2 c = a - b; // incorrect
//    color2 c = __operator__sub__(a,b); // incorrect
//    color2 c = current__operator__sub__(a,b); // incorrect
//    color2 c = fixed__operator__sub__(a,b); // correct

    value = color(c.r, c.a, 0.0);
}

float - color2 and is incorrect, reproducer/fix:

#include "color2.h"

color2 current__operator__sub__(float a, color2 b)
{
    return b - color2(a, a);
}

color2 fixed__operator__sub__(float a, color2 b)
{
    return color2(a, a) - b;
}

shader do_float_sub_color2(
    output color value = 0,
    color2 b = {0.75, 0.25},
    float a = 1)
{
    color2 c = a - b; // incorrect
//    color2 c = __operator__sub__(a,b); // incorrect
//    color2 c = current__operator__sub__(a,b); // incorrect
//    color2 c = fixed__operator__sub__(a,b); // correct

    value = color(c.r, c.a, 0.0);
}

[AlexMWells]

color2 == color2 is incorrect, reproducer/fix:

#include "color2.h"

int current__operator__eq__(color2 a, color2 b)
{
    return (a.r == a.r) && (b.a == b.a); // broken:  always returns true
}

int fixed__operator__eq__(color2 a, color2 b)
{
    return (a.r == b.r) && (a.a == b.a);
}

shader do_color2_eq(
    output color value = 0,
    color2 a = {2.5, 1},
    color2 b = {5, 6})
{
    int is_eq = a == b; // incorrect
//    int is_eq = __operator__eq__(a,b); // incorrect
//    int is_eq = current__operator__eq__(a,b); // incorrect
//    int is_eq = fixed__operator__eq__(a,b); // correct
    value = color(is_eq, is_eq, is_eq);
}

[AlexMWells]

fmod(color2, color2) is broken, reproducer/fix:

#include "color2.h"

color2 current_fmod(color2 a, color2 b)
{
    return color2(fmod(a.r, a.r), // broken, fmod(x,x) will always be 0
                  fmod(b.a, b.a));
}

color2 fixed_fmod(color2 a, color2 b)
{
    return color2(fmod(a.r, b.r),
                  fmod(a.a, b.a));
}
shader do_color2_fmod(
    output color value = 0,
    color2 a = {2.75, 9.99},
    color2 b = {.5,3.2})
{
    color2 c = fmod(a, b); // incorrect
    //color2 c = current_fmod(a,b); // incorrect
    //color2 c = fixed_fmod(a,b); // correct
    value = color(c.r, c.a, 0.0);
}

[AlexMWells]

color4 / int is broken, reproducer/fix:

#include "color4.h"

color4 current__operator__div__(color4 a, int b)
{
    float b_inv = 1/b; // nothing promoted to float, so b_inv will be 0|-0 for all b values other than 1|-1
    return a * color4(color(b_inv), b_inv);
}

color4 fixed__operator__div__(color4 a, int b)
{
    float b_inv = 1.0/b;
    return a * color4(color(b_inv), b_inv);
} 
shader do_div_color4(
    output color value = 0,
    color4 a = {color(3.5, 2, 1), 1.5},
    int b = 5)
{
    color4 c = a/b; // incorrect
//    color4 c = __operator__div__(a,b); // incorrect
//    color4 c = current__operator__div__(a,b); // incorrect
//    color4 c = fixed__operator__div__(a,b); // correct
    value = color(c.rgb.r, c.rgb.g, c.rgb.b + c.a);
}

[AlexMWells]

Noticed that dot for color4 only accepts a color as 2nd argument, ignores color4.a (which might be fine depending on expected behavior).

float dot(color4 a, color b)

However dot(color4, color4) is undefined, which again might be OK depending on expected behavior.
Should dot(color4, color4) be defined? And if so, should alpha be included in result or ignored?
Reproducer:

#include "color4.h"

shader do_color4_dot(
    output color value = 0,
    color4 a = {color(3.5, 2, 1), 1.5},
    color b = color(1,2,3))
{
    float c = dot(a,b)/20.0;
    float d = dot(a,a)/20.0;
    value = color(c + d);
}

will fail to compile:

TestColor4Dot.osl:9: error: No matching function call to 'dot (struct color4, struct color4)'
  Candidates are:
    /nfs/pdx/home/smonteir/OSLMain-CI-Issues/OpenShadingLanguage/dist/share/OSL/shaders/color4.h:186	float dot (struct color4, color)
    /nfs/pdx/home/smonteir/OSLMain-CI-Issues/OpenShadingLanguage/dist/share/OSL/shaders/stdosl.h:190	float dot (vector, vector)

[AlexMWells]

@lgritz I Finished reviewing /src/shaders/*.h files, with the correctness issues noted above in the comments.

I guess matrix33.h could use a comment in it explaining is usefulness, which I assume is the function matrix44To33 which introduces compile time constant 0's and 1 into an underlying 4x4 matrix which hopefully will allow the optimizer to remove portions of subsequent matrix operations whose results would always be 0 or idenity.

[lgritz]

Apparently, the reason MaterialX works is that they have vendored these headers on their end and mostly fixed the bugs long ago.

That makes this make more sense to me. Probably these have gone undiscovered for so long because using 2- and 4-vectors is not idiomatic OSL, and so there is essentially nobody depending on them outside of their use in MaterialX.

But I'll fix here and submit a PR right away.

[fpsunflower]

If MaterialX codegen already has their own copy, maybe we should deprecate these?

[fpsunflower]

Looking at the MaterialX copy -- I see the same bug ...

https://github.com/AcademySoftwareFoundation/MaterialX/blob/main/libraries/stdlib/genosl/include/vector2.h#L243C1-L247C2

[HardCoreCodin]

Yes, the original issue I reported, is also in need of fixing in MaterialX's vendored header.
The div overloads for vecotr2, vector4 and color4 have already been fixed there a while ago though.
And color2 is not a thing in MaterialX, they don't even have that header in their repo.

As for dot between a color and color4, I'd say it is ok for it to fail to compile, as dot product should not be well defined between arguments of different dimensions.

[lgritz]

I think dot shouldn't be defined for any color types.