-
Notifications
You must be signed in to change notification settings - Fork 6k
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is amazing! Thanks for hacking on this, @yjbanov!
|
||
@JS() | ||
@staticInterop | ||
class DOMMatrix extends DOMMatrixReadOnly {} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not specific to this PR, but can we use this and delete our vector_math.dart
implementation? Seems to be widely available in modern browsers, and implements most (all?) of our Matrix4.
// DO_NOT_SUBMIT: revert this before submitting. | ||
// ..style.pointerEvents = 'none'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
REMINDER TO NOT SUBMIT THIS.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fwiw, one trick for this kind of thing is to use // TODO: revert before submitting
because then the todo style lint will make the analyzer shard red, preventing you from landing.
/// clip is set to "svgClip${_clipIdCounter}", e.g. "svgClip123". | ||
SVGSVGElement pathToSvgClipPath(ui.Path path, | ||
{double offsetX = 0, | ||
class SvgClip { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Cleaner as a class. Me like it!
|
||
@override | ||
void drawRRect(ui.RRect rrect, SurfacePaintData paint) { | ||
// TODO(yjbanov): this could be faster if specialized for rrect. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One optimization we could do is use an SVGRectElement
and change its rx
and ry
. Flutter's RRect is more flexible (e.g. each corner can have its own radius), so this optimization can only be applied in certain cases. But I would argue that in most cases, rrects are symmetric, which makes it hit this optimization case.
imageElement.setAttribute('clip-path', clip.url); | ||
imageElement.style | ||
..transformOrigin = '${dst.left}px ${dst.top}px 0' | ||
..transform = 'scale(calc(${dst.width / src.width}), calc(${dst.height / src.height})) ' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think calc(..)
is necessary here? You aren't asking the CSS engine to calculate anything. You are giving it a number.
Or maybe you wanted to do this:
..transform = 'scale(calc(${dst.width}/${src.width}), calc(${dst.height}/${src.height})) '
// the Y coordinate. However, Flutter positions the top edge of the text | ||
// line at the Y coordinate. For the SVG text to match what Flutter | ||
// expects, the baseline is added to the Y coordinate. | ||
spanElement.addY(boxRect.top + line.baseline); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(I think you already did this but haven't pushed it yet)
spanElement.addY(boxRect.top + line.baseline); | |
spanElement.addY(line.baseline); |
_sceneHostElement = domDocument.createElement('flt-scene-host') | ||
..style.pointerEvents = 'none'; | ||
_sceneHostElement = domDocument.createElement('flt-scene-host'); | ||
// DO_NOT_SUBMIT: revert this before submitting. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(Just to make the PR "red" :P)
external factory DOMPointInit({ | ||
double x = 0, | ||
double y = 0, | ||
double z = 0, | ||
double w = 1, | ||
}); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I normally don't separate the external factory
from the staticInterop class that it creates. I think you should be able to do this:
@JS()
@anonymous
@staticInterop
class DOMPoint {
external factory DOMPoint({
double x = 0,
double y = 0,
double z = 0,
double w = 1,
});
}
extension DOMPointExtension on DOMPoint {
external double get x;
external double get y;
external double get z;
external double get w;
}
(And then the types of stuff elsewhere can receive a DOMPoint
and a DOMMatrix
rather than the ...Init
versions?)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I might remove this. I was confused by MDN docs saying that SVGMatrix
is deprecated and we must use DOMMatrix
, but when I tried using it the browser didn't accept it. So I changed it to SVGMatrix
.
Couldn't schedule time to investigate this further. In particular, it's still unclear how to deal with the poor Safari performance until Safari itself optimizes SVG filters (there are bugs filed in WebKit dating back to 2012 on this, but no movement so far). Finally, the recent improvements in the size and performance of CanvasKit are getting closer to removing the need for an HTML renderer altogether. So spending time on this has dubious value. Closing for now. |
A proof-of-concept for a canvas that uses SVG exclusively to render all pictures. Reuses HTML renderer's layer tree and text layout. Only replaces the
PersistedPicture
implementation. Does not implement things that are not implementable in SVG with expected correctness and performance characteristics:drawVertices
anddrawPoints
.This is currently in a state where it can render the gallery 99% correctly, but probably contains a long tail of small issues that would need to be fixed before this can turn into anything serious. In particular, this needs a higher coverage of
Paint
features, such as gradients.Notes on performance
Tested on M1 MacBook Pro, iPhone 7, and Pixel 6 Pro.
Good news:
Bad news:
Next steps
I don't think this is ready for anything close to production. We should at least eliminate the obvious regressions. We could land this experimentally without enabling it, so we can iterate on it. All changes outside
lib/src/engine/html/svg
directory are meant to be non-breaking. They just generalize existing code so it can be used both in the HTML mode and SVG mode. If we decide to abandon this experiment, we can simply deletelib/src/engine/html/svg
and a couple of lines that refer to it, and forget this ever happened. The actual "SVG canvas" part is ~450 LoC (svg_canvas.dart
+svg_picture.dart
).The other thing is that this should not become yet another canvas, but a replacement for both
dom_canvas.dart
andbitmap_canvas.dart
. If we can't get it to a point that it fully replaces the old canvases, we should drop it.