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

ShaderBrush#createShader state reads do not invalidate Text composable #4903

Closed
halilozercan opened this issue May 30, 2024 · 5 comments · Fixed by JetBrains/compose-multiplatform-core#1395
Assignees
Labels
bug Something isn't working rendering Low level rendering skia text

Comments

@halilozercan
Copy link

Describe the bug
Brush instances created using the abstract ShaderBrush class, does not animate when applied on Text.

Affected platforms
Only tested on;

  • Desktop (Windows, Linux, macOS)

Versions

  • Libraries:
    • Compose Multiplatform version: 1.6.10

To Reproduce
Steps to reproduce the behavior:

  1. Run this code snippet:

https://cs.android.com/androidx/platform/frameworks/support/+/androidx-main:compose/foundation/foundation/integration-tests/foundation-demos/src/main/java/androidx/compose/foundation/demos/text/BrushDemo.kt;l=204-233

fun AnimatedBrush() {
    val infiniteTransition = rememberInfiniteTransition()
    val radius by infiniteTransition.animateFloat(
        initialValue = 100f,
        targetValue = 300f,
        animationSpec = infiniteRepeatable(
            animation = tween(durationMillis = 1000, easing = LinearEasing),
            repeatMode = RepeatMode.Reverse
        )
    )
    val brush = remember {
        // postpone the state read to shader creation time which happens during draw.
        ShaderBrush { size ->
            RadialGradientShader(
                center = size.center,
                radius = radius,
                colors = RainbowColors,
                colorStops = RainbowStops,
                tileMode = TileMode.Mirror
            )
        }
    }
    Text(
        text = loremIpsum(wordCount = 29),
        style = TextStyle(
            brush = brush,
            fontSize = 30.sp
        )
    )
}

Expected behavior
Expecting text to have an animated brush coloring.

Additional context

Android handles this by creating the shader in a derivedStateOf. This way any state read that happens in createShader invalidates the derived state and only re-runs the draw phase. Otherwise devs need to change the brush instance for every animation frame which can get very costly due to unnecessary recomposition.

https://cs.android.com/androidx/platform/frameworks/support/+/androidx-main:compose/ui/ui-text/src/androidMain/kotlin/androidx/compose/ui/text/platform/AndroidTextPaint.android.kt;l=111-143

@halilozercan halilozercan added bug Something isn't working submitted labels May 30, 2024
@m-sasha
Copy link
Member

m-sasha commented May 30, 2024

Looks like we need to apply (androidx/androidx@1631196) to SkiaTextPaint (where it says // Copied from AndroidTextPaint.) as part of merging in 1.7 (that change is in AndroidTextPaint.android.kt of the integration branch).

@igordmn

@MatkovIvan
Copy link
Member

Unfortunately the fix from mentioned commit won't work for CMP because unlike Android we do not control Paint state during paragraph redrawing - it's fulling inside C++/skia. So to apply such changes, currently it requires full re-layout/re-creation of skia's paragraph. Even with a few layers of caching that we currently have, I doubt that it will animate smoothly. To properly do this, global changes in text rendering/layout are required.

@rock3r
Copy link
Contributor

rock3r commented Jun 7, 2024

😭

@MatkovIvan
Copy link
Member

Adopted it with re-layouting in JetBrains/compose-multiplatform-core#1395

MatkovIvan added a commit to JetBrains/compose-multiplatform-core that referenced this issue Jun 10, 2024
- Adopt
androidx@1631196
change
- Use `paragraph.updateForegroundPaint` instead of full rebuild where
possible

Fixes JetBrains/compose-multiplatform#4903

## Testing

Added `Android TextBrushDemo` in mpp demo

<img width="1029" alt="Screenshot 2024-06-07 at 19 21 39"
src="https://github.com/JetBrains/compose-multiplatform-core/assets/1836384/97d17949-e743-4774-ae55-cab664412091">

This should be tested by QA

## Release Notes

### Fixes - Multiple Platforms

- Fix text `brush` animation and optimized updating some visual text
properties (applying time is reduced up to 40%)
@okushnikov
Copy link
Collaborator

Please check the following ticket on YouTrack for follow-ups to this issue. GitHub issues will be closed in the coming weeks.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working rendering Low level rendering skia text
Projects
None yet
Development

Successfully merging a pull request may close this issue.

7 participants