[Abandoned] Add StarBorder and StarBorder.polygon #107979
Conversation
gspencergoog
force-pushed
the
polygon_border
branch
from
af3d771 to
6a90c36
Compare
flutter-dashboard
bot
added
a: animation
gspencergoog
force-pushed
the
polygon_border
branch
from
6a90c36 to
73887ae
Compare
|
Wow. That's so good. I would never be able to do something like this that fast. |
|
Thanks.. I have been doing this for a while, though. Plus graphics stuff is my favorite kind of thing to build. |
|
Did you take a look at the Android implementation? Or is it 100% from your mind? It is so sophisticated... |
gspencergoog
force-pushed
the
polygon_border
branch
11 times, most recently
from
55797f4 to
e451429
Compare
|
This is small, don't worry if it is too hard to fix, but lerp 0.00 to 0.01 changes the size: Screen.Recording.2022-07-24.at.20.00.18.movI think squash is the reason: Screen.Recording.2022-07-24.at.20.25.15.mov |
|
Suggestion: add a constructor that calculates Also, I don't know what I did here, but it doesn't seem fine (again, if it is too hard or just an edge case, don't worry): |
gspencergoog
force-pushed
the
polygon_border
branch
3 times, most recently
from
76a7488 to
62eee7a
Compare
|
What you're seeing there in the last one you posted is numerical instability as the inner radius ratio goes to zero. I'm now clamping it to a minimum of 1e-6, which should avoid that. As for your suggestion around |
gspencergoog
force-pushed
the
polygon_border
branch
2 times, most recently
from
4103ad1 to
dd9a500
Compare
Hmmm.. :( makes sense. I still wish there were a 'simpler' way of dealing with That said, I like them, I think they should be exposed, just maybe not 'by default'. |
gspencergoog
force-pushed
the
polygon_border
branch
2 times, most recently
from
47d8011 to
b0d5615
Compare
gspencergoog
force-pushed
the
polygon_border
branch
from
8cd2950 to
2632c9a
Compare
|
Golden file changes have been found for this pull request. Click here to view and triage (e.g. because this is an intentional change). If you are still iterating on this change and are not ready to resolve the images on the Flutter Gold dashboard, consider marking this PR as a draft pull request above. You will still be able to view image results on the dashboard, commenting will be silenced, and the check will not try to resolve itself until marked ready for review. For more guidance, visit Writing a golden file test for Reviewers: Read the Tree Hygiene page and make sure this patch meets those guidelines before LGTMing. |
|
Golden file changes are available for triage from new commit, Click here to view. For more guidance, visit Writing a golden file test for Reviewers: Read the Tree Hygiene page and make sure this patch meets those guidelines before LGTMing. |
| @override | ||
| void dispose() { | ||
| super.dispose(); | ||
| _model.removeListener(_modelChanged); |
There was a problem hiding this comment.
textController.dispose()
| fontStyle: FontStyle.normal, | ||
| ), | ||
| child: SliderTheme( | ||
| data: SliderTheme.of(context).copyWith(), |
There was a problem hiding this comment.
This seems odd. If it's necessary, a comment that explains why would help.
There was a problem hiding this comment.
No, it was a half-compelted thought that happened to compile. Removed.
| mainAxisAlignment: MainAxisAlignment.spaceEvenly, | ||
| children: <Widget>[ | ||
| Expanded( | ||
| child: Container( |
There was a problem hiding this comment.
Could factor out a widget here and below, like:
class StarBorderView extends StatelessWidget {
const StarBorderView(this.star, this.title);
final StarBorder star;
final String title;
@override
Widget build(BuildContext) {
return Container(
key: UniqueKey(),
alignment: Alignment.center,
padding: const EdgeInsets.all(20),
width: 150,
height: 100,
decoration: ShapeDecoration(
color: Colors.blue.shade100,
shape: star,
),
child: const Text(title),
);
}
}There are other places where you could do this kind of thing. Not a big deal.
There was a problem hiding this comment.
OK, I factored this out. It does help with illustrating how to use it too, aside from the refactor.
| Expanded( | ||
| child: Container( | ||
| color: Colors.black12, | ||
| margin: const EdgeInsets.all(16.0), |
There was a problem hiding this comment.
padding: const EdgeInsets.all(32) has the same effect?
There was a problem hiding this comment.
No, margin is on the outside, padding is on the inside.
|
|
||
| @override | ||
| void dispose() { | ||
| super.dispose(); |
There was a problem hiding this comment.
Conventionally super.dispose() is last (here and elsewhere).
| points: points ?? this.points, | ||
| rotation: rotation ?? this.rotation, | ||
| innerRadiusRatio: innerRadiusRatio ?? this.innerRadiusRatio, | ||
| pointRounding: valleyRounding ?? this.pointRounding, |
There was a problem hiding this comment.
Looks like a typo here => valleyRounding
There was a problem hiding this comment.
Ooh! Good catch, thanks!
|
ContinuousRectangleBorder can't lerp to anything, it is one of the weirdest OutlinedBorders. Maybe you could apply what you learned in the star to make it better? Maybe it could extend StarBorder so it gets lerp for free? It seems like your polygonal is pretty close to ContinuousRectangleBorder and could help achieve what people want: #91523
|
Well, sort of. I don't think I could generate a superellipse with the polygon generator: it would generate something similar but not quite the same, and because it would have to be rotated 45 degrees (and I use the non-rotated radius to calculate the scale), it would be smaller than they expected. Best would be to just fix the |
|
Golden file changes are available for triage from new commit, Click here to view. For more guidance, visit Writing a golden file test for Reviewers: Read the Tree Hygiene page and make sure this patch meets those guidelines before LGTMing. |
Shall I open the issue? lol |
gspencergoog
added
the
autosubmit
gspencergoog
changed the title
|
I'm abandoning this PR for a fresh copy: #108489 This is because Skia Gold got really confused about which patchsets were which because of my force-pushing to this PR. The contents are the same. |
|
Leaving this open for a few days so the Skia Gold team can take a look at it. |
|
@kjlubick @LeandroLovisolo this is the PR associated with https://bugs.chromium.org/p/skia/issues/detail?id=13589 |
gspencergoog
removed
the
autosubmit
|
@kjlubick @LeandroLovisolo Please let me know when you're done investigating so that I can can close this stale PR. |
4 participants
Description
This adds
StarBorder, along with the interpolations to/fromCircleBorder,RoundedRectBorder, andStadiumBorder.The example looks like this Dartpad. (If it won't load because GitHub's API is overloaded, copy the Gist here into your own DartPad).
Related Issues
Tests