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

Round primary color in swrast #3097

Merged
merged 3 commits into from Dec 12, 2017

Conversation

Projects
None yet
8 participants
@ds84182
Copy link
Contributor

commented Nov 9, 2017

Also includes a fix for logic ops not being enabled in OpenGL (depending on circumstances).

Also ports the hack from the hardware renderer to software rasterizer where null texture addresses are ignored. (Otherwise the software rasterizer will crash doing a null deref).


This change is Reviewable

@Subv

This comment has been minimized.

Copy link
Member

commented Nov 9, 2017

Could you please include a hwtest for this?
Also, in the future, please don't include unrelated changes in the same PR, they make it much harder to review.

@fincs

This comment has been minimized.

Copy link
Contributor

commented Nov 9, 2017

The test case from last night which resulted in this PR: https://feos.mtheall.com/citra-logicopbug.zip (originally intended just for the logic op bug, ended up discovering the rounding bug too)

@FearlessTobi

This comment has been minimized.

Copy link
Contributor

commented Nov 9, 2017

#2440 is fixed with this (by skipping the null textures).

@@ -293,16 +293,16 @@ static void ProcessTriangleInternal(const Vertex& v0, const Vertex& v1, const Ve
};

Math::Vec4<u8> primary_color{
(u8)(
(u8)round(

This comment has been minimized.

Copy link
@B3n30

B3n30 Nov 15, 2017

Contributor

is there a reason static_cast() isn't used?

@@ -331,6 +331,7 @@ static void ProcessTriangleInternal(const Vertex& v0, const Vertex& v1, const Ve
// Only unit 0 respects the texturing type (according to 3DBrew)
// TODO: Refactor so cubemaps and shadowmaps can be handled
PAddr texture_address = texture.config.GetPhysicalAddress();
if (texture_address == 0) continue;

This comment has been minimized.

Copy link
@B3n30

B3n30 Nov 15, 2017

Contributor

Should there be a log if that case happens? Maybe debug

@B3n30

This comment has been minimized.

Copy link
Contributor

commented Nov 28, 2017

What's the status of that PR? It needs clang-format fixing. Also my comments weren't addressed yet.

@B3n30 B3n30 added the pr:needs-work label Nov 28, 2017

@ds84182

This comment has been minimized.

Copy link
Contributor Author

commented Nov 29, 2017

I'll try to get back to this today but I discovered that this fixes Pokemon X&Y because textures with a null address get skipped with one of my changes. I did some GPU research and I concluded that the color output from a null address texture depends on the previous texture used and it varies if the types of the texture are different. The color is deterministic since it depends on what was drawn before it, but I'd conclude that this is a bug in Pokemon X & Y. The only reason why it wasn't fixed was because the developers could not see what was happening with the null texture, since they always draw the male character's body before the two cheeks. In my tests I get slightly non-deterministic results when opening the home menu while using the null address. If a game depends on this weird hardware behavior for correct rendering we'd be in trouble though.

Anyways the correct thing to do is to warn in the log when a null texture is used and emit a transparent black. I've already argued that using the null texture is a bug in the game and is undefined (could break if Nintendo ever decides to do a new GPU revision, although I don't think they will), and we should provide a sensible default.

I'll remove the null address check out of software rasterizer for now, and I'll address the comments.

@ds84182 ds84182 force-pushed the ds84182:round-primary-color-swrast branch from 40aa9a6 to 350082a Nov 29, 2017

Maintain the PICA's 8 bits of color precision when using the interpol…
…ated primary color

This matches the software renderer by using round.
The actual hardware rounds the results up instead of flooring.
@Subv

Subv approved these changes Dec 10, 2017

@yuriks yuriks merged commit ae7240a into citra-emu:master Dec 12, 2017

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@citra-emu citra-emu deleted a comment from MakerMayker Jun 20, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.