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

[Impeller] fix drawPoints scaling factors. #54368

Merged
merged 11 commits into from
Aug 13, 2024
Merged

[Impeller] fix drawPoints scaling factors. #54368

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
11 commits
Select commit Hold shift + click to select a range
b897f3f
[Impeller] fix drawPoints scaling factors.
jonahwilliams Aug 6, 2024
7e6fdbf
use big number.
jonahwilliams Aug 6, 2024
ec28758
++
jonahwilliams Aug 6, 2024
badfb0c
add goldens.
jonahwilliams Aug 6, 2024
c174ab2
++
jonahwilliams Aug 6, 2024
1ce6a7b
tessellation adjustments.
jonahwilliams Aug 7, 2024
f372355
max basis XYZ
jonahwilliams Aug 8, 2024
f6ca941
remove extra radius.
jonahwilliams Aug 9, 2024
fcce4aa
update matrix.
jonahwilliams Aug 9, 2024
16ad4f5
set property.
jonahwilliams Aug 10, 2024
ce54247
Merge branch 'main' of github.com:flutter/engine into fix_draw_points…
jonahwilliams Aug 13, 2024
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
10 changes: 5 additions & 5 deletions impeller/entity/geometry/point_field_geometry.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,7 @@

#include "impeller/entity/geometry/point_field_geometry.h"

#include "impeller/core/vertex_buffer.h"
#include "impeller/core/formats.h"
#include "impeller/entity/geometry/geometry.h"
#include "impeller/renderer/command_buffer.h"

Expand All @@ -28,17 +28,17 @@ GeometryResult PointFieldGeometry::GetPositionBuffer(
return {};
}

Scalar min_size = 1.0f / sqrt(std::abs(determinant));
Scalar min_size = 0.5f / sqrt(std::abs(determinant));
Copy link
Member Author

Choose a reason for hiding this comment

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

I'm having a brain fart, should this actually be 0.5 / transform.GetMaxBasisLengthXY(); ?

Copy link
Contributor

Choose a reason for hiding this comment

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

I think so. Determinant of 2D matrices is the area of the (transformed) unit square which is pretty much the same thing, but it combines them geometrically so 2x in one direction balances 0.5 in the other. But matrix is a 3D thing so it becomes volume of the unit cube which might not be what we want. Or maybe it's OK since the volume of the cube is the area of the square times the height which is 1.0 - but still it will combine scales between the directions which is not what we want.

So, I think MaxBasisXY is a better value to use.

Really in all of these cases we should be doing separated 2D measurements rather than using the "max" or determinant. We should independently clamp each dimension separately.

Copy link
Contributor

Choose a reason for hiding this comment

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

And if they've imposed a Z scale then determinant would be not at all what we want. Another reason to go with MaxBasisXY

Copy link
Member Author

Choose a reason for hiding this comment

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

Done

Scalar radius = std::max(radius_, min_size);

HostBuffer& host_buffer = renderer.GetTransientsBuffer();
VertexBufferBuilder<SolidFillVertexShader::PerVertexData> vtx_builder;

