Skip to content
This repository was archived by the owner on Feb 25, 2025. It is now read-only.

Conversation

@flar
Copy link
Contributor

@flar flar commented Aug 5, 2022

It may seem like this is a broken implementation calling drawLine on individual paths, but that is actually the semantics of the call. We could do these in parallel for performance, but they must behave as if they are individual calls to drawLine - complete with Polygon mode not connecting with joins and all of the individual lines/points/polygon segments over-drawing at their overlap points. The following test case helps show the behavior and shows the same behavior with and without Impeller.

DrawPoints interactive app
import 'dart:typed_data';
import 'dart:math' as math;
import 'dart:ui' as ui;

import 'package:flutter/material.dart';

void main() {
  runApp(MyApp());
}

class MyApp extends StatelessWidget {
  @override
  Widget build(BuildContext context) {
    return MaterialApp(
      title: 'Flutter Demo',
      theme: ThemeData(
        primarySwatch: Colors.blue,
      ),
      home: MyHomePage(title: 'Flutter Demo Home Page'),
    );
  }
}

class MyHomePage extends StatefulWidget {
  MyHomePage({Key? key, required this.title}) : super(key: key);

  final String title;

  @override
  _MyHomePageState createState() => _MyHomePageState();
}

class MobilePoint {
  static math.Random generator = math.Random();

  MobilePoint.random(this.size) {
    x = generator.nextDouble() * size.width;
    y = generator.nextDouble() * size.height;
    dx = (generator.nextDouble() - 0.5) * size.width / 100;
    dy = (generator.nextDouble() - 0.5) * size.height / 100;
  }

  void bump() {
    x += dx;
    if (x > size.width) {
      x = size.width * 2 - x;
      dx = -dx;
    } else if (x < 0) {
      x = -x;
      dx = -dx;
    }
    y += dy;
    if (y > size.height) {
      y = size.height * 2 - y;
      dy = -dy;
    } else if (y < 0) {
      y = -y;
      dy = -dy;
    }
  }

  Size size;
  double x = 0, y = 0;
  double dx = 0, dy = 0;
}

class _MyHomePageState extends State<MyHomePage> with SingleTickerProviderStateMixin {
  static Size pointFieldSize = Size(200, 200);
  List<MobilePoint> points = List<MobilePoint>.generate(100, (index) => MobilePoint.random(pointFieldSize));
  late AnimationController controller;

  @override
  void initState() {
    super.initState();
    controller = AnimationController(vsync: this);
    controller.repeat(period: Duration(seconds: 10));
  }

  @override
  void dispose() {
    controller.stop();
    controller.dispose();
    super.dispose();
  }

  @override
  Widget build(BuildContext context) {
    return Scaffold(
      appBar: AppBar(
        title: Text(widget.title),
      ),
      body: Center(
        child: AnimatedBuilder(
          animation: controller,
          builder: (context, child) {
            final Float32List pointList = Float32List(points.length * 2);
            for (int i = 0; i < points.length; i++) {
              points[i].bump();
              pointList[i * 2] = points[i].x;
              pointList[i * 2 + 1] = points[i].y;
            }
            return Column(
              mainAxisAlignment: MainAxisAlignment.spaceEvenly,
              crossAxisAlignment: CrossAxisAlignment.center,
              children: <Widget>[
                Row(
                  mainAxisAlignment: MainAxisAlignment.spaceEvenly,
                  children: <Widget>[
                    CustomPaint(
                      size: pointFieldSize,
                      painter: PointPainter(pointList, ui.PointMode.points),
                    ),
                    CustomPaint(
                      size: pointFieldSize,
                      painter: PointPainter(pointList, ui.PointMode.lines),
                    ),
                  ],
                ),
                Row(
                  mainAxisAlignment: MainAxisAlignment.spaceEvenly,
                  children: <Widget>[
                    CustomPaint(
                      size: pointFieldSize,
                      painter: PointPainter(pointList, ui.PointMode.polygon),
                    ),
                    CustomPaint(
                      size: pointFieldSize,
                      painter: PathPainter(points),
                    ),
                  ],
                ),
              ],
            );
          },
        ),
      ),
    );
  }
}

