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

In lighting, always normalize zero to (0, 0, 1) #14231

Merged
merged 3 commits into from
Feb 28, 2021

Conversation

unknownbrackets
Copy link
Collaborator

Fixes #14167. We've seen several cases of this now, so I think it's better to just assume it's the case for all normalize usage.

-[Unknown]

@unknownbrackets unknownbrackets added this to the v1.12.0 milestone Feb 28, 2021
@hrydgard
Copy link
Owner

I wonder if that SSE4 path is really worth it. But probably doesn't matter much.

@hrydgard hrydgard merged commit 91c0ef2 into hrydgard:master Feb 28, 2021
@hrydgard hrydgard added the GE emulation Backend-independent GPU issues label Feb 28, 2021
@unknownbrackets
Copy link
Collaborator Author

Well, it was already there from #11425, I just figured might as well use it more places since it's there anyway.

-[Unknown]

@unknownbrackets unknownbrackets deleted the lighting branch February 28, 2021 15:19
@hrydgard
Copy link
Owner

Yeah, I know, just wondering out loud.

@@ -1056,7 +1062,7 @@ bool GenerateVertexShader(const VShaderID &id, char *buffer, const ShaderLanguag
break;
case GE_PROJMAP_NORMALIZED_NORMAL: // Use normalized transformed normal as source
if (hasNormal)
temp_tc = flipNormal ? "vec4(normalize(-normal), 1.0)" : "vec4(normalize(normal), 1.0)";
temp_tc = StringFromFormat("length(%snormal) == 0.0 ? vec4(0.0, 0.0, 1.0, 1.0) : vec4(normalize(%snormal), 1.0)", flipNormal ? "-" : "");
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@unknownbrackets I think the bug is here. Two %s but one parameter.

You don't really need the sign in the length check anyway...

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oops, I don't think I intended to have it twice. Bah.

-[Unknown]

Copy link
Owner

@hrydgard hrydgard Mar 5, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's kinda ridiculous that this compiles. (I mean, I know why, but ... )

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Makes me miss D where this wouldn't. I'm sure rust has something similar.

-[Unknown]

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Indeed, this would break with Rust formatting macros.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
GE emulation Backend-independent GPU issues
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Android] Pro Yakyu Spirits 2010 (NPJH50234): Rendering errors with hardware transform off
2 participants