From bc5e5afc7be272d04f8b1767895dfec6d3dbc455 Mon Sep 17 00:00:00 2001 From: Jelle Raaijmakers Date: Wed, 23 Mar 2022 12:00:33 +0100 Subject: [PATCH] LibSoftGPU+LibGfx: Transform and normalize normals before lighting We were transforming the vertices' normals twice (bug 1) and normalizing them after lighting (bug 2). In the lighting code, we were then diverting from the spec to deal with the normal situation, which is now no longer needed. This fixes the lighting of Tux in Tux Racer. --- Userland/Libraries/LibGfx/Matrix4x4.h | 12 ++------- Userland/Libraries/LibSoftGPU/Device.cpp | 34 +++++++++++------------- 2 files changed, 17 insertions(+), 29 deletions(-) diff --git a/Userland/Libraries/LibGfx/Matrix4x4.h b/Userland/Libraries/LibGfx/Matrix4x4.h index 4e0c6dda8c..ed767e48c9 100644 --- a/Userland/Libraries/LibGfx/Matrix4x4.h +++ b/Userland/Libraries/LibGfx/Matrix4x4.h @@ -27,6 +27,8 @@ constexpr static Vector4 operator*(const Matrix4x4& m, const Vector4& v v.x() * elements[3][0] + v.y() * elements[3][1] + v.z() * elements[3][2] + v.w() * elements[3][3]); } +// FIXME: this is a specific Matrix4x4 * Vector3 interaction that implies W=1; maybe move this out of LibGfx +// or replace a Matrix4x4 * Vector4 operation? template constexpr static Vector3 transform_point(const Matrix4x4& m, const Vector3& p) { @@ -37,16 +39,6 @@ constexpr static Vector3 transform_point(const Matrix4x4& m, const Vector3 p.x() * elements[2][0] + p.y() * elements[2][1] + p.z() * elements[2][2] + elements[2][3]); } -template -constexpr static Vector3 transform_direction(const Matrix4x4& m, const Vector3& d) -{ - auto const& elements = m.elements(); - return Vector3( - d.x() * elements[0][0] + d.y() * elements[0][1] + d.z() * elements[0][2], - d.x() * elements[1][0] + d.y() * elements[1][1] + d.z() * elements[1][2], - d.x() * elements[2][0] + d.y() * elements[2][1] + d.z() * elements[2][2]); -} - template constexpr static Matrix4x4 translation_matrix(const Vector3& p) { diff --git a/Userland/Libraries/LibSoftGPU/Device.cpp b/Userland/Libraries/LibSoftGPU/Device.cpp index 669d18a30f..f5380bb78d 100644 --- a/Userland/Libraries/LibSoftGPU/Device.cpp +++ b/Userland/Libraries/LibSoftGPU/Device.cpp @@ -730,10 +730,15 @@ void Device::draw_primitives(PrimitiveType primitive_type, FloatMatrix4x4 const& triangle.vertices[1].eye_coordinates = model_view_transform * triangle.vertices[1].position; triangle.vertices[2].eye_coordinates = model_view_transform * triangle.vertices[2].position; - // Transform the vertex normals into eye-space - triangle.vertices[0].normal = transform_direction(model_view_transform, triangle.vertices[0].normal); - triangle.vertices[1].normal = transform_direction(model_view_transform, triangle.vertices[1].normal); - triangle.vertices[2].normal = transform_direction(model_view_transform, triangle.vertices[2].normal); + // Transform normals before use in lighting + triangle.vertices[0].normal = normal_transform * triangle.vertices[0].normal; + triangle.vertices[1].normal = normal_transform * triangle.vertices[1].normal; + triangle.vertices[2].normal = normal_transform * triangle.vertices[2].normal; + if (m_options.normalization_enabled) { + triangle.vertices[0].normal.normalize(); + triangle.vertices[1].normal.normalize(); + triangle.vertices[2].normal.normalize(); + } // Calculate per-vertex lighting if (m_options.lighting_enabled) { @@ -823,22 +828,23 @@ void Device::draw_primitives(PrimitiveType primitive_type, FloatMatrix4x4 const& // Diffuse auto const normal_dot_vertex_to_light = sgi_dot_operator(vertex.normal, vertex_to_light); - auto const diffuse_component = ((diffuse * light.diffuse_intensity) * normal_dot_vertex_to_light); + auto const diffuse_component = diffuse * light.diffuse_intensity * normal_dot_vertex_to_light; // Specular FloatVector4 specular_component = { 0.0f, 0.0f, 0.0f, 0.0f }; if (normal_dot_vertex_to_light > 0.0f) { FloatVector3 half_vector_normalized; if (!m_lighting_model.viewer_at_infinity) { - half_vector_normalized = (vertex_to_light + FloatVector3(0.0f, 0.0f, 1.0f)).normalized(); + half_vector_normalized = vertex_to_light + FloatVector3(0.0f, 0.0f, 1.0f); } else { - auto const vertex_to_eye_point = sgi_arrow_operator(vertex.eye_coordinates.normalized(), { 0.f, 0.f, 0.f, 1.f }, vertex_to_light_length); + auto const vertex_to_eye_point = sgi_arrow_operator(vertex.eye_coordinates, { 0.f, 0.f, 0.f, 1.f }, vertex_to_light_length); half_vector_normalized = vertex_to_light + vertex_to_eye_point; } + half_vector_normalized.normalize(); - auto const normal_dot_half_vector = sgi_dot_operator(vertex.normal.normalized(), half_vector_normalized); + auto const normal_dot_half_vector = sgi_dot_operator(vertex.normal, half_vector_normalized); auto const specular_coefficient = AK::pow(normal_dot_half_vector, material.shininess); - specular_component = (specular * light.specular_intensity) * specular_coefficient; + specular_component = specular * light.specular_intensity * specular_coefficient; } auto color = ambient_component + diffuse_component + specular_component; @@ -938,16 +944,6 @@ void Device::draw_primitives(PrimitiveType primitive_type, FloatMatrix4x4 const& if (area > 0) swap(triangle.vertices[0], triangle.vertices[1]); - // Transform normals - triangle.vertices[0].normal = normal_transform * triangle.vertices[0].normal; - triangle.vertices[1].normal = normal_transform * triangle.vertices[1].normal; - triangle.vertices[2].normal = normal_transform * triangle.vertices[2].normal; - if (m_options.normalization_enabled) { - triangle.vertices[0].normal.normalize(); - triangle.vertices[1].normal.normalize(); - triangle.vertices[2].normal.normalize(); - } - if (texture_coordinate_generation_enabled) { generate_texture_coordinates(triangle.vertices[0], m_options); generate_texture_coordinates(triangle.vertices[1], m_options);