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.

Sign up for GitHub

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

Correct shade mapping when light pos is all zeros #11567

Merged
merged 3 commits into from
Nov 18, 2018
Merged

Correct shade mapping when light pos is all zeros #11567

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
6600896
GPU: Fix shade mapping in soft transform.
unknownbrackets Nov 18, 2018
986371c
SoftGPU: Normalize 0,0,0 as 0,0,1 for shade map.
unknownbrackets Nov 18, 2018
0c5c776
GPU: Correct shade mapping with 0,0,0 light pos.
unknownbrackets Nov 18, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
19 changes: 17 additions & 2 deletions GPU/Common/SoftwareTransformCommon.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -391,8 +391,23 @@ void SoftwareTransform(
case GE_TEXMAP_ENVIRONMENT_MAP:
// Shade mapping - use two light sources to generate U and V.
{
Vec3f lightpos0 = Vec3f(&lighter.lpos[gstate.getUVLS0() * 3]).Normalized();
Vec3f lightpos1 = Vec3f(&lighter.lpos[gstate.getUVLS1() * 3]).Normalized();
auto getLPosFloat = [&](int l, int i) {
return getFloat24(gstate.lpos[l * 3 + i]);
};
auto getLPos = [&](int l) {
return Vec3f(getLPosFloat(l, 0), getLPosFloat(l, 1), getLPosFloat(l, 2));
};
auto calcShadingLPos = [&](int l) {
Vec3f pos = getLPos(l);
if (pos.Length() == 0.0f) {
Copy link
Owner

Choose a reason for hiding this comment

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

Not that it's important, but you could save a square root here by using Length2() instead (which is simply length without the square root).

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Fair point, didn't think about that.

-[Unknown]

return Vec3f(0.0f, 0.0f, 1.0f);
} else {
return pos.Normalized();
}
};
// Might not have lighting enabled, so don't use lighter.
Vec3f lightpos0 = calcShadingLPos(gstate.getUVLS0());
Vec3f lightpos1 = calcShadingLPos(gstate.getUVLS1());

uv[0] = (1.0f + Dot(lightpos0, worldnormal))/2.0f;
uv[1] = (1.0f + Dot(lightpos1, worldnormal))/2.0f;
Expand Down
6 changes: 3 additions & 3 deletions GPU/Common/TransformCommon.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -107,9 +107,9 @@ void Lighter::Light(float colorOut0[4], float colorOut1[4], const float colorIn[
Vec3f lightDir(0, 0, 0);

if (type == GE_LIGHTTYPE_DIRECTIONAL)
toLight = Vec3f(&lpos[l * 3]); // lightdir is for spotlights
toLight = Vec3Packedf(&lpos[l * 3]); // lightdir is for spotlights
else
toLight = Vec3f(&lpos[l * 3]) - pos;
toLight = Vec3Packedf(&lpos[l * 3]) - pos;

bool doSpecular = gstate.isUsingSpecularLight(l);
bool poweredDiffuse = gstate.isUsingPoweredDiffuseLight(l);
Expand Down Expand Up @@ -139,7 +139,7 @@ void Lighter::Light(float colorOut0[4], float colorOut1[4], const float colorIn[
break;
case GE_LIGHTTYPE_SPOT:
case GE_LIGHTTYPE_UNKNOWN:
lightDir = Vec3f(&ldir[l * 3]);
lightDir = Vec3Packedf(&ldir[l * 3]);
angle = Dot(toLight.Normalized(), lightDir.Normalized());
if (angle >= lcutoff[l])
lightScale = clamp(1.0f / (latt[l * 3] + latt[l * 3 + 1] * distanceToLight + latt[l * 3 + 2] * distanceToLight*distanceToLight), 0.0f, 1.0f) * powf(angle, lconv[l]);
Expand Down
6 changes: 5 additions & 1 deletion GPU/Directx9/VertexShaderGeneratorDX9.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -715,7 +715,11 @@ void GenerateVertexShaderHLSL(const VShaderID &id, char *buffer, ShaderLanguage
break;

case GE_TEXMAP_ENVIRONMENT_MAP: // Shade mapping - use dots from light sources.
WRITE(p, " Out.v_texcoord = float3(u_uvscaleoffset.xy * float2(1.0 + dot(normalize(u_lightpos%i), worldnormal), 1.0 + dot(normalize(u_lightpos%i), worldnormal)) * 0.5, 1.0);\n", ls0, ls1);
{
std::string lightFactor0 = StringFromFormat("(length(u_lightpos%i) == 0.0 ? worldnormal.z : dot(normalize(u_lightpos%i), worldnormal))", ls0, ls0);
Copy link
Owner

Choose a reason for hiding this comment

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

Related to the previous comment, I wonder if GPU compilers optimize length==0 to avoid the square root. Then again, square roots are pretty fast on gpu so maybe it doesn't matter. No idea if either way is faster than just comparing all three components to 0.0.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Hm, I can't get any even slightly consistent results between:

  • all(equal(light.pos[%i], vec3(0.0)))
  • length(light.pos[%i]) == 0.0
  • dot(light.pos[%i], vec3(1.0)) == 0.0
  • dot(light.pos[%i], light.pos[%i]) == 0.0
  • light.pos[%i].x == 0.0 && light.pos[%i].y == 0.0 && light.pos[%i].z == 0.0

If you have any preference, happy to change it. No idea which might be faster on various drivers...

-[Unknown]

Copy link
Owner

Choose a reason for hiding this comment

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

Probably all of small enough impact to not really make a performance difference...

std::string lightFactor1 = StringFromFormat("(length(u_lightpos%i) == 0.0 ? worldnormal.z : dot(normalize(u_lightpos%i), worldnormal))", ls1, ls1);
WRITE(p, " Out.v_texcoord = float3(u_uvscaleoffset.xy * float2(1.0 + %s, 1.0 + %s) * 0.5, 1.0);\n", lightFactor0.c_str(), lightFactor1.c_str());
}
break;

default:
Expand Down
6 changes: 5 additions & 1 deletion GPU/GLES/VertexShaderGeneratorGLES.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -810,7 +810,11 @@ void GenerateVertexShader(const VShaderID &id, char *buffer, uint32_t *attrMask,
break;

case GE_TEXMAP_ENVIRONMENT_MAP: // Shade mapping - use dots from light sources.
WRITE(p, " v_texcoord = vec3(u_uvscaleoffset.xy * vec2(1.0 + dot(normalize(u_lightpos%i), worldnormal), 1.0 + dot(normalize(u_lightpos%i), worldnormal)) * 0.5, 1.0);\n", ls0, ls1);
{
std::string lightFactor0 = StringFromFormat("(length(u_lightpos%i) == 0.0 ? worldnormal.z : dot(normalize(u_lightpos%i), worldnormal))", ls0, ls0);
std::string lightFactor1 = StringFromFormat("(length(u_lightpos%i) == 0.0 ? worldnormal.z : dot(normalize(u_lightpos%i), worldnormal))", ls1, ls1);
WRITE(p, " v_texcoord = vec3(u_uvscaleoffset.xy * vec2(1.0 + %s, 1.0 + %s) * 0.5, 1.0);\n", lightFactor0.c_str(), lightFactor1.c_str());
}
break;

default:
Expand Down
3 changes: 2 additions & 1 deletion GPU/Software/Lighting.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -38,7 +38,8 @@ void Process(VertexData& vertex, bool hasColor)
// TODO: Not sure if this really should be done even if lighting is disabled altogether
if (gstate.getUVGenMode() == GE_TEXMAP_ENVIRONMENT_MAP) {
Vec3<float> L = Vec3<float>(getFloat24(gstate.lpos[3 * light]), getFloat24(gstate.lpos[3 * light + 1]),getFloat24(gstate.lpos[3 * light + 2]));
float diffuse_factor = Dot(L.Normalized(), vertex.worldnormal);
// In other words, L.Length() == 0.0f means Dot({0, 0, 1}, worldnormal).
float diffuse_factor = L.Length() == 0.0f ? vertex.worldnormal.z : Dot(L.Normalized(), vertex.worldnormal);

if (gstate.getUVLS0() == (int)light)
vertex.texturecoords.s() = (diffuse_factor + 1.f) / 2.f;
Expand Down
8 changes: 6 additions & 2 deletions GPU/Vulkan/VertexShaderGeneratorVulkan.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -602,8 +602,12 @@ bool GenerateVulkanGLSLVertexShader(const VShaderID &id, char *buffer) {
break;

case GE_TEXMAP_ENVIRONMENT_MAP: // Shade mapping - use dots from light sources.
WRITE(p, " v_texcoord = vec3(base.uvscaleoffset.xy * vec2(1.0 + dot(normalize(light.pos[%i]), worldnormal), 1.0 + dot(normalize(light.pos[%i]), worldnormal)) * 0.5, 1.0);\n", ls0, ls1);
break;
{
std::string lightFactor0 = StringFromFormat("(length(light.pos[%i]) == 0.0 ? worldnormal.z : dot(normalize(light.pos[%i]), worldnormal))", ls0, ls0);
std::string lightFactor1 = StringFromFormat("(length(light.pos[%i]) == 0.0 ? worldnormal.z : dot(normalize(light.pos[%i]), worldnormal))", ls1, ls1);
WRITE(p, " v_texcoord = vec3(base.uvscaleoffset.xy * vec2(1.0 + %s, 1.0 + %s) * 0.5, 1.0);\n", lightFactor0.c_str(), lightFactor1.c_str());
}
break;

default:
// ILLEGAL
Expand Down