Skip to content

Conversation

@fzyzcjy
Copy link
Contributor

@fzyzcjy fzyzcjy commented Oct 21, 2022

  1. I will finish code details, refine code, add tests, make tests pass, etc, after a code review that thinks the rough idea is acceptable. It is because, from my past experience, reviews may request changing a lot. If the general idea is to be changed, all detailed implementation efforts are wasted :)
  2. The PR has an already-working counterpart, and it produces ~60FPS smooth experimental results. The benchmark results and detailed analysis is in chapter https://cjycode.com/flutter_smooth/benchmark/. All the source code is in https://github.com/fzyzcjy/engine/tree/flutter-smooth and https://github.com/fzyzcjy/flutter/tree/flutter-smooth.
  3. Possibly useful as a context to this PR, there is a whole chapter discussing the internals - how flutter_smooth is implemented. (Link: https://cjycode.com/flutter_smooth/design/)

Currently, the user of Ticker only knows the elapsed time. However, it looks reasonable to allow the user to know when the ticker thinks it starts ticking.

If this does not wanted to be widely used, maybe we can mark it as @protected or @visibleForTesting or @experimental.

As for where it is needed inside flutter_smooth, it is utilized to know the relative time between a few Tickers as well as the system. In other words, when Ticker A says it elapsed 1 second, flutter_smooth needs to know the startTime such that it knows what "1second" means in absolute time. Detailed code can be seen in https://github.com/fzyzcjy/flutter_smooth/blob/0c5db0ff270aa0c8cff28ea19055999627a8df6d/packages/smooth/lib/src/drop_in/list_view/shift.dart#L352-L356

Pre-launch Checklist

  • I read the Contributor Guide and followed the process outlined there for submitting PRs.
  • I read the Tree Hygiene wiki page, which explains my responsibilities.
  • I read and followed the Flutter Style Guide, including Features we expect every widget to implement.
  • I signed the CLA.
  • I listed at least one issue that this PR fixes in the description above.
  • I updated/added relevant documentation (doc comments with ///).
  • I added new tests to check the change I am making, or this PR is test-exempt.
  • All existing and new tests are passing.

If you need help, consider asking for advice on the #hackers-new channel on Discord.

@flutter-dashboard flutter-dashboard bot added the framework flutter/packages/flutter repository. See also f: labels. label Oct 21, 2022
/// When [_onTick], the callback is provided with a relative time (the
/// "elapsed"). By adding the `elapsed` to this [startTime], you can get the
/// absolute time.
Duration? get startTime => _startTime;
Copy link
Member

Choose a reason for hiding this comment

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

What does it mean when this returns null?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

When _startTime is really null. For example, a fresh new ticker that is created at idle period without ticked the first time - looking through Ticker code you can see it has startTime as null.

I guess we should not provide non-null assert here, because users of this field have no sufficient information to know whether it already has a value.

/// [isTicking].
bool get isActive => _future != null;

/// The start time of the ticker.
Copy link
Member

Choose a reason for hiding this comment

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

The Ticker is designed to measure relative time. I don't see a good reason to change that.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Well it is required for flutter_smooth, mainly here: https://github.com/fzyzcjy/flutter_smooth/blob/0c5db0ff270aa0c8cff28ea19055999627a8df6d/packages/smooth/lib/src/drop_in/list_view/shift.dart#L352-L356

Shortly speaking, I need to know the time that the ListView animation is using, such that I can fully replicate it in an external extra SmoothShift.

@fzyzcjy
Copy link
Contributor Author

fzyzcjy commented Nov 5, 2022

So... What should I do now? I have already replied to all questions and it has been more than a week :)

@goderbauer
Copy link
Member

I don't think we should be exposing random implementation details like that. I would suggest filing an issue discussing the problem you're trying to solve without tying it to a particular solution. We can then see if this fixes that particular problem.

@goderbauer goderbauer closed this Nov 16, 2022
@fzyzcjy
Copy link
Contributor Author

fzyzcjy commented Nov 16, 2022

I see, same as before, I will file soon when having time. Thanks!

@fzyzcjy
Copy link
Contributor Author

fzyzcjy commented Nov 23, 2022

The requested issue is created and described the details: #115901

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

framework flutter/packages/flutter repository. See also f: labels.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants