From a8682204dc3dfee150c24afdae3e8c275e14c130 Mon Sep 17 00:00:00 2001 From: Jim Van Verth Date: Thu, 17 Dec 2020 10:18:16 -0500 Subject: [PATCH] Fix shadow directional light bounds. The shadowRec bounds code didn't handle directional lights, that's now fixed. Also fixes normalization of the light direction -- it was only using two components, it should use all three. Bug: skia:10781 Change-Id: Ia7d39c5187f976627d017ac4abecbe1d1dc62712 Reviewed-on: https://skia-review.googlesource.com/c/skia/+/345126 Reviewed-by: Brian Osman Commit-Queue: Jim Van Verth --- gm/shadowutils.cpp | 2 +- src/core/SkDrawShadowInfo.cpp | 38 +++++++++++++++++++++++++------- src/gpu/GrSurfaceDrawContext.cpp | 2 +- src/utils/SkShadowUtils.cpp | 2 +- tests/ShadowTest.cpp | 23 +++++++++++++++++-- 5 files changed, 54 insertions(+), 13 deletions(-) diff --git a/gm/shadowutils.cpp b/gm/shadowutils.cpp index 572324fa64..eca7f6b0ae 100644 --- a/gm/shadowutils.cpp +++ b/gm/shadowutils.cpp @@ -266,7 +266,7 @@ DEF_SIMPLE_GM(shadow_utils_directional, canvas, 256, 384) { static constexpr SkScalar kHeight = 12.f; SkPath rrect(SkPath::RRect(SkRect::MakeLTRB(-25, -25, 25, 25), 10, 10)); - SkPoint3 lightPos = { -45, -45, 500 }; + SkPoint3 lightPos = { -45, -45, 45 }; SkColor ambientColor = SkColorSetARGB(0.02f * 255, 0, 0, 0); SkColor spotColor = SkColorSetARGB(0.35f * 255, 0, 0, 0); diff --git a/src/core/SkDrawShadowInfo.cpp b/src/core/SkDrawShadowInfo.cpp index f75b32b18b..e56b00dbb8 100644 --- a/src/core/SkDrawShadowInfo.cpp +++ b/src/core/SkDrawShadowInfo.cpp @@ -8,6 +8,7 @@ #include "include/core/SkMatrix.h" #include "include/core/SkPath.h" #include "include/core/SkRect.h" +#include "include/private/SkShadowFlags.h" #include "src/core/SkDrawShadowInfo.h" #include "src/utils/SkPolyUtils.h" @@ -149,11 +150,19 @@ void GetLocalBounds(const SkPath& path, const SkDrawShadowRec& rec, const SkMatr ambientBlur = SkDrawShadowMetrics::AmbientBlurRadius(occluderZ); // get spot params (in device space) - SkPoint devLightPos = SkPoint::Make(rec.fLightPos.fX, rec.fLightPos.fY); - ctm.mapPoints(&devLightPos, 1); - SkDrawShadowMetrics::GetSpotParams(occluderZ, devLightPos.fX, devLightPos.fY, - rec.fLightPos.fZ, rec.fLightRadius, - &spotBlur, &spotScale, &spotOffset); + if (SkToBool(rec.fFlags & SkShadowFlags::kDirectionalLight_ShadowFlag)) { + SkPoint3 lightDir = rec.fLightPos; + lightDir.normalize(); + SkDrawShadowMetrics::GetDirectionalParams(occluderZ, lightDir.fX, lightDir.fY, + lightDir.fZ, rec.fLightRadius, + &spotBlur, &spotScale, &spotOffset); + } else { + SkPoint devLightPos = SkPoint::Make(rec.fLightPos.fX, rec.fLightPos.fY); + ctm.mapPoints(&devLightPos, 1); + SkDrawShadowMetrics::GetSpotParams(occluderZ, devLightPos.fX, devLightPos.fY, + rec.fLightPos.fZ, rec.fLightRadius, + &spotBlur, &spotScale, &spotOffset); + } } else { SkScalar devToSrcScale = SkScalarInvert(ctm.getMinScale()); @@ -162,9 +171,22 @@ void GetLocalBounds(const SkPath& path, const SkDrawShadowRec& rec, const SkMatr ambientBlur = devSpaceAmbientBlur*devToSrcScale; // get spot params (in local space) - SkDrawShadowMetrics::GetSpotParams(occluderZ, rec.fLightPos.fX, rec.fLightPos.fY, - rec.fLightPos.fZ, rec.fLightRadius, - &spotBlur, &spotScale, &spotOffset); + if (SkToBool(rec.fFlags & SkShadowFlags::kDirectionalLight_ShadowFlag)) { + SkPoint3 lightDir = rec.fLightPos; + lightDir.normalize(); + SkDrawShadowMetrics::GetDirectionalParams(occluderZ, lightDir.fX, lightDir.fY, + lightDir.fZ, rec.fLightRadius, + &spotBlur, &spotScale, &spotOffset); + // light dir is in device space, so need to map spot offset back into local space + SkMatrix inverse; + if (ctm.invert(&inverse)) { + inverse.mapVectors(&spotOffset, 1); + } + } else { + SkDrawShadowMetrics::GetSpotParams(occluderZ, rec.fLightPos.fX, rec.fLightPos.fY, + rec.fLightPos.fZ, rec.fLightRadius, + &spotBlur, &spotScale, &spotOffset); + } // convert spot blur to local space spotBlur *= devToSrcScale; diff --git a/src/gpu/GrSurfaceDrawContext.cpp b/src/gpu/GrSurfaceDrawContext.cpp index 3721f2adb4..9d098d2b68 100644 --- a/src/gpu/GrSurfaceDrawContext.cpp +++ b/src/gpu/GrSurfaceDrawContext.cpp @@ -1067,7 +1067,7 @@ bool GrSurfaceDrawContext::drawFastShadow(const GrClip* clip, SkPoint3 devLightPos = rec.fLightPos; bool directional = SkToBool(rec.fFlags & kDirectionalLight_ShadowFlag); if (directional) { - ((SkPoint*)&devLightPos.fX)->normalize(); + devLightPos.normalize(); } else { // transform light viewMatrix.mapPoints((SkPoint*)&devLightPos.fX, 1); diff --git a/src/utils/SkShadowUtils.cpp b/src/utils/SkShadowUtils.cpp index 0c2b21b1da..f65877ee09 100644 --- a/src/utils/SkShadowUtils.cpp +++ b/src/utils/SkShadowUtils.cpp @@ -629,7 +629,7 @@ void SkBaseDevice::drawShadow(const SkPath& path, const SkDrawShadowRec& rec) { SkPoint3 zPlaneParams = rec.fZPlaneParams; SkPoint3 devLightPos = rec.fLightPos; if (directional) { - ((SkPoint*)&devLightPos.fX)->normalize(); + devLightPos.normalize(); } else { viewMatrix.mapPoints((SkPoint*)&devLightPos.fX, 1); } diff --git a/tests/ShadowTest.cpp b/tests/ShadowTest.cpp index 03029a38cf..7217c8f2b8 100644 --- a/tests/ShadowTest.cpp +++ b/tests/ShadowTest.cpp @@ -123,7 +123,7 @@ DEF_TEST(ShadowUtils, reporter) { } void check_xformed_bounds(skiatest::Reporter* reporter, const SkPath& path, const SkMatrix& ctm) { - const SkDrawShadowRec rec = { + SkDrawShadowRec rec = { SkPoint3::Make(0, 0, 4), SkPoint3::Make(100, 0, 600), 800.f, @@ -131,7 +131,7 @@ void check_xformed_bounds(skiatest::Reporter* reporter, const SkPath& path, cons 0x40000000, 0 }; - // TODO: implement bounds calculation for directional lights + // point light SkRect bounds; SkDrawShadowMetrics::GetLocalBounds(path, rec, ctm, &bounds); ctm.mapRect(&bounds); @@ -148,6 +148,25 @@ void check_xformed_bounds(skiatest::Reporter* reporter, const SkPath& path, cons if (verts) { REPORTER_ASSERT(reporter, bounds.contains(verts->bounds())); } + + // directional light + rec.fFlags |= SkShadowFlags::kDirectionalLight_ShadowFlag; + rec.fLightRadius = 2.0f; + SkDrawShadowMetrics::GetLocalBounds(path, rec, ctm, &bounds); + ctm.mapRect(&bounds); + + verts = SkShadowTessellator::MakeAmbient(path, ctm, rec.fZPlaneParams, true); + if (verts) { + REPORTER_ASSERT(reporter, bounds.contains(verts->bounds())); + } + + devLightPos = rec.fLightPos; + devLightPos.normalize(); + verts = SkShadowTessellator::MakeSpot(path, ctm, rec.fZPlaneParams, devLightPos, + rec.fLightRadius, false, true); + if (verts) { + REPORTER_ASSERT(reporter, bounds.contains(verts->bounds())); + } } void check_bounds(skiatest::Reporter* reporter, const SkPath& path) {