From 722a9a66f3180ac9a4e47e76b206e0c8495211b4 Mon Sep 17 00:00:00 2001 From: Chinmay Garde Date: Tue, 12 Nov 2024 17:14:13 -0800 Subject: [PATCH] Revert "[Impeller] fix line/polygon depth and GLES scissor state. (#56494)" This reverts commit 950e240eb7a6bd52124d8efda6806a9eb3a18a83. This caused a regression in a customer application (@lyceel). I am not sure I should revert this whole patch. The goldens don't change if I just revert the patches to `render_pass_gles.cc`. So while that part was untested in the original commit, the other patches should be good. The original commit message did mention two separate issues were being addressed. I am working on reduced test case with just the scissor regression with the customer. --- impeller/display_list/aiks_dl_unittests.cc | 62 ------------------- impeller/display_list/canvas.cc | 9 +-- impeller/display_list/canvas.h | 5 +- impeller/display_list/dl_dispatcher.cc | 4 +- .../renderer/backend/gles/render_pass_gles.cc | 2 + testing/impeller_golden_tests_output.txt | 6 -- 6 files changed, 8 insertions(+), 80 deletions(-) diff --git a/impeller/display_list/aiks_dl_unittests.cc b/impeller/display_list/aiks_dl_unittests.cc index 01aff4309ad26..279b7bc00a48f 100644 --- a/impeller/display_list/aiks_dl_unittests.cc +++ b/impeller/display_list/aiks_dl_unittests.cc @@ -3,14 +3,12 @@ // Use of this source code is governed by a BSD-style license that can be // found in the LICENSE file. -#include #include "display_list/dl_sampling_options.h" #include "display_list/dl_tile_mode.h" #include "display_list/effects/dl_color_filter.h" #include "display_list/effects/dl_color_source.h" #include "display_list/effects/dl_image_filter.h" #include "display_list/geometry/dl_geometry_types.h" -#include "display_list/geometry/dl_path.h" #include "display_list/image/dl_image.h" #include "flutter/impeller/display_list/aiks_unittests.h" @@ -23,9 +21,7 @@ #include "impeller/display_list/dl_dispatcher.h" #include "impeller/display_list/dl_image_impeller.h" #include "impeller/geometry/scalar.h" -#include "include/core/SkCanvas.h" #include "include/core/SkMatrix.h" -#include "include/core/SkPath.h" #include "include/core/SkRSXform.h" #include "include/core/SkRefCnt.h" @@ -986,63 +982,5 @@ TEST_P(AiksTest, CanEmptyPictureConvertToImage) { ASSERT_TRUE(OpenPlaygroundHere(recorder_builder.Build())); } -TEST_P(AiksTest, DepthValuesForLineMode) { - // Ensures that the additional draws created by line/polygon mode all - // have the same depth values. - DisplayListBuilder builder; - - SkPath path = SkPath::Circle(100, 100, 100); - - builder.DrawPath(path, DlPaint() - .setColor(DlColor::kRed()) - .setDrawStyle(DlDrawStyle::kStroke) - .setStrokeWidth(5)); - builder.Save(); - builder.ClipPath(path); - - std::vector points = { - DlPoint::MakeXY(0, -200), DlPoint::MakeXY(400, 200), - DlPoint::MakeXY(0, -100), DlPoint::MakeXY(400, 300), - DlPoint::MakeXY(0, 0), DlPoint::MakeXY(400, 400), - DlPoint::MakeXY(0, 100), DlPoint::MakeXY(400, 500), - DlPoint::MakeXY(0, 150), DlPoint::MakeXY(400, 600)}; - - builder.DrawPoints(DisplayListBuilder::PointMode::kLines, points.size(), - points.data(), - DlPaint().setColor(DlColor::kBlue()).setStrokeWidth(10)); - builder.Restore(); - - ASSERT_TRUE(OpenPlaygroundHere(builder.Build())); -} - -TEST_P(AiksTest, DepthValuesForPolygonMode) { - // Ensures that the additional draws created by line/polygon mode all - // have the same depth values. - DisplayListBuilder builder; - - SkPath path = SkPath::Circle(100, 100, 100); - - builder.DrawPath(path, DlPaint() - .setColor(DlColor::kRed()) - .setDrawStyle(DlDrawStyle::kStroke) - .setStrokeWidth(5)); - builder.Save(); - builder.ClipPath(path); - - std::vector points = { - DlPoint::MakeXY(0, -200), DlPoint::MakeXY(400, 200), - DlPoint::MakeXY(0, -100), DlPoint::MakeXY(400, 300), - DlPoint::MakeXY(0, 0), DlPoint::MakeXY(400, 400), - DlPoint::MakeXY(0, 100), DlPoint::MakeXY(400, 500), - DlPoint::MakeXY(0, 150), DlPoint::MakeXY(400, 600)}; - - builder.DrawPoints(DisplayListBuilder::PointMode::kPolygon, points.size(), - points.data(), - DlPaint().setColor(DlColor::kBlue()).setStrokeWidth(10)); - builder.Restore(); - - ASSERT_TRUE(OpenPlaygroundHere(builder.Build())); -} - } // namespace testing } // namespace impeller diff --git a/impeller/display_list/canvas.cc b/impeller/display_list/canvas.cc index eae343d2fcd2f..ba53272912ef4 100644 --- a/impeller/display_list/canvas.cc +++ b/impeller/display_list/canvas.cc @@ -446,17 +446,13 @@ bool Canvas::AttemptDrawBlurredRRect(const Rect& rect, return true; } -void Canvas::DrawLine(const Point& p0, - const Point& p1, - const Paint& paint, - bool reuse_depth) { +void Canvas::DrawLine(const Point& p0, const Point& p1, const Paint& paint) { Entity entity; entity.SetTransform(GetCurrentTransform()); entity.SetBlendMode(paint.blend_mode); LineGeometry geom(p0, p1, paint.stroke_width, paint.stroke_cap); - AddRenderEntityWithFiltersToCurrentPass(entity, &geom, paint, - /*reuse_depth=*/reuse_depth); + AddRenderEntityWithFiltersToCurrentPass(entity, &geom, paint); } void Canvas::DrawRect(const Rect& rect, const Paint& paint) { @@ -576,6 +572,7 @@ void Canvas::ClipGeometry(const Geometry& geometry, // See https://github.com/flutter/flutter/issues/147021 FML_DCHECK(current_depth_ <= transform_stack_.back().clip_depth) << current_depth_ << " <=? " << transform_stack_.back().clip_depth; + uint32_t clip_depth = transform_stack_.back().clip_depth; const Matrix clip_transform = diff --git a/impeller/display_list/canvas.h b/impeller/display_list/canvas.h index fbb4a8bb0abde..b3d336cd49f43 100644 --- a/impeller/display_list/canvas.h +++ b/impeller/display_list/canvas.h @@ -187,10 +187,7 @@ class Canvas { void DrawPaint(const Paint& paint); - void DrawLine(const Point& p0, - const Point& p1, - const Paint& paint, - bool reuse_depth = false); + void DrawLine(const Point& p0, const Point& p1, const Paint& paint); void DrawRect(const Rect& rect, const Paint& paint); diff --git a/impeller/display_list/dl_dispatcher.cc b/impeller/display_list/dl_dispatcher.cc index 1bdad62aff584..8e035961c2727 100644 --- a/impeller/display_list/dl_dispatcher.cc +++ b/impeller/display_list/dl_dispatcher.cc @@ -675,7 +675,7 @@ void DlDispatcherBase::drawPoints(PointMode mode, for (uint32_t i = 1; i < count; i += 2) { Point p0 = points[i - 1]; Point p1 = points[i]; - GetCanvas().DrawLine(p0, p1, paint, /*reuse_depth=*/i > 1); + GetCanvas().DrawLine(p0, p1, paint); } break; case flutter::DlCanvas::PointMode::kPolygon: @@ -683,7 +683,7 @@ void DlDispatcherBase::drawPoints(PointMode mode, Point p0 = points[0]; for (uint32_t i = 1; i < count; i++) { Point p1 = points[i]; - GetCanvas().DrawLine(p0, p1, paint, /*reuse_depth=*/i > 1); + GetCanvas().DrawLine(p0, p1, paint); p0 = p1; } } diff --git a/impeller/renderer/backend/gles/render_pass_gles.cc b/impeller/renderer/backend/gles/render_pass_gles.cc index f8c1a25fd997d..b0359c1640873 100644 --- a/impeller/renderer/backend/gles/render_pass_gles.cc +++ b/impeller/renderer/backend/gles/render_pass_gles.cc @@ -369,6 +369,8 @@ static bool BindVertexBuffer(const ProcTableGLES& gl, scissor.GetWidth(), // width scissor.GetHeight() // height ); + } else { + gl.Disable(GL_SCISSOR_TEST); } //-------------------------------------------------------------------------- diff --git a/testing/impeller_golden_tests_output.txt b/testing/impeller_golden_tests_output.txt index f7f9601873af6..fddecde08074b 100644 --- a/testing/impeller_golden_tests_output.txt +++ b/testing/impeller_golden_tests_output.txt @@ -575,12 +575,6 @@ impeller_Play_AiksTest_CoordinateConversionsAreCorrect_Vulkan.png impeller_Play_AiksTest_CoverageOriginShouldBeAccountedForInSubpasses_Metal.png impeller_Play_AiksTest_CoverageOriginShouldBeAccountedForInSubpasses_OpenGLES.png impeller_Play_AiksTest_CoverageOriginShouldBeAccountedForInSubpasses_Vulkan.png -impeller_Play_AiksTest_DepthValuesForLineMode_Metal.png -impeller_Play_AiksTest_DepthValuesForLineMode_OpenGLES.png -impeller_Play_AiksTest_DepthValuesForLineMode_Vulkan.png -impeller_Play_AiksTest_DepthValuesForPolygonMode_Metal.png -impeller_Play_AiksTest_DepthValuesForPolygonMode_OpenGLES.png -impeller_Play_AiksTest_DepthValuesForPolygonMode_Vulkan.png impeller_Play_AiksTest_DestructiveBlendColorFilterFloodsClip_Metal.png impeller_Play_AiksTest_DestructiveBlendColorFilterFloodsClip_OpenGLES.png impeller_Play_AiksTest_DestructiveBlendColorFilterFloodsClip_Vulkan.png