Possible bug in ShaderMaterial

I was looking at the shaderMaterial.ts source code of r4.0.3 and saw this function:

    private _checkCache(mesh?: AbstractMesh, useInstances?: boolean): boolean {
        if (!mesh) {
            return true;
        }

        if (this._effect && (this._effect.defines.indexOf("#define INSTANCES") !== -1) !== useInstances) {
            return false;
        }

        return false;
    }

I think it’s the same thing than (except if I miss some side-effects):

    private _checkCache(mesh?: AbstractMesh, useInstances?: boolean): boolean {
        return !mesh;
    }

So, useInstances is of no use: is it the intended behaviour?

1 Like

Well useInstances is used in the test right?
if there is an effect and if the presence of the define is different from useInstances value?

The return value of the function depends only on mesh. It could be rewritten;

    private _checkCache(mesh?: AbstractMesh, useInstances?: boolean): boolean {
        if (!mesh) {
            return true;
        }

        return false;
    }

because we have return false both in the if (this._effect ...) and as the last statement of the function.

So, in effect, everything depends only on mesh value.

Maybe one of the latest return false should have been a return true instead?

Haha yeah you are totally right!

Well I will fix it now unless you want to do a PR :slight_smile:

No, I don’t think a PR is needed for such a small one!

Also, I would not know how to correct it. Is it this:

private _checkCache(mesh?: AbstractMesh, useInstances?: boolean): boolean {
    return !mesh;
}

or:

private _checkCache(mesh?: AbstractMesh, useInstances?: boolean): boolean {
    if (!mesh) {
        return true;
    }

    if (this._effect && (this._effect.defines.indexOf("#define INSTANCES") !== -1) !== useInstances) {
        return false;
    }

    return true;
}

or:

private _checkCache(mesh?: AbstractMesh, useInstances?: boolean): boolean {
    if (!mesh) {
        return true;
    }

    if (this._effect && (this._effect.defines.indexOf("#define INSTANCES") !== -1) !== useInstances) {
        return true;
    }

    return false;
}

It seems awkward to me that it is #1, because it means the second if has no purpose (and so why has it been written in the first place?) and that the cache does in fact has no purpose (because I assume that mesh is really never null / undefined in real world?).

So maybe the right one is #2, but it’s a wild guess… (I don’t think it is #3, we enter the if if there is a discrepancy between the effect and the variable useInstances regarding instancing usage, so I think in this case the cache is not valid anymore).

yes if has no purpose. It is definitely an old bug (even though it is not impactful)

I will do the change as this is actually a miss. We do not use this cache anymore

Thanks a lot!

1 Like