class PointPainter extends CustomPainter {
  PointPainter(this.list, this.mode);

  final ui.PointMode mode;
  final Float32List list;

  @override
  void paint(Canvas canvas, Size size) {
    Paint paint = Paint()
      ..color = Colors.blue.withAlpha(64)
      ..strokeWidth = 4.0
      ..strokeJoin = StrokeJoin.miter
      ..strokeCap = StrokeCap.square;
    canvas.drawRawPoints(mode, list, paint);
  }

  @override
  bool shouldRepaint(CustomPainter oldDelegate) => true;
}

class PathPainter extends CustomPainter {
  PathPainter(this.list);

  final List<MobilePoint> list;

  @override
  void paint(Canvas canvas, Size size) {
    Paint paint = Paint()
      ..color = Colors.blue.withAlpha(64)
      ..style = PaintingStyle.stroke
      ..strokeWidth = 4.0
      ..strokeJoin = StrokeJoin.miter
      ..strokeCap = StrokeCap.square;
    Path path = Path();
    path.moveTo(list[0].x, list[1].y);
    for (int i = 1; i < list.length; i++) {
      path.lineTo(list[i].x, list[i].y);
    }
    canvas.drawPath(path, paint);
  }

  @override
  bool shouldRepaint(CustomPainter oldDelegate) => true;
}

@flutter-dashboard
Copy link

It looks like this pull request may not have tests. Please make sure to add tests before merging. If you need an exemption to this rule, contact Hixie on the #hackers channel in Chat (don't just cc him here, he won't see it! He's on Discord!).

If you are not sure if you need tests, consider this rule of thumb: the purpose of a test is to make sure someone doesn't accidentally revert the fix. Ask yourself, is there anything in your PR that you feel it is important we not accidentally revert back to how it was before your fix?

Reviewers: Read the Tree Hygiene page and make sure this patch meets those guidelines before LGTMing.

@flar flar requested a review from bdero August 5, 2022 23:22
@bdero bdero changed the title Implement drawPoints in Impeller [Impeller] Implement drawPoints in Impeller Aug 6, 2022
}
for (uint32_t i = 0; i < count; i++) {
SkPoint p0 = points[i];
SkPoint p1 = points[i] + SkPoint{0.0001, 0.0};
Copy link
Member

@bdero bdero Aug 6, 2022

Choose a reason for hiding this comment

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

Nit: Maybe use kEhCloseEnough as the magic number for this hack (and perhaps a comment mentioning it's needed because stroke caps on 0 length line segments don't really work right)? 🤣

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done. See flutter/flutter#109077 and the new comment about kEhCloseEnough

Copy link
Member

@bdero bdero left a comment

Choose a reason for hiding this comment

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

Is there supposed to be overdraw happening for a single drawPoints call? I suspect the answer is yes but just in case: I used a stencil to prevent overdraw from happening when drawing strokes internally, so if overdraw isn't supposed to happen, building/drawing a single path with multiple contours can work here (as opposed to building/drawing several single contour paths).

image

@flar
Copy link
Contributor Author

flar commented Aug 6, 2022

Is there supposed to be overdraw happening for a single drawPoints call? I suspect the answer is yes but just in case: I used a stencil to prevent overdraw from happening when drawing strokes internally, so if overdraw isn't supposed to happen, building/drawing a single path with multiple contours can work here (as opposed to building/drawing several single contour paths).

I said exactly this in the description. This is what drawPoints does. Creating a single path will produce the wrong result, though there might be more optimal implementations that would put all of the lines into a single op and inform the stroking code NOT to do its over-draw optimizations, but that would involve a lot of digging around in the internals. Meanwhile, this dozen or so lines of code does exactly what the op is supposed to do.

@flar flar requested a review from bdero August 6, 2022 05:04
@bdero
Copy link
Member

bdero commented Aug 6, 2022

Sounds good and I agree: If it matches Skia's behavior we should land it.

@flar flar added the autosubmit Merge PR when tree becomes green via auto submit App label Aug 6, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

autosubmit Merge PR when tree becomes green via auto submit App e: impeller needs tests

Projects

No open projects
Archived in project

Development

Successfully merging this pull request may close these issues.

2 participants