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.

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] Recommend adding native support for dashed lines. #110723

Closed
chinmaygarde opened this issue Aug 31, 2022 · 12 comments
Closed

[Impeller] Recommend adding native support for dashed lines. #110723

chinmaygarde opened this issue Aug 31, 2022 · 12 comments
Labels
c: proposal A detailed proposal for a change to Flutter e: impeller Impeller rendering backend issues and features requests engine flutter/engine repository. See also e: labels. P3 Issues that are less important to the Flutter project team-engine Owned by Engine team triaged-engine Triaged by Engine team

Comments

@chinmaygarde
Copy link
Member

These should be about as fast as regular lines when using the Impeller backend. Instead of getting potentially thousands of paths through the dispatcher and then attempting to optimize drawing them, a single path with effects would be easier and faster for everyone.

@chinmaygarde chinmaygarde added engine flutter/engine repository. See also e: labels. e: impeller Impeller rendering backend issues and features requests c: proposal A detailed proposal for a change to Flutter P3 Issues that are less important to the Flutter project labels Aug 31, 2022
@jonahwilliams
Copy link
Member

In wonderous today the global timeline page includes one large dashed line, and a few smaller ones. On a large iPad Pro, we're spending nearly 15ms of time on the GPU in the required fill shaders to draw the eumlated dashed line. Its highly likely that exposing a framework level primitive for dashing would lead to a more performant implementation, though this needs investigation.

@bdero
Copy link
Member

bdero commented Feb 3, 2023

15 ms!? 🤔 This smells like something that could be improved without additional Impeller Entities features. Are each of the dashes coming through separate draw calls?

@jonahwilliams
Copy link
Member

Yes, dashed lines are emulated today by making N drawLine calls:

class _DashedLinePainter extends CustomPainter {
  _DashedLinePainter(this.vertical);
  final bool vertical;

  @override
  void paint(Canvas canvas, Size size) {
    double dashPx = 3, gapPx = 3, pos = 0;
    final paint = Paint()..color = Colors.white;
    if (vertical) {
      while (pos < size.height) {
        canvas.drawLine(Offset(0, pos), Offset(0, pos + dashPx), paint);
        pos += dashPx + gapPx;
      }
    } else {
      while (pos < size.width) {
        canvas.drawLine(Offset(pos, 0), Offset(pos + dashPx, 0), paint);
        pos += dashPx + gapPx;
      }
    }
  }

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

@bdero
Copy link
Member

bdero commented Feb 3, 2023

Impeller Scene can render millions of triangles across thousands of draw calls on an iPhone 6s under budget, so there's definitely some low hanging fruit here if the captured metrics are accurate.

@tvolkert
Copy link
Contributor

tvolkert commented Feb 3, 2023

FYI, here's how I've drawn dashed lines pre-Impeller -- I create a dashed path, then just stroke the path:

/// The `dash` parameter is a list of dash offsets and lengths. For example,
/// the array `[5, 10]` would result in dashes 5 pixels long followed by blank
/// spaces 10 pixels long.  The array `[5, 10, 5]` would result in a 5 pixel
/// dash, a 10 pixel gap, a 5 pixel dash, a 5 pixel gap, a 10 pixel dash, etc.
Path _dashPath(
  Path source,
  List<double> dash, {
  double offset = 0.5,
}) {
  final Path dest = Path();
  for (final PathMetric metric in source.computeMetrics()) {
    double distance = offset;
    bool draw = true;
    for (int i = 0; distance < metric.length; i = (i + 1) % dash.length) {
      final double len = dash[i];
      if (draw) {
        dest.addPath(metric.extractPath(distance, distance + len), Offset.zero);
      }
      distance += len;
      draw = !draw;
    }
  }

  return dest;
}

@bdero
Copy link
Member

bdero commented Feb 3, 2023

@tvolkert The stroke generator in Impeller can handle multi-contour paths, so your solution would indeed reduce to 2 draw calls per dashed line in Wondrous (assuming addPath just appends components to the same SkPath and doesn't end up tracking separate paths :) ).

@jonahwilliams
Copy link
Member

On one hand, we should probably just send a patch to wonderous to make this faster. On the other hand, these feels like a pretty sharp edge. While I would probably expect a single path to be faster than individual drawLines, the difference is pretty stark - and unfortunately doesn't show up in intuitive ways in the profile. On the other, other hand, any sorts of batching optimization on the backend would have similar problems with potential sharp edges. For example, if we batched based on paint contents, then alternating color would perform much worse than solid color.

@jonahwilliams
Copy link
Member

I'd be interested to learn a little more in how the clip/stencil/restore works, maybe we can be a bit smarter about it

@bdero
Copy link
Member

bdero commented Feb 3, 2023

On the other hand, these feels like a pretty sharp edge. While I would probably expect a single path to be faster than individual drawLines, the difference is pretty stark

I agree. There's clearly something else going on here that needs to be fixed if our simplest draw calls are taking up this much space on profiling/frame captures. Not being able to push thousands of light draw calls on a high end iOS device is unacceptable in my eyes.

@chinmaygarde
Copy link
Member Author

Cross referencing related issues #4858 and #112981. The second issue looks identical to this one. Is that right @dnfield? I'm going to close this one as a duplicate.

@jonahwilliams
Copy link
Member

The issue here ended up being that the fullscreen clip restore was destroying rendering performance. This was fixed, making the overhead not as noticable.

Its not clear that add support for dashed lines actually solves a problem here, closing.

Copy link

This thread has been automatically locked since there has not been any recent activity after it was closed. If you are still experiencing a similar issue, please open a new bug, including the output of flutter doctor -v and a minimal reproduction of the issue.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Dec 10, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
c: proposal A detailed proposal for a change to Flutter e: impeller Impeller rendering backend issues and features requests engine flutter/engine repository. See also e: labels. P3 Issues that are less important to the Flutter project team-engine Owned by Engine team triaged-engine Triaged by Engine team
Projects
No open projects
Archived in project
Development

No branches or pull requests

4 participants