Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

PixelShaderGen: Manually wrap negative indirect texture coordinates #3509

Merged
merged 1 commit into from
Jan 16, 2016
Merged

PixelShaderGen: Manually wrap negative indirect texture coordinates #3509

merged 1 commit into from
Jan 16, 2016

Conversation

stenzek
Copy link
Contributor

@stenzek stenzek commented Jan 13, 2016

(x % y) is not defined in GLSL when sign(x) != sign(y).
This also has the added benefit of behaving the same as sampler wrapping modes, in regards to negative inputs.

This was causing raindrops to render incorrectly in F-Zero GX with OGL on windows/intel and mesa/llvmpipe, as well as D3D, due to the previous implementation invoking UB when the coordinates were negative. (see GLSL spec re % operator, D3D seems to take the sign from the LHS)

Marked WIP because we're not sure if this actually matches hardware behavior in regards to how negative coordinates are handled.

@delroth
Copy link
Member

delroth commented Jan 13, 2016

@JMC47 is this a candidate for 5.0?

@degasus
Copy link
Member

degasus commented Jan 13, 2016

It fixes an undefined behavior bug, but it may lead to unknown regressions ...

default:
return 0;
}

return (coord < 0) ? (size - (std::abs(coord) % size)) : (std::abs(coord) % size);

This comment was marked as off-topic.

This comment was marked as off-topic.

This comment was marked as off-topic.

This comment was marked as off-topic.

This comment was marked as off-topic.

This comment was marked as off-topic.

This comment was marked as off-topic.

This comment was marked as off-topic.

@JMC47
Copy link
Contributor

JMC47 commented Jan 13, 2016

@delroth imo yes, as it brings D3D/OpenGL even on a few graphical bugs. It also makes it a lot easier for me to cleanup the wiki on those two bugs.

@CrossVR CrossVR added the WIP / do not merge Work in progress (do not merge) label Jan 13, 2016
@delroth delroth added this to the Dolphin Release 5.0 milestone Jan 14, 2016
@BhaaLseN
Copy link
Member

Those two diffs look good to me tho. Did you try writing a HW-Test yet?

@degasus
Copy link
Member

degasus commented Jan 15, 2016

As all of our guesses which returns negative values didn't fix those two fifologs, I think this is the only possible way for the hardware. LGTM when the coding style issues are fixed.

case ITW_16:
return (coord % (16 << 7));
return (coord & ((16 << 7)) - 1);

This comment was marked as off-topic.

…ates

(x % y) is not defined in GLSL when sign(x) != sign(y).
This also has the added benefit of behaving the same as sampler wrapping modes, in regards to negative inputs.
@dolphin-emu-bot
Copy link
Contributor

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

  • f-zero-rain on ogl-lin-mesa: diff
  • ss-map on ogl-lin-mesa: diff
  • f-zero-rain on sw-lin-mesa: diff
  • ss-map on sw-lin-mesa: diff

automated-fifoci-reporter

@JMC47
Copy link
Contributor

JMC47 commented Jan 16, 2016

This probably isn't important to merge due to the bug being graphical, but at the same time it'd make F-Zero GX's updated wiki page a lot cleaner if we were able to outright remove the bug instead of noting it for another release cycle. I know that's stupid; but in TFN, that was one of the issues we advertised as fixing

@delroth delroth changed the title [WIP] PixelShaderGen: Manually wrap negative indirect texture coordinates PixelShaderGen: Manually wrap negative indirect texture coordinates Jan 16, 2016
@delroth delroth removed the WIP / do not merge Work in progress (do not merge) label Jan 16, 2016
delroth added a commit that referenced this pull request Jan 16, 2016
PixelShaderGen: Manually wrap negative indirect texture coordinates
@delroth delroth merged commit ea2765e into dolphin-emu:master Jan 16, 2016
@stenzek stenzek deleted the negative-indirect branch February 5, 2016 09:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
9 participants