-
Notifications
You must be signed in to change notification settings - Fork 149
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
[Compound] Bloom #1253
[Compound] Bloom #1253
Conversation
.../snapshots/images/ui_S_t[f.roomlist.impl_null_RoomListViewLight_0_null_3,NEXUS_5,1.0,en].png
Outdated
Show resolved
Hide resolved
- Remove component based bloom. - Reduce recalculation in recompositions. - Add `BloomDefaults`. - Improve names of functions and parameters. - Add documentation and preview group.
📱 Scan the QR code below to install the build (arm64 only) for this PR. |
3dbdecd
to
10964f9
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
...s/designsystem/src/main/kotlin/io/element/android/libraries/designsystem/components/Bloom.kt
Outdated
Show resolved
Hide resolved
It does work, but only for avatars with very vivid colors, and it's quite subtle even in that case (try some vivid primary color like red/green/blue). We probably need to improve this, but it's the result we have with the current opacity and blend modes. This is also the case in iOS: element-hq/element-x-ios#1630.
That's the expected behavior: the bloom gets clipped and a subtle divider is added to the app bar. It's probably quite hard to see there because it's also gray-ish though 😅 . |
Codecov ReportPatch coverage:
Additional details and impacted files@@ Coverage Diff @@
## develop #1253 +/- ##
===========================================
+ Coverage 57.43% 57.65% +0.21%
===========================================
Files 1084 1086 +2
Lines 28241 28538 +297
Branches 5806 5851 +45
===========================================
+ Hits 16221 16454 +233
- Misses 9478 9515 +37
- Partials 2542 2569 +27
☔ View full report in Codecov by Sentry. |
Kudos, SonarCloud Quality Gate passed! 0 Bugs No Coverage information |
For bloom in light mode: I have found that not using I think most of the users are using light mode, so I do not get the point to merge this if the effect is not visible to them. |
We need to finish it for the polishing epic, and the deadline for this task is today, so there's not much we can do right now. Either the design team decides to use other blend modes or opacity values or we need to merge it as is. In the future we can try some other strategies as extracting key colors or applying some other blurring algorithms. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK
@bmarty It's a known limitation of the current implementation that for avatars that have low chroma content (either being mostly grayscale, or too pale/pastel colors) the bloom effect is too subtle or lost in light mode. It's currently implemented with a mix of layer blending modes and opacity values. This however has some limitations that would only be properly addressed by image analysis libraries like Android's Palette API, that is, extracting dominant colors and building a gradient from scratch, but due to time constraints we decided to go ahead with the simpler cross-platform implementation (layer blending modes). /cc @nadonomy |
+1, also keen to spike color thief (& native mobile implementations) when we have time: https://lokeshdhakar.com/projects/color-thief/ |
Type of change
Content
bloom
andavatarBloom
modifiers which work on API 29+.ConnectivityIndicatorContainer
component to be able to draw the bloom effect under the statusbar.avatarColors
to the initial avatar for the current user in the top-left corner of the room list screen.Motivation and context
Closes #1217.
Screenshots / GIFs
Included in the PR. The bloom is not that visible for initial avatars in light mode though, some times it's completely transparent. We might need to iterate on this.
Tested devices
Checklist