Skip to content

Conversation

@gspencergoog
Copy link
Contributor

This enables the Paint.strokeJoin and Paint.strokeMiterLimit properties added to the engine in PR flutter/engine#3777.

Also, added a new example that shows line join/cap types for various line-drawing primitives.

@gspencergoog
Copy link
Contributor Author

gspencergoog commented Jun 15, 2017

Also, this fixes #7199, and must be committed after the flutter/engine PR.

@Hixie
Copy link
Contributor

Hixie commented Jun 16, 2017

There's a cost to having these examples, I'm not sure it's really worth it. Most people won't be writing code at this level, and it's not clear to me that this example explains the system in much more detail than the other examples at this layer that we already have.

If we want them for pedagogical reasons, I think we're better off using a ## Sample code section somewhere (see e.g. https://master-docs-flutter-io.firebaseapp.com/flutter/rendering/CustomPainter-class.html), maybe with a screenshot (see e.g. the diagrams in https://master-docs-flutter-io.firebaseapp.com/flutter/dart-ui/TileMode-class.html which are based on code in https://github.com/flutter/assets-for-api-docs).

Shader,
Size,
StrokeCap,
StrokeJoin,
Copy link
Contributor

Choose a reason for hiding this comment

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

this change is valuable

@gspencergoog
Copy link
Contributor Author

OK, I'm fine with just reverting the example and adding some sample code somewhere. What's the process for editing the docs?

One reason I did the example was to show the popping of the miter joins so that it's clear that miter joins are a no-no on shapes where the angles are animated, but I guess they'll learn that pretty quickly.

I did think about how to improve Skia's miter limit code so that it is both easier to understand what the units are for it, and also make it not pop. Basically, I'd just clip it at a length equivalent to the miter limit instead of popping to a bevel. It involved adding some extra math and an extra couple of vertices for clipped miters, and (most damning of all) it's a breaking change, and would probably screw up font rendering or something else, so I decided it wasn't worth it.

@Hixie
Copy link
Contributor

Hixie commented Jun 16, 2017

The docs are all inline in the source. You already wrote the docs for this enum, this would just be a matter of adding more. You can see examples in other enums in that source file (e.g. TileMode and the others I cited above).

I didn't follow what you were saying about popping from your comment above and definitely didn't get that from the source, so we should definitely document that, ideally with diagrams (any diagrams you generate should be generated from source, like the example I cited above, so that we can easily adjust them later and adapt their style to our evolving style).

@gspencergoog
Copy link
Contributor Author

gspencergoog commented Jun 16, 2017

Oh, I see, sure. I was thinking you wanted it as part of a tutorial or something. I can put it into the source, no problem.

Below is an animation of the popping (and the output of my sample code). Look at the top one, that's set to the default miter limit. It's actually "as designed" for Skia (and pretty much any other system that supports a miter limit), and it's fine as long as you're not animating the angle.

[This is why I put in the comment about "popping" in the documentation for the API.]

popping

@Hixie
Copy link
Contributor

Hixie commented Jun 16, 2017

Ah, I see.

How did you record the animation? Is it possible to include that in the docs? (Not that precise image, but a cropped one that shows the problem with a smooth cycling animation, say.)

@gspencergoog
Copy link
Contributor Author

Just used screenrecord on adb, and converted it to a GIF. I'll clean it up and put it in the docs.

@gspencergoog
Copy link
Contributor Author

Except how would you like me to automate generating that from source? Should I leave around the example code, or move it somewhere else (something like the curves documentation diagrams?)

I can also make a script that will build it using adb screenrecord, ffmpeg and imagemagick if you like (but it'll probably only run on Linux and Mac, which I think is probably OK).

@gspencergoog
Copy link
Contributor Author

Actually, that's silly. If I'm going to generate the GIF with imagemagick, I can just generate the frames with Flutter.

@Hixie
Copy link
Contributor

Hixie commented Jun 16, 2017

See https://github.com/flutter/assets-for-api-docs for the way we autogenerate the screenshots today. I've never tried using adb screenrecord, that sounds cool.

The key is that someone else should be able to regenerate the image with slight changes (e.g. changing the colors, or just running it again with a newer build to show newer antialiasing or to get a differently sized image or whatever), without having to think. If you look at the existing examples, you'll see they dump out the precise commands to run to get the PNG.

@gspencergoog
Copy link
Contributor Author

Is there any image format support in Flutter/Dart that would help? Seems like what I'd ideally like to do is use Flutter to draw into a canvas and then render that into a GIF stream and write it out while controlling the animation clock ticks so it's smooth. I imagine that not all parts of that scenario are implemented... :-)

@Hixie
Copy link
Contributor

Hixie commented Jun 17, 2017

Yeah, we don't yet have that. I've been getting the images using screen shots on actual devices. I believe we have the technology to do automated screenshots from simulators (cc @HansMuller) but I haven't tried using that yet. We don't yet have a way to take a Picture or Image and turn it into pixels or PNGs or something in a byte stream that can then be handled by the code.

@gspencergoog
Copy link
Contributor Author

gspencergoog commented Jun 17, 2017

Hmm. Seems like an opportunity for a 20%er... :-) . What's the policy on using third party open-source code in plugins? Something that compiles on iOS and Android, and could be made into a Flutter plugin, like FreeImage?

Or just a single format, like PNG, that's pretty simple...

@Hixie
Copy link
Contributor

Hixie commented Jun 17, 2017

Well in due course we want to actually expose encoders as well.

In the short term it's fine to use something like adb screenrecord and command line tools, so long as you say what the dependencies are and you give the commands that will regenerate the files.

@gspencergoog
Copy link
Contributor Author

OK, that's what I'll do then, for now.

@gspencergoog
Copy link
Contributor Author

gspencergoog commented Jun 17, 2017

I'll create the documentation and the diagram in another PR, if that's OK. (Well, I guess I have to, since it's in another repo).

@Hixie
Copy link
Contributor

Hixie commented Jun 17, 2017

you bet!

@Hixie
Copy link
Contributor

Hixie commented Jun 17, 2017

I'll land an engine roll then we can land this.

@Hixie
Copy link
Contributor

Hixie commented Jun 17, 2017

LGTM on the one-line change that remains.

@Hixie Hixie mentioned this pull request Jun 17, 2017
@Hixie
Copy link
Contributor

Hixie commented Jun 19, 2017

I've landed the necessary engine roll; I'll land this once the tests clear.

Merging changes from flutter/flutter
Updating my flutter fork to master.
@Hixie Hixie merged commit ddf25d2 into flutter:master Jun 26, 2017
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Aug 13, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants