Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: Clamp opacity set by the ColorEffect to 1.0 #3069

Merged
merged 4 commits into from
Mar 6, 2024

Conversation

billy1380
Copy link
Contributor

@billy1380 billy1380 commented Mar 6, 2024

Description

Clamp opacity set by the ColorEffect to 1.0; this was causing an issue when the tween returned values greater than 1.

The test shows that when dt is greater than 0.5 and less than 0.75 for a period of 1 the tween returns approx 1.2 which causes Color to throw an exception.

This has now been fixed.

Since flutter/flutter#89433 has also now been fixed, I have changed the min opacity to 0.

Checklist

  • I have followed the Contributor Guide when preparing my PR.
  • I have updated/added tests for ALL new/updated/fixed functionality.
  • I have updated/added relevant documentation in docs and added dartdoc comments with ///.
  • I have updated/added relevant examples in examples or docs.

Breaking Change?

  • Yes, this PR is a breaking change.
  • No, this PR is not a breaking change.

Related Issues

- added unit test to attempt to set opacity greater than 1
@billy1380 billy1380 changed the title Fixed exception with tween returns value greater than 1 Fixed exception when tween returns value greater than 1 on ColorEffect Mar 6, 2024
@spydon spydon changed the title Fixed exception when tween returns value greater than 1 on ColorEffect fix: Thow an exception when tween returns a value greater than 1 Mar 6, 2024
@spydon spydon changed the title fix: Thow an exception when tween returns a value greater than 1 fix: Clamp opacity set by the ColorEffect to 1.0 Mar 6, 2024
Copy link
Member

@spydon spydon left a comment

Choose a reason for hiding this comment

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

Lgtm, the PR description just has to be updated

@billy1380
Copy link
Contributor Author

Lgtm, the PR description just has to be updated

All done now 🙏

@@ -46,7 +46,7 @@ class ColorEffect extends ComponentEffect<HasPaint> {
// Currently there is a bug when opacity is 0 in the color filter.
// "Expected a value of type 'SkDeletable', but got one of type 'Null'"
// https://github.com/flutter/flutter/issues/89433
max(_tween.transform(progress), 1 / 255),
min(max(_tween.transform(progress), 1 / 255), 1),
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
min(max(_tween.transform(progress), 1 / 255), 1),
min(max(_tween.transform(progress), 0), 1),

The bug mentioned above is fixed, maybe we could bundle this into the PR too? And remove the comment above.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I have updated the pr and the description.

Copy link
Member

Choose a reason for hiding this comment

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

Super, thanks a lot!

@spydon spydon enabled auto-merge (squash) March 6, 2024 13:34
auto-merge was automatically disabled March 6, 2024 13:39

Head branch was pushed to by a user without write access

@spydon spydon enabled auto-merge (squash) March 6, 2024 13:40
@spydon spydon merged commit 9282cc3 into flame-engine:main Mar 6, 2024
8 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants