SpringDescription parameter for the AnimationController fling method

[nt4f04uNd]

Description

I provided the fling method with a new parameter springDescription. It just adds more customization to the methon by allowing to override the existing internal constant variable and so permitting developers to manipulate how the resulting fling animation will look like.

Related Issues

#64400 - the original issue, though I then decided that tolerance shouldn't be public.

Tests

Added

Checklist

Before you create this PR, confirm that it meets all requirements listed below by checking the relevant checkboxes ([x]). This will ensure a smooth and quick review process.

• I read the [Contributor Guide] and followed the process outlined there for submitting PRs.
• I signed the [CLA].
• I read and followed the [Flutter Style Guide], including [Features we expect every widget to implement].
• I read the [Tree Hygiene] wiki page, which explains my responsibilities.
• I updated/added relevant documentation (doc comments with ///).
• All existing and new tests are passing.
• The analyzer (flutter analyze --flutter-repo) does not report any problems on my PR.
• I am willing to follow-up on review comments in a timely manner.

Breaking Change

• No, no existing tests failed, so this is not a breaking change.

[flutter-dashboard]

It looks like this pull request may not have tests. Please make sure to add tests before merging. If you need an exemption to this rule, contact Hixie on the #hackers channel in Chat.

Reviewers: Read the Tree Hygiene page and make sure this patch meets those guidelines before LGTMing.

[LongCatIsLooong]

I'm still not sure why animeWith won't work if you want to use a custom simulation. As I understand it, the "direction" of the animation has little meaning for a custom simulation. Could you shed a little light on why having the direction set to be always forward is problematic?

[nt4f04uNd]

@LongCatIsLooong refer to my comment here #60419 (comment).

See where I want to use this. When I have such animation, I want to have all the valid animation statuses: forward, complete, reverse, dismissed. Using animateWith will give me only two of them, which is so bad, it's so wrong when I have animation that actually goes back, but status says me that it's forward instead. This is severe bug to me, actually I have no clue about the actual status of the controller (I mean, I can write some workarounds in my code, but why is it even always forward at all?).

Fling is working fine and updates status properly, so I can track if my bottom page is now expanding or whether it is collapsing.

[LongCatIsLooong]

@nt4f04uNd that makes sense. Thanks for the explanation! In that case the documentation needs to be updated, as the custom spring is not guaranteed to be critically damped. Also I think the interaction between underdamped/overdamped springs with lowerBound, upBound is worth documenting.

[goderbauer]

@nt4f04uNd Do you have plans to follow up on the feedback soon-ish?

[nt4f04uNd]

@LongCatIsLooong @goderbauer sorry for delayed updates. I updated docs a bit, but I guess I'm not competent enough to document the relationships between various types of springs and lowerBound and upperBound.

I tested with different bounds and different types of springs, and criticallyDamped and overDamped springs behave as I would expect from them - which is overdamped is mostly like critically damped but slowed down, though the underDamped springs behave weirdly - they bounce way too fast and it looks more like flickering, rather than actual bouncing.

I'm not that much into all that stuff, so if you think it should be documented more detailed, I ask you to do that instead of me

[LongCatIsLooong]

@nt4f04uNd yeah it gets a bit hairy when the given spring has a damping ratio that's not 1. Does your use case involve using a non-critically damped spring? If not I think we can add an assert to ensure the custom SpringDescriptor supplied describes a critically damped spring and add in the doc that this method currently only supports critically damped springs.

[nt4f04uNd]

@LongCatIsLooong over damped seems to be working pretty ok and I use it as I want to slow down a bit the default critically damped spring and make it a bit smoother (I use the one I included in tests).

Did you observe any problems with over damped springs? Because I experience problems only with under damped as I said and if we want to add asserts, I would do that allowing to use both critically and over damped springs.

If you can't test it out, I can upload some videos of how fling with various springs looks like

[LongCatIsLooong]

I think the problem with using an overdamped SpringDescription is that it doesn't naturally stop at lowerBound (or upperBound), this could be unintuitive in your use case too because the animation may stop prematurely (e.g., if you present a new sheet with that fling animation, the sheet presentation animation may stop half screen if you set the damping ratio to a large number).

[nt4f04uNd]

@LongCatIsLooong that's not the case. If you set ratio to very large number, the animation will always end up in the upper/lower bound (depending on the direction), but what you actually will see when you do this - is drastically slowed down movement, it's not stopped. You can set timeDilation to e.g. 0.001 to ensure that.

But I think maybe we should actually document in this method the behaviour of different spring types to not confuse anyone who will actually try to do that.

[nt4f04uNd]

@LongCatIsLooong also, just to be sure, as with large ratio the animation may start moving just because of applying dilation (I'm not aware how it internally works), so I added listener to the controller and it reports about changes even without dilation, even though on the screen there's no animation at all

[LongCatIsLooong]

Oh you're right, it should eventually converge to 0:

flutter/packages/flutter/lib/src/physics/spring_simulation.dart

Lines 232 to 235 in 1510865

	double x(double time) {
	return _c1 * math.pow(math.e, _r1 * time) +
	_c2 * math.pow(math.e, _r2 * time);
	}

But I think maybe we should actually document in this method the behaviour of different spring types to not confuse anyone who will actually try to do that.

Sounds good 👍

[nt4f04uNd]

@LongCatIsLooong will we restrict underDamped though?

[LongCatIsLooong]

It looks like we clamp the value to [lowerBound, upperBound] so I assume with an underdamped spring it should still oscillate except when the sheet is supposed to overshoot, it stays at lowerBound or upperBound instead?

flutter/packages/flutter/lib/src/animation/animation_controller.dart

Lines 785 to 798 in 1510865

	void _tick(Duration elapsed) {
	_lastElapsedDuration = elapsed;
	final double elapsedInSeconds = elapsed.inMicroseconds.toDouble() / Duration.microsecondsPerSecond;
	assert(elapsedInSeconds >= 0.0);
	_value = _simulation!.x(elapsedInSeconds).clamp(lowerBound, upperBound);
	if (_simulation!.isDone(elapsedInSeconds)) {
	_status = (_direction == _AnimationDirection.forward) ?
	AnimationStatus.completed :
	AnimationStatus.dismissed;
	stop(canceled: false);
	}
	notifyListeners();
	_checkStatusChanged();
	}

I don't know if there's a use case for that, the movement doesn't feel natural (as the animation suddenly stops when it goes out of range, losing all its momentum, then regains the momentum after a short while). But I'm OK with just documenting the behavior and warning developers about the weirdness. @goderbauer do you think we should disallow underdamped springs in this case?

[goderbauer]

(PR triage): @LongCatIsLooong @nt4f04uNd are there still plans for this PR?

[nt4f04uNd]

@goderbauer I've been planning to add a few comment lines about how the springs work with fling, but been waiting for your response

But I'm OK with just documenting the behavior and warning developers about the weirdness. @goderbauer do you think we should disallow underdamped springs in this case?

[goderbauer]

@goderbauer do you think we should disallow underdamped springs in this case?

I think we should disallow it if it doesn't make sense and from the discussion so far it sounds like it wouldn't really make sense? We can always revisit that if we have a use case that requires it.

[nt4f04uNd]

@goderbauer @LongCatIsLooong I guess that's it

[LongCatIsLooong]

Thanks! There seems to be a merge conflict, could you resolve it?

[nt4f04uNd]

done

[LongCatIsLooong]

Thank you for the PR! LGTM.

[LongCatIsLooong]

@nt4f04uNd could you do a git pull upstream/master and see if that fixes the CI failure?

[nt4f04uNd]

@LongCatIsLooong do you mean git pull upstream master, yup? So, I should do that on my pull request branch and then what, push?

[LongCatIsLooong]

@nt4f04uNd yes. The linux docs check on CI seems to be failing for some unrelated reason so let's see if syncing to head fixes that.