Convert NaN to 1 when generating texture coordinates

[Pokechu22]

This fixes eyelids in Shadow the Hedgehog during cutscenes (https://bugs.dolphin-emu.org/issues/11458).

The eyelids (objects 36 and 37) draw on the same data, but object 36 uses texture coord 0 for its texture coordinate while object 37 uses the normal for its texture coordinate. The normal coordinates come from 00793400 (80793400 virtual), and some of them are NaN (specifically encoded as ffc00000). This specifically affects the center of the white part of the eye and the top left part of it, but because of how NaN behaves, the result of trying to interpolate anything with those vertices is NaN.

I used the hardware fifoplayer with the fifolog from that issue, and things rendered correctly, so this isn't a case of bad emulation creating NaNs:

My fix is to convert NaN to 1 before doing any math with it, which seems to give correct results. (Remember that if x is NaN, x != x is true.)

[Pokechu22]

In testing this, I found that using x != x doesn't work with ubershaders on Vulkan (with my Nvidia GPU), though isnan worked. Additionally, neither of them currently work on D3D11 or D3D12 with ubershaders, with "warning X3577: value cannot be NaN, isnan() may not be necessary. /Gis may force isnan() to be performed" being logged when isnan is used. This happens even if D3DCOMPILE_IEEE_STRICTNESS (which is what /Gis controls according to this page) is added here.

So, currently, this doesn't work when using ubershaders with either D3D11 or D3D12, but all other backends-ubershader combinations work fine.

[AdmiralCurtiss]

I'm not convinced this is entirely correct. Compare the upper-left part of the affected eyelid, where it meets the red part of the eye. On console there's a noticeable bit of black before the reflection starts, but in the current replace-NaN-with-1 it starts immediately. The lower-right part is also more point-y on console than with this PR.

Definitely an improvement over the current situation, mind, but I suspect there's more going on here in hardware...

[Pokechu22]

The other eyelid also has some oddness going on with it:

Before this change:

After this change:

Both of these are OGL with specialized shaders. This eyelid isn't visible normally, and there isn't any way to move the camera with the hardware fifoplayer, so it may just be messed up like this normally and you can't see it.

[iwubcode]

So, currently, this doesn't work when using ubershaders with either D3D11 or D3D12, but all other backends-ubershader combinations work fine.

Hmm, surprised your flag didn't work (did you purge your shader cache?).

Alternatively to having this in the vertex shader, could we just correct this when processing the texture coordinates before we send it down. It concerns me a bit to have the backends be broken for some apis (even if this is a niche issue).

but object 36 uses texture coord 0 for its texture coordinate while object 37 uses the normal for its texture coordinate

Using a normal for a tex coord seems a bit odd too. Is there a different issue at play here? Kinda makes me think of some game bug that just so happens to work right on console (that we've seen many times this year)

[Pokechu22]

Hmm, surprised your flag didn't work (did you purge your shader cache?).

Yep, I deleted the folder each time. Possibly I did testing wrong still, but I'm pretty sure it wasn't a shader cache thing.

Using a normal for a tex coord seems a bit odd too.

XF lets you use a lot of different inputs for texture coordinates (so-called texgens), including positions and normals. (#9876 and its write-up is an example of a good use of positions for texture coordinates.) That said, I don't know why they're doing it here, as I don't think they use the normals as actual normals, so they could just have specified two texture coordinates instead.

Alternatively to having this in the vertex shader, could we just correct this when processing the texture coordinates before we send it down.

The problem is that things other than texture coordinates can be converted to texture coordinates, so NaN would also need to be converted for positions and normals in all places they can be used (instead of just for when they're changed to texture coordinates, since that happens in the shader). It could be done, but it would require more hardware testing to determine exactly what should happen in all cases.

[dolphin-emu-bot]

FifoCI detected that this change impacts graphical rendering. Here are the behavior differences detected by the system:

• shadow-eyes on ogl-lin-mesa: diff
• shadow-eyes on sw-lin-mesa: diff
• shadow-eyes on ogl-lin-radeon: diff
• shadow-eyes on uberogl-lin-radeon: diff

_{^{automated-fifoci-reporter}}

[JMC47]

I tested and approve of this change. Maybe needs to be checked on Android to make sure it doesn't break their shaders?

[JosJuice]

Seems fine on my Adreno phone. (I don't have Shadow the Hedgehog, so I just tested some random game to check that nothing had broken.)

[Rumi-Larry]

Is a hardware test possible in this case?

[nbohr1more]

Is there a reason to expect that all NaN's of this type can be approximated as 1? For example, a divide by zero NaN should be a clamped max value, no?

[Pokechu22]

Is a hardware test possible in this case?

Sure, it's just drawing a triangle with the appropriate data (NaN at one or more vertices) and seeing what happens.

Is there a reason to expect that all NaN's of this type can be approximated as 1? For example, a divide by zero NaN should be a clamped max value, no?

Division by 0 (other than 0/0 and the equivalents for negative zero) would give +Inf or -Inf, not NaN. isnan only looks for NaN, while isinf would check for +Inf and -Inf.

There's no guarantee that all NaNs will become 1. All that I know for sure is that NaN encoded as ffc00000 appears to become 1 based on the hardware fifoplayer.

[Rumi-Larry]

Is a hardware test possible in this case?

Sure, it's just drawing a triangle with the appropriate data (NaN at one or more vertices) and seeing what happens.

Ahh, I didn't know the 'Hardware FIFO player' was the same thing.

[Pokechu22]

The hardware FIFO player is a different thing (it's this repo, which takes a bit of work to get running). It lets you run fifologs on hardware (in the screenshots above, I used test2-obj37.zip from issue 11458). That's how I've been testing things so far.

It would be possible to test further by writing something to intentionally draw a triangle like that and see what happens, but I haven't done that yet.

[Rumi-Larry]

The hardware FIFO player is a different thing (it's this repo, which takes a bit of work to get running). It lets you run fifologs on hardware (in the screenshots above, I used test2-obj37.zip from issue 11458). That's how I've been testing things so far.

It would be possible to test further by writing something to intentionally draw a triangle like that and see what happens, but I haven't done that yet.

Have the results ever differed?

[Pokechu22]

Have the results ever differed?

I don't understand what you mean by that. If you mean the results of the hardware fifoplayer differing when run on hardware versus using Dolphin's built-in fifoplayer, yes, sometimes the results do differ (you can get vertex explosions with the hardware fifoplayer in some cases; I haven't investigated why). But that can be resolved by using the hardware fifoplayer in Dolphin, which generally gives the same results as on console (other than the actual graphical issues being investigated). If you mean testing by using the fifoplayer giving different results compared to writing a smaller, self-contained test, that can also sometimes happen (the debug cubes in Super Mario Sunshine are an example of this; they don't show up with the fifoplayer but writing a simpler test that does what they do does result in things showing up, though it is complicated).

[iwubcode]

@Pokechu22 - is this ready to go? I thought you said isnan couldn't be used on D3D without a warning?

[Pokechu22]

The D3D isnan thing still does apply, and I don't know how to fix it. However, D3D11 and D3D12 render correctly when using specialized shaders despite the warning; they only don't render correctly with ubershaders.

[iwubcode]

The D3D isnan thing still does apply, and I don't know how to fix it. However, D3D11 and D3D12 render correctly when using specialized shaders despite the warning; they only don't render correctly with ubershaders.

Dang, not sure how I feel about merging this personally. Not because this in itself is very bad but because if we start having little differences in how backends perform or render, then the whole point of VideoCommon goes away.

What do you mean when you say "don't render correctly"? Does this effect games outside of Shadow the Hedgehog? (I'd assume so since it's always writing this code to be parsed in the shader)

[Pokechu22]

What do you mean when you say "don't render correctly"? Does this effect games outside of Shadow the Hedgehog? (I'd assume so since it's always writing this code to be parsed in the shader)

Nope, it's just that on D3D11/D3D12 with ubershaders, it acts as if the isnan call always returns false, so the issue in Shadow just isn't fixed. It shouldn't introduce any new issues into games that don't use NaN.

[iwubcode]

Code LGTM. Untested

[iwubcode]

@Pokechu22 - can we open an issue for ubershaders (assuming this gets merged without a fix to resolve this)?

[Pokechu22]

Yeah, I'll open an issue for it when it gets merged.

[Pokechu22]

https://bugs.dolphin-emu.org/issues/12609

[Pokechu22]

I did a basic test where I hex-edited the fifolog to have various values by replacing ffc00000 with other values. The results with this PR and on console can be seen below. (The modified files can be found in shadoweyes.tar.gz.)

	Dolphin 5.0-14812	Console
NaN
-1
-.5
0
.5
1

I also tried a few where I set the y and z components set to 0 and only changed x:

	Dolphin 5.0-14812	Console
NaN
-1
1

At least to me, the color seems to be correct, but the positioning is indeed wrong. Using -1 instead of +1 gives closer results, but the size is too big. The behavior with a normal of (NaN, 0, 0) is particularly odd, and rules out something like the value being normalized to a unit vector as it does not match (-1, 0, 0).

[Pokechu22]

I did some tests where only the texture or the rasterized color were in use, to try and isolate behavior with normals from behavior with texture coordinates generated from those normals (the only tev stage in use has the output color set to the texture (in b) multiplied by the rasterized color (in c)). I did this by either changing b or c to 1: for tex only, I wrote 61c008f8cf to 00007837, and for ras only, I wrote 61c008fcaf. These are all based on the fifologs from the previous comment.

Note that these are screenshots using a (low-quality) capture card, with corresponding analog video issues (including overall brightness changing over time).

	Texture only	Both	Rasterized color only
NaN
-1
-.5
0
.5
1

And with the x and y components set to 0:

	Texture only	Both	Rasterized color only
NaN
-1
1

Conclusion: the behavior with normals seems to be as if the NaN value is -1, but the behavior for texture coordinates seems to be different from both (though it's closer to -1 than to 1). This was still tested with a texture coordinate generated from the normals, which could potentially behave differently from a texture coordinate specified directly.