Fix for type comparison for glsl programs

[kwokcb]

Issue

Fixes #1608 where type comparison will fail and hence uniforms are not discoverable for rendering.

Fix

The pointer comparison can fail so use name type comparison instead.

[jstone-lucasfilm]

@kwokcb Do you have a sense of why the original pointer comparisons are no longer robust? Since we use this pattern throughout the shader generation system, and not just in this one function, I'd recommend that we address the source of the pointer comparison failures, rather than working around it in just one location.

[kwokcb]

@jstone-lucasfilm. I'm not sure what may be the cause of this. It may perhaps never be safe to perform pointer comparisons and just worked since all code is called within the same module -- I found this out by using GLSLProgram outside the module via Python.

[jstone-lucasfilm]

@kwokcb That sounds like a good theory of the case, and if these pointer comparisons are indeed unsafe across modules, then we should refactor this PR as a proposal to replace the pointer comparisons globally.

CC'ing @niklasharrysson for his thoughts!

[niklasharrysson]

@kwokcb @jstone-lucasfilm It is very strange if these pointer compares have become unsafe. The TypeDesc class has been there since the early days of shader generation many many years ago. And these type compares are being done all over the code base, in many different modules.

It's also hard to see why it would fail here. Since when you get to the points where this mapTypeToOpenGLType is being called a shader has already been generated successfully, and hundreds of these type compares must have been performed already.

Bernard, could you get some more information out from a debugger? If you look at the TypeDesc pointers and data pointed to, and compare to what is sent in, how does that look?

We can take this one offline and discuss more.

[kwokcb]

Hi @niklasharrysson
Sure we can take this offline. I'll leave this debugging information here.
The contents of the type I assume are okay since things render, just the addresses are "off".

This is what I get in general:

Failed to get mapped type: float address: 000002190A37E170vs Type:: 000002190A3CBF80
Failed to get mapped type: matrix44 address: 000002190A37F3F0vs Type:: 000002190A3CBB00
Failed to get mapped type: filename address: 000002190A37DC70vs Type:: 000002190A3CC600
Failed to get mapped type: integer address: 000002190A37E570vs Type:: 000002190A3CBA00
Failed to get mapped type: integer address: 000002190A37E570vs Type:: 000002190A3CBA00
Failed to get mapped type: filename address: 000002190A37DC70vs Type:: 000002190A3CC600
Failed to get mapped type: boolean address: 000002190A37E670vs Type:: 000002190A3CB600
Failed to get mapped type: displacementshader address: 000002190A37DEF0vs Type:: 000002190A3CAE80
Failed to get mapped type: matrix44 address: 000002190A37F3F0vs Type:: 000002190A3CBB00
Failed to get mapped type: matrix44 address: 000002190A37F3F0vs Type:: 000002190A3CBB00
Failed to get mapped type: matrix44 address: 000002190A37F3F0vs Type:: 000002190A3CBB00
Failed to get mapped type: matrix44 address: 000002190A37F3F0vs Type:: 000002190A3CBB00

filename, string, and displacementshader are not supported got GLSL program mapping, but all other types seems to fail for pointer comparison.

Again this is from Python so maybe there's some address shuffling going on?
The only thing I can think of is whether this is related to the changes made for Python package publishing to PyPi ?

---

This is the debug code:

int GlslProgram::mapTypeToOpenGLType(const TypeDesc* type)
{
    if (type == Type::INTEGER)
        return GL_INT;
    else if (type == Type::BOOLEAN)
        return GL_BOOL;
    else if (type == Type::FLOAT)
        return GL_FLOAT;
    else if (type->isFloat2())
        return GL_FLOAT_VEC2;
    else if (type->isFloat3())
        return GL_FLOAT_VEC3;
    else if (type->isFloat4())
        return GL_FLOAT_VEC4;
    else if (type == Type::MATRIX33)
    {
        std::cout << "Pass to get mapped type:" << type << "vs matrix33: " << Type::MATRIX33 << std::endl;
        return GL_FLOAT_MAT3;
    }
    else if (type == Type::MATRIX44)
    {
        std::cout << "Pass to get mapped type:" << type << "vs matrix44: " << Type::MATRIX44 << std::endl;
        return GL_FLOAT_MAT4;
    }
    else if (type == Type::FILENAME)
    {
        // A "filename" is not indicative of type, so just return a 2d sampler.
        return GL_SAMPLER_2D;
    }
    std::cout << "Failed to get mapped type: " + type->getName() << " address: " << type << "vs Type:: ";
    if (type->getName() == Type::INTEGER->getName())
        std::cout << Type::INTEGER << std::endl;
    else if (type->getName() == Type::BOOLEAN->getName())
        std::cout << Type::BOOLEAN << std::endl;
    else if (type->getName() == Type::FLOAT->getName())
        std::cout << Type::FLOAT << std::endl;
    else if (type->getName() == Type::MATRIX33->getName())
        std::cout << Type::MATRIX33 << std::endl;
    else if (type->getName() == Type::MATRIX44->getName())
    {
        std::cout << Type::MATRIX33 << std::endl;
    }
    else if (type->getName() == Type::FILENAME->getName())
    {
        std::cout << Type::FILENAME << std::endl;
    }
    else if (type->getName() == Type::STRING->getName())
    {
        std::cout << Type::STRING << std::endl;
    }
    else if (type->getName() == Type::DISPLACEMENTSHADER->getName())
    {
        std::cout << Type::DISPLACEMENTSHADER << std::endl;
    }
    return GlslProgram::Input::INVALID_OPENGL_TYPE;
}

I'm also wondering is it actually safe to use address comparison if your using shared libraries ?

[jstone-lucasfilm]

I'll close out this specific pull request, since the fix will likely be broader than the changes described here, and we'll need additional research into the cause.

We can continue the discussion in #1608, which is a good summary of the issue that needs to be addressed.