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

Ensure sorted rects in ui.Canvas for legacy compatibility #49309

Merged
merged 2 commits into from
Dec 21, 2023
Merged

Ensure sorted rects in ui.Canvas for legacy compatibility #49309

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
187322c
Ensure sorted rects in ui.Canvas for legacy compatibility
flar Dec 21, 2023
a38cabf
cull zero-sized rects in ui.Canvas when filling them
flar Dec 21, 2023
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
26 changes: 24 additions & 2 deletions lib/ui/painting.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5774,6 +5774,18 @@ base class _NativeCanvas extends NativeFieldWrapperClass1 implements Canvas {
@Native<Void Function(Pointer<Void>)>(symbol: 'Canvas::save', isLeaf: true)
external void save();

static Rect _sorted(Rect rect) {
if (rect.isEmpty) {
Copy link
Contributor

Choose a reason for hiding this comment

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

This will be true if we have a zero rect. I'm not sure how much it really matters to capture that case, but if we're optimizing we might as well avoid the native calls for the zero rect.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We'd have to add a test for stroke vs fill. You still stroke a zero rect. This is approximately the same as SkCanvas where they just sort it and pass it along and then the filtering against zero happens when it figures out whether it wants to fill or stroke. Given the complication, I'm happy just having it sort the rect at this stage rather than testing here for both zero and negative independently and then factoring in the operation type.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hmmm, I suppose it isn't too complicated and would only happen for inverted rects in the first place. How expensive is the native call?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The code changes were pretty minor so I went with the suggestion...

rect = Rect.fromLTRB(
math.min(rect.left, rect.right),
math.min(rect.top, rect.bottom),
math.max(rect.left, rect.right),
math.max(rect.top, rect.bottom),
);
}
return rect;
}

@override
void saveLayer(Rect? bounds, Paint paint) {
if (bounds == null) {
Expand Down Expand Up @@ -5844,6 +5856,10 @@ base class _NativeCanvas extends NativeFieldWrapperClass1 implements Canvas {
@override
void clipRect(Rect rect, { ClipOp clipOp = ClipOp.intersect, bool doAntiAlias = true }) {
assert(_rectIsValid(rect));
rect = _sorted(rect);
// Even if rect is still empty - which implies it has a zero dimension -
// we still need to perform the clipRect operation as it will effectively
// nullify any further rendering until the next restore call.
_clipRect(rect.left, rect.top, rect.right, rect.bottom, clipOp.index, doAntiAlias);
}

Expand Down Expand Up @@ -5916,7 +5932,10 @@ base class _NativeCanvas extends NativeFieldWrapperClass1 implements Canvas {
@override
void drawRect(Rect rect, Paint paint) {
assert(_rectIsValid(rect));
_drawRect(rect.left, rect.top, rect.right, rect.bottom, paint._objects, paint._data);
rect = _sorted(rect);
if (paint.style != PaintingStyle.fill || !rect.isEmpty) {
_drawRect(rect.left, rect.top, rect.right, rect.bottom, paint._objects, paint._data);
}
}

@Native<Void Function(Pointer<Void>, Double, Double, Double, Double, Handle, Handle)>(symbol: 'Canvas::drawRect')
Expand Down Expand Up @@ -5944,7 +5963,10 @@ base class _NativeCanvas extends NativeFieldWrapperClass1 implements Canvas {
@override
void drawOval(Rect rect, Paint paint) {
assert(_rectIsValid(rect));
_drawOval(rect.left, rect.top, rect.right, rect.bottom, paint._objects, paint._data);
rect = _sorted(rect);
if (paint.style != PaintingStyle.fill || !rect.isEmpty) {
_drawOval(rect.left, rect.top, rect.right, rect.bottom, paint._objects, paint._data);
}
}

@Native<Void Function(Pointer<Void>, Double, Double, Double, Double, Handle, Handle)>(symbol: 'Canvas::drawOval')
Expand Down
232 changes: 160 additions & 72 deletions testing/dart/canvas_test.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -557,6 +557,95 @@ void main() async {
expect(tabToTofuComparison, isFalse);
});

test('drawRect, drawOval, and clipRect render with unsorted rectangles', () async {
final PictureRecorder recorder = PictureRecorder();
final Canvas canvas = Canvas(recorder);

canvas.drawColor(const Color(0xFFE0E0E0), BlendMode.src);

void draw(Rect rect, double x, double y, Color color) {
final Paint paint = Paint()
..color = color
..strokeWidth = 5.0;

final Rect tallThin = Rect.fromLTRB(
min(rect.left, rect.right) - 10,
rect.top,
min(rect.left, rect.right) - 10,
rect.bottom,
);
final Rect wideThin = Rect.fromLTRB(
rect.left,
min(rect.top, rect.bottom) - 10,
rect.right,
min(rect.top, rect.bottom) - 10,
);

canvas.save();
canvas.translate(x, y);

paint.style = PaintingStyle.fill;
canvas.drawRect(rect, paint);
canvas.drawRect(tallThin, paint);
canvas.drawRect(wideThin, paint);

canvas.save();
canvas.translate(0, 100);
paint.style = PaintingStyle.stroke;
canvas.drawRect(rect, paint);
canvas.drawRect(tallThin, paint);
canvas.drawRect(wideThin, paint);
canvas.restore();

canvas.save();
canvas.translate(100, 0);
paint.style = PaintingStyle.fill;
canvas.drawOval(rect, paint);
canvas.drawOval(tallThin, paint);
canvas.drawOval(wideThin, paint);
canvas.restore();

canvas.save();
canvas.translate(100, 100);
paint.style = PaintingStyle.stroke;
canvas.drawOval(rect, paint);
canvas.drawOval(tallThin, paint);
canvas.drawOval(wideThin, paint);
canvas.restore();

canvas.save();
canvas.translate(50, 50);

canvas.save();
canvas.clipRect(rect);
canvas.drawPaint(paint);
canvas.restore();

canvas.save();
canvas.clipRect(tallThin);
canvas.drawPaint(paint);
canvas.restore();

canvas.save();
canvas.clipRect(wideThin);
canvas.drawPaint(paint);
canvas.restore();

canvas.restore();

canvas.restore();
}

draw(const Rect.fromLTRB(10, 10, 40, 40), 50, 50, const Color(0xFF2196F3));
draw(const Rect.fromLTRB(40, 10, 10, 40), 250, 50, const Color(0xFF4CAF50));
draw(const Rect.fromLTRB(10, 40, 40, 10), 50, 250, const Color(0xFF9C27B0));
draw(const Rect.fromLTRB(40, 40, 10, 10), 250, 250, const Color(0xFFFF9800));

final Picture picture = recorder.endRecording();
final Image image = await picture.toImage(450, 450);
await comparer.addGoldenImage(image, 'render_unordered_rects.png');
});

Matcher closeToTransform(Float64List expected) => (dynamic v) {
Expect.type<Float64List>(v);
final Float64List value = v as Float64List;
Expand Down Expand Up @@ -676,83 +765,82 @@ void main() async {
Expect.fail('$value is too close to $expected');
};

test('Canvas.clipRect(doAA=true) affects canvas.getClipBounds', () async {
final PictureRecorder recorder = PictureRecorder();
final Canvas canvas = Canvas(recorder);
const Rect clipBounds = Rect.fromLTRB(10.2, 11.3, 20.4, 25.7);
const Rect clipExpandedBounds = Rect.fromLTRB(10, 11, 21, 26);
canvas.clipRect(clipBounds);

// Save initial return values for testing restored values
final Rect initialLocalBounds = canvas.getLocalClipBounds();
final Rect initialDestinationBounds = canvas.getDestinationClipBounds();
expect(initialLocalBounds, closeToRect(clipExpandedBounds));
expect(initialDestinationBounds, closeToRect(clipExpandedBounds));

canvas.save();
canvas.clipRect(const Rect.fromLTRB(0, 0, 15, 15));
// Both clip bounds have changed
expect(canvas.getLocalClipBounds(), notCloseToRect(clipExpandedBounds));
expect(canvas.getDestinationClipBounds(), notCloseToRect(clipExpandedBounds));
// Previous return values have not changed
expect(initialLocalBounds, closeToRect(clipExpandedBounds));
expect(initialDestinationBounds, closeToRect(clipExpandedBounds));
canvas.restore();

// save/restore returned the values to their original values
expect(canvas.getLocalClipBounds(), initialLocalBounds);
expect(canvas.getDestinationClipBounds(), initialDestinationBounds);

canvas.save();
canvas.scale(2, 2);
const Rect scaledExpandedBounds = Rect.fromLTRB(5, 5.5, 10.5, 13);
expect(canvas.getLocalClipBounds(), closeToRect(scaledExpandedBounds));
// Destination bounds are unaffected by transform
expect(canvas.getDestinationClipBounds(), closeToRect(clipExpandedBounds));
canvas.restore();

// save/restore returned the values to their original values
expect(canvas.getLocalClipBounds(), initialLocalBounds);
expect(canvas.getDestinationClipBounds(), initialDestinationBounds);
});

test('Canvas.clipRect(doAA=false) affects canvas.getClipBounds', () async {
final PictureRecorder recorder = PictureRecorder();
final Canvas canvas = Canvas(recorder);
const Rect clipBounds = Rect.fromLTRB(10.2, 11.3, 20.4, 25.7);
canvas.clipRect(clipBounds, doAntiAlias: false);
test('Canvas.clipRect affects canvas.getClipBounds', () async {
void testRect(Rect clipRect, bool doAA) {
final PictureRecorder recorder = PictureRecorder();
final Canvas canvas = Canvas(recorder);
canvas.clipRect(clipRect, doAntiAlias: doAA);

final Rect clipSortedBounds = Rect.fromLTRB(
min(clipRect.left, clipRect.right),
min(clipRect.top, clipRect.bottom),
max(clipRect.left, clipRect.right),
max(clipRect.top, clipRect.bottom),
);
Rect clipExpandedBounds;
if (doAA) {
clipExpandedBounds = Rect.fromLTRB(
clipSortedBounds.left.floorToDouble(),
clipSortedBounds.top.floorToDouble(),
clipSortedBounds.right.ceilToDouble(),
clipSortedBounds.bottom.ceilToDouble(),
);
} else {
clipExpandedBounds = clipSortedBounds;
}

// Save initial return values for testing restored values
final Rect initialLocalBounds = canvas.getLocalClipBounds();
final Rect initialDestinationBounds = canvas.getDestinationClipBounds();
expect(initialLocalBounds, closeToRect(clipBounds));
expect(initialDestinationBounds, closeToRect(clipBounds));
// Save initial return values for testing restored values
final Rect initialLocalBounds = canvas.getLocalClipBounds();
final Rect initialDestinationBounds = canvas.getDestinationClipBounds();
expect(initialLocalBounds, closeToRect(clipExpandedBounds));
expect(initialDestinationBounds, closeToRect(clipExpandedBounds));

canvas.save();
canvas.clipRect(const Rect.fromLTRB(0, 0, 15, 15));
// Both clip bounds have changed
expect(canvas.getLocalClipBounds(), notCloseToRect(clipExpandedBounds));
expect(canvas.getDestinationClipBounds(), notCloseToRect(clipExpandedBounds));
// Previous return values have not changed
expect(initialLocalBounds, closeToRect(clipExpandedBounds));
expect(initialDestinationBounds, closeToRect(clipExpandedBounds));
canvas.restore();

// save/restore returned the values to their original values
expect(canvas.getLocalClipBounds(), initialLocalBounds);
expect(canvas.getDestinationClipBounds(), initialDestinationBounds);

canvas.save();
canvas.scale(2, 2);
final Rect scaledExpandedBounds = Rect.fromLTRB(
clipExpandedBounds.left / 2.0,
clipExpandedBounds.top / 2.0,
clipExpandedBounds.right / 2.0,
clipExpandedBounds.bottom / 2.0,
);
expect(canvas.getLocalClipBounds(), closeToRect(scaledExpandedBounds));
// Destination bounds are unaffected by transform
expect(canvas.getDestinationClipBounds(), closeToRect(clipExpandedBounds));
canvas.restore();

// save/restore returned the values to their original values
expect(canvas.getLocalClipBounds(), initialLocalBounds);
expect(canvas.getDestinationClipBounds(), initialDestinationBounds);
}

canvas.save();
canvas.clipRect(const Rect.fromLTRB(0, 0, 15, 15));
// Both clip bounds have changed
expect(canvas.getLocalClipBounds(), notCloseToRect(clipBounds));
expect(canvas.getDestinationClipBounds(), notCloseToRect(clipBounds));
// Previous return values have not changed
expect(initialLocalBounds, closeToRect(clipBounds));
expect(initialDestinationBounds, closeToRect(clipBounds));
canvas.restore();
testRect(const Rect.fromLTRB(10.2, 11.3, 20.4, 25.7), false);
testRect(const Rect.fromLTRB(10.2, 11.3, 20.4, 25.7), true);

// save/restore returned the values to their original values
expect(canvas.getLocalClipBounds(), initialLocalBounds);
expect(canvas.getDestinationClipBounds(), initialDestinationBounds);
// LR swapped
testRect(const Rect.fromLTRB(20.4, 11.3, 10.2, 25.7), false);
testRect(const Rect.fromLTRB(20.4, 11.3, 10.2, 25.7), true);

canvas.save();
canvas.scale(2, 2);
const Rect scaledClipBounds = Rect.fromLTRB(5.1, 5.65, 10.2, 12.85);
expect(canvas.getLocalClipBounds(), closeToRect(scaledClipBounds));
// Destination bounds are unaffected by transform
expect(canvas.getDestinationClipBounds(), closeToRect(clipBounds));
canvas.restore();
// TB swapped
testRect(const Rect.fromLTRB(10.2, 25.7, 20.4, 11.3), false);
testRect(const Rect.fromLTRB(10.2, 25.7, 20.4, 11.3), true);

// save/restore returned the values to their original values
expect(canvas.getLocalClipBounds(), initialLocalBounds);
expect(canvas.getDestinationClipBounds(), initialDestinationBounds);
// LR and TB swapped
testRect(const Rect.fromLTRB(20.4, 25.7, 10.2, 11.3), false);
testRect(const Rect.fromLTRB(20.4, 25.7, 10.2, 11.3), true);
});

test('Canvas.clipRect with matrix affects canvas.getClipBounds', () async {
Expand Down