if (round_) {
// Get triangulation relative to {0, 0} so we can translate it to each
// point in turn.
// point in turn. Note: we intentionally used the original radius here
// to capture any scaling < 1.0.
Copy link
Contributor

Choose a reason for hiding this comment

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

But the minimum size of stroked primitives is (somewhat arbitrarily chosen as) 1.0 device pixels which means you are allowing the transform to scale the size of the dots to (near) 0.

Copy link
Member Author

Choose a reason for hiding this comment

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

I will change this so we enforce the min size in the tessellator logic.

Right now we're rounding too early

Copy link
Contributor

Choose a reason for hiding this comment

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

The rounding was supposed to be reversible, but I think the issue here is that the rounding is 1-dimensional and the use of the rounded radius was 2-dimensional?

Also, tessellator is also used for filled shapes and those are allowed to scale to 0, so it minimally needs a flag indicating if the operation is a stroke or a fill in order to know when to apply the rounding.

Copy link
Contributor

Choose a reason for hiding this comment

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

I just realized that since the rounding code in the points code is using sqrt(determinant), it is probably suffering from the same issue that GetMaxBasisXYZ was suffering from. If the rouding in the points code uses GetMaxBasisXY instead, does that also solve the issue?

Copy link
Member Author

Choose a reason for hiding this comment

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

hmm good point. maybe then Tessellator::FilledCircle should have separate radius and pixel_radius arguments and we can let the caller manage.

I just realized that since the rounding code in the points code is using sqrt(determinant), it is probably suffering from the same issue that GetMaxBasisXYZ was suffering from. If the rouding in the points code uses GetMaxBasisXY instead, does that also solve the issue?

Ahh good catch

Copy link
Contributor

Choose a reason for hiding this comment

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

If the new radius works, then we should just use it in the call get get the generator below.

auto generator =
renderer.GetTessellator()->FilledCircle(transform, {}, radius);
renderer.GetTessellator()->FilledCircle(transform, {}, radius_);
Copy link
Member

Choose a reason for hiding this comment

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

We should have a test that asserts this. Probably a golden test that is similar to the reproduction code on the linked issue is easiest.

FML_DCHECK(generator.GetTriangleType() == PrimitiveType::kTriangleStrip);
std::vector<Point> circle_vertices;
circle_vertices.reserve(generator.GetVertexCount());
Expand Down
7 changes: 7 additions & 0 deletions impeller/geometry/matrix.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -298,6 +298,13 @@ struct Matrix {
Scalar GetMaxBasisLength() const;

constexpr Scalar GetMaxBasisLengthXY() const {
// The full basis computation requires computing the squared scaling factor
// for translate/scale only matrices. This substantially limits the range of
// precision for small and large scales. Instead, check for the common cases
// and directly return the max scaling factor.
if (IsTranslationScaleOnly()) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Technically, we only care if [0,1] and [1,0] are 0s. The rest of the entries don't matter since we're not planning on consulting them.

Copy link
Member Author

Choose a reason for hiding this comment

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

need to do this still.

Copy link
Contributor

Choose a reason for hiding this comment

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

Another thing to consider, probably in a follow-on PR, there are only a couple of other uses of the non XY version of this method and they should probably all be converted to the XY version and then the non-XY method removed so it doesn't cause more problems. All of our rendering ops are 2D geometry so I don't think there is any code in the system that would want to use the regular version of this method.

Copy link
Member Author

Choose a reason for hiding this comment

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

YEah that sounds like a good idea. I can take a pass after this change.

Copy link
Member Author

Choose a reason for hiding this comment

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

(Done with this one though)

return std::max(m[0], m[5]);
}
return std::sqrt(std::max(e[0][0] * e[0][0] + e[0][1] * e[0][1],
e[1][0] * e[1][0] + e[1][1] * e[1][1]));
}
Expand Down
16 changes: 16 additions & 0 deletions impeller/geometry/matrix_unittests.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@
#include "flutter/impeller/geometry/matrix.h"

#include "flutter/impeller/geometry/geometry_asserts.h"
#include "impeller/geometry/constants.h"

namespace impeller {
namespace testing {
Expand Down Expand Up @@ -160,5 +161,20 @@ TEST(MatrixTest, TransformHomogenous) {
Vector3(32.0f, 33.0f, 41.0f));
}

// Verifies a translate scale matrix doesn't need to compute sqrt(pow(scale, 2))
TEST(MatrixTest, GetMaxBasisXYWithLargeAndSmallScalingFactor) {
Matrix m = Matrix::MakeScale({2.625e+20, 2.625e+20, 1});
EXPECT_NEAR(m.GetMaxBasisLengthXY(), 2.625e+20, 1e+20);

m = Matrix::MakeScale({2.625e-20, 2.625e-20, 1});
EXPECT_NEAR(m.GetMaxBasisLengthXY(), 2.625e-20, 1e-20);
}

TEST(MatrixTest, GetMaxBasisXYWithLargeAndSmallScalingFactorNonScaleTranslate) {
Matrix m = Matrix::MakeScale({2.625e+20, 2.625e+20, 1}) *
Matrix::MakeRotationX(Radians(kPi / 2));
EXPECT_TRUE(std::isinf(m.GetMaxBasisLengthXY()));
Copy link
Contributor

Choose a reason for hiding this comment

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

This is failing, perhaps that isn't enough to create infinities on some platforms?

Could this be a case of a platform treating results that are too big for single precision as large, but not infinite values? I looked for answers and discovered that a compliant implementation is allowed to round to ::max if it is close enough. I think they said if it is closer to ::max than ::nextafter(::max). Perhaps use a larger exponent here to be sure (one that doesn't overflow ::max but squaring it overflows by a larger amount?

Copy link
Member Author

Choose a reason for hiding this comment

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

oh, I think its just not tripping on the scale/translate check anymore. I should just set one of matrix entires that will trigger it directly.

Copy link
Contributor

Choose a reason for hiding this comment

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

Is the math done in doubles and so the sqrt reduces it back into 32-bit float before the return?

Copy link
Member Author

Choose a reason for hiding this comment

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

oh, so the matrix is something like:

[ERROR:flutter/impeller/geometry/matrix_unittests.cc(176)] (
       2.000000,       0.000000,       0.000000,       0.000000,
       0.000000,       0.000000,      -2.000000,       0.000000,
       0.000000,       1.000000,       0.000000,       0.000000,
       0.000000,       0.000000,       0.000000,       1.000000,
)

am I making the right check here?

    if (e[0][1] == 0 && e[1][0] == 0) {
      return std::max(m[0], m[5]);
    }
    ```

Copy link
Contributor

@flar flar Aug 10, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

You rotated around the X axis which moved it into the 3rd dimension. We're seeing it end-on, so the Y scale went to zero as everything is in the Y=0 plane.

Copy link
Contributor

Choose a reason for hiding this comment

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

Another way to look at this is that if the optimization doesn't trigger then only these 4 entries will be used to compute the value. So, if 2 of them are 0 (2 that happen to be 0 a lot), then you can simplify the calculation.

One question is why you are mixing e and m representations? e[0][0] should be optimized by the compiler to "base of union + 0" and e[1][1] should be optimized to "base of union + 5" so m[0] and m[5] aren't any more efficient, are they?

Copy link
Member Author

Choose a reason for hiding this comment

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

No Its not an efficiency thing, I was just being a bit lazy. So otherwise this code is correct then?

Copy link
Contributor

Choose a reason for hiding this comment

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

"optimization" doesn't necessarily mean an efficiency thing. You did it to do less work because more work was causing an overflow. It's still a more direct path to the answer.

The new test code is one way to force the old computation to occur. Yes.

}

} // namespace testing
} // namespace impeller
14 changes: 7 additions & 7 deletions impeller/tessellator/tessellator.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -238,8 +238,8 @@ EllipticalVertexGenerator Tessellator::FilledCircle(
const Matrix& view_transform,
const Point& center,
Scalar radius) {
auto divisions =
ComputeQuadrantDivisions(view_transform.GetMaxBasisLength() * radius);
size_t divisions =
ComputeQuadrantDivisions(view_transform.GetMaxBasisLengthXY() * radius);
return EllipticalVertexGenerator(Tessellator::GenerateFilledCircle,
GetTrigsForDivisions(divisions),
PrimitiveType::kTriangleStrip, 4,
Expand All @@ -257,7 +257,7 @@ EllipticalVertexGenerator Tessellator::StrokedCircle(
Scalar half_width) {
if (half_width > 0) {
auto divisions = ComputeQuadrantDivisions(
view_transform.GetMaxBasisLength() * radius + half_width);
view_transform.GetMaxBasisLengthXY() * radius + half_width);
return EllipticalVertexGenerator(Tessellator::GenerateStrokedCircle,
GetTrigsForDivisions(divisions),
PrimitiveType::kTriangleStrip, 8,
Expand All @@ -280,7 +280,7 @@ EllipticalVertexGenerator Tessellator::RoundCapLine(
auto length = along.GetLength();
if (length > kEhCloseEnough) {
auto divisions =
ComputeQuadrantDivisions(view_transform.GetMaxBasisLength() * radius);
ComputeQuadrantDivisions(view_transform.GetMaxBasisLengthXY() * radius);
return EllipticalVertexGenerator(Tessellator::GenerateRoundCapLine,
GetTrigsForDivisions(divisions),
PrimitiveType::kTriangleStrip, 4,
Expand All @@ -302,8 +302,8 @@ EllipticalVertexGenerator Tessellator::FilledEllipse(
bounds.GetWidth() * 0.5f);
}
auto max_radius = bounds.GetSize().MaxDimension();
auto divisions =
ComputeQuadrantDivisions(view_transform.GetMaxBasisLength() * max_radius);
auto divisions = ComputeQuadrantDivisions(
view_transform.GetMaxBasisLengthXY() * max_radius);
auto center = bounds.GetCenter();
return EllipticalVertexGenerator(Tessellator::GenerateFilledEllipse,
GetTrigsForDivisions(divisions),
Expand All @@ -323,7 +323,7 @@ EllipticalVertexGenerator Tessellator::FilledRoundRect(
radii.height * 2 < bounds.GetHeight()) {
auto max_radius = radii.MaxDimension();
auto divisions = ComputeQuadrantDivisions(
view_transform.GetMaxBasisLength() * max_radius);
view_transform.GetMaxBasisLengthXY() * max_radius);
auto upper_left = bounds.GetLeftTop() + radii;
auto lower_right = bounds.GetRightBottom() - radii;
return EllipticalVertexGenerator(Tessellator::GenerateFilledRoundRect,
Expand Down
1 change: 1 addition & 0 deletions impeller/tessellator/tessellator_unittests.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@
// Use of this source code is governed by a BSD-style license that can be
// found in the LICENSE file.

#include <cmath>
Copy link
Member

Choose a reason for hiding this comment

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

Stray include, did you forget to include some tessellator tests?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah I had some but I removed them. The only way to test that we're using masBasisXY instead of XYZ is to assert the subdivision count - but these are supposed to be arbitrary so it doesnt seem like a good route. Will stick with goldens for now.

#include "flutter/testing/testing.h"
#include "gtest/gtest.h"

Expand Down