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

Sora no kiseki SC / HD light problem #8403

Closed
daniel229 opened this issue Jan 8, 2016 · 5 comments · Fixed by #11574
Closed

Sora no kiseki SC / HD light problem #8403

daniel229 opened this issue Jan 8, 2016 · 5 comments · Fixed by #11574
Milestone

Comments

@daniel229
Copy link
Collaborator

HD version capture from https://youtu.be/n94plorFBXE?t=14m40s
schd1

PPSSPP OpenGL just black behind the box
schd2

PPSSPP software redering looks fine.
schd3

Non-HD version
PSP Looks darker
psp1

PPSSPP OpenGL less darkness
psp2

software rendering looks like the HD version
psp3

@daniel229
Copy link
Collaborator Author

GE debugger ,don't know if this one,next prim still not draw the boxes.

HD version
hd
https://gist.github.com/daniel229/c24d3f143c47bb08c055

Non-HD
non-hd
https://gist.github.com/daniel229/26d2c8c8bcdbe13503c5

@unknownbrackets
Copy link
Collaborator

I think that's the right place. If you go there, and then double click one of the lights (like light 3), does it look any closer to the PSP?

I wonder if this could be influenced at all by the normals. Maybe it's just a precision thing.

-[Unknown]

@daniel229
Copy link
Collaborator Author

Yes,double click lights3,it looks closer to the PSP.

@unknownbrackets
Copy link
Collaborator

unknownbrackets commented Nov 19, 2018

Here's a dump (just hurried to the same place, since it's early): NPUH10191_#8403_trails_lighting.zip

Should look like this per a PSP (as noted above, just showing this dump's frame):
npuh10191_ 8403_trails_lighting

Though, of course, it looks much better the way PPSSPP displays it. I'd ideally prefer it looked like the HD version.

Draw uses texture 0x04100000, and point light as above. It also uses color doubling.

To make software renderer use the PSP behavior (correct?) I can make this change:

-		if (gstate.isUsingSpecularLight(light)) {
+		if (gstate.isUsingSpecularLight(light) && diffuse_factor > 0.f) {

That is, ignore specular if diffuse factor was <= 0.

To make the Vulkan/backend renderer use the HD/software behavior (better looking - I think the PC version looks this way too):

				WRITE(p, "  dot%i = dot(normalize(toLight + vec3(0.0, 0.0, 1.0)), worldnormal);\n", i);
+				WRITE(p, "  if (dot%i <= 0.0 && light.matspecular.a == 0.0) {\n", i);
+				WRITE(p, "    dot%i = 1.0;\n", i, i);
+				WRITE(p, "  } else {\n");
+				WRITE(p, "    dot%i = pow(dot%i, light.matspecular.a);\n", i, i);
+				WRITE(p, "  }\n");
+				WRITE(p, "  if (dot%i > 0.0)\n", i);
+				WRITE(p, "    lightSum1 += light.specular[%i] * %s * (dot%i %s);\n", i, specularStr, i, timesLightScale);
-				WRITE(p, "    lightSum1 += light.specular[%i] * %s * (pow(dot%i, light.matspecular.a) %s);\n", i, specularStr, i, timesLightScale);

Note the logic - similar to #2424. Importantly, this includes <= 0.0 not just == 0.0, which might be interesting to try in the powered diffuse case (not sure...) Maybe we should just simplify the check to light.matspecular.a == 0.0 and ignore dot%i.

Anyway, I wonder if this difference affects lighting in other cases, such as other lighting bugs we're aware of...

-[Unknown]

@hrydgard
Copy link
Owner

hrydgard commented Nov 19, 2018

I think if we do this, the if (gstate.isUsingSpecularLight(light) && diffuse_factor > 0.f) { should check the diffuse factor from before it gets changed by the powered diffuse - that is, just use the dot product directly. It seems very plausible that the PSP would use that check to skip the specular processing if possible.

unknownbrackets added a commit to unknownbrackets/ppsspp that referenced this issue Nov 22, 2018
This is correct per hardware tests, see hrydgard#8403.  Note that the PS3 emulator
running PSP HD remasters does not correctly handle this, and applies
specular for negative diffuse factor.
@unknownbrackets unknownbrackets added this to the v1.8.0 milestone Nov 22, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants