Fixed the "Undeclared identifier: uv0" OpenGL shader compile error that appears in NBA2K11.

[skidau]

Fixed the "Undeclared identifier: uv0" OpenGL shader compile error that appears in NBA2K11.

[neobrain]

Do we have any idea how come the game is using different values for BP's and XF's numtexgen in the first place, even though (IIRC) GX shouldn't be able to even create such a configuration?

[skidau]

Neobrain, no idea - it might be a FIFO desync situation. Would you agree that the Dolphin code should use the same variable for the declaration generation?

[neobrain]

BP's numtexgen should be used in PixelShaderGen while VertexShaderGen should use XF's numtexgen (miming hardware behavior, which uses BP for pixel processing and XF for vertex processing).

So basically this change is good from that perspective, since it removes XF usage from PixelShaderGen. However, it might hide unexpected bugs by making invalid configurations with BP.numtexgen!=XF.numtexgen work, so I'd say there should be some ERROR_LOG in place somewhere if XF.numtexgen != BP.numtexgen. Putting such a check in one of the shader generators will not work because our shader UIDs might prevent properly catching this, so maybe VertexManagerBase::Flush might be a good place.

[JMC47]

I've seen this bug come up in other games, but I can't remember which :(

[degasus]

Does this also fix this issue on OGL? I guess there will be linking issues now.

[skidau]

@degasus the "bug" is only in the OGL path. The D3D backend already uses the variable. What is a "linking" issue?

[magcius]

@neobrain don't compiled display lists allow you to poke this stuff? Could it be a bug in their tooling?

[neobrain]

@magcius I'm not sure to what extend software developers crafted their own display lists on the GC/Wii. AFAIK, usually display lists are created by calling some Begin() call and then invoking GX calls regularly to record them.

[magcius]

I know Nintendo's own software has precompiled display lists, and I know they were regularly pushing this technique in their third-party support channels, so that might be it. This also makes sure to match what the hardware does, from my understanding.

But if you think it's still our bug with unsynced FIFOs, yeah, let's add a warning.

[Sonicadvance1]

Shouldn't this just be merged on the basis that it is more correct? Is there really a need to fight about this other nonsense here?

[neobrain]

@Sonicadvance1 I'm not seeing how anyone's fighting in this PR. Don't make drama out to be where none actually is.

@skidau Please just add a warning message because it's the right thing to do and logically fits into this PR, and because it literally takes half a minute to do so when one has the branch already checked out and a PR created. This PR in its current state effectively hides scenarios in which we hit undefined behavior.

[skidau]

Have added an error log as suggested. I noticed that there already was an assert for this condition in VertexShaderGen GenerateVertexShader.

[degasus]

@skidau For OGL, the shader API must match, so every texcoord defined as output in the vertex shader must be read in the pixel shader. But I think this could be "fixed" with a full interface (always alloc all texdims, also if they are unused. This may match the HW quite well)

btw, xfmem.numTexGen.numTexGens wasn't in the shader uid, was it?

[neobrain]

LGTM (thanks!)

[neobrain]

@degasus the code implicitly assumed that bpmem.numTexGens is equal to xfmem.numTexGens, indeed. That's why I figured an error message in PixelShaderGen might have been useless :)