Skip to content

Import flutter_shaders package - #12583

Open
tarrinneal wants to merge 67 commits into
flutter:mainfrom
tarrinneal:import-flutter-shaders
Open

Import flutter_shaders package#12583
tarrinneal wants to merge 67 commits into
flutter:mainfrom
tarrinneal:import-flutter-shaders

Conversation

@tarrinneal

Copy link
Copy Markdown
Contributor

⚠️ DO NOT LAND AS NORMAL — DO NOT ADD autosubmit ⚠️

Reviewers: Please DO NOT add the autosubmit label to this PR. This is an initial repository migration and must be landed manually following the repository transfer process.

Description

This PR imports flutter_shaders into flutter/packages from its original repository:
Source Repository: https://github.com/jonahwilliams/flutter_shaders
The commit history from the original repository was preserved via git subtree/merge up to commit 2802b9e47b.

Reviewer Guide

Note for Reviewers: Please review only the commits authored on top of the initial import (6eb14ba6e2..HEAD), as the prior commits represent the imported history


Summary of Changes Relative to Initial Import

  1. Repository Integration & Metadata:
    • Updated pubspec.yaml with the flutter/packages mono-repo repository and issue_tracker URLs, and added topics: [shaders, graphics].
    • Added p: flutter_shaders to .github/labeler.yml.
    • Added flutter_shaders to the packages table in the root README.md.
    • Standardized BSD license headers across all files to match repository conventions (// Copyright 2013 The Flutter Authors).
  2. SDK Constraints & Dependencies:
    • Bumped minimum environment SDK constraints in both pubspec.yaml and example/pubspec.yaml to Flutter >=3.38.0 / Dart ^3.10.0.
  3. Dart 3 Base Class Compatibility & Test Fixes:
    • set_uniforms: Extracted FloatUniformsSetter as an abstract base class/interface so uniform conversion logic can be tested and extended without implementing ui.FragmentShader (which is a base class in Dart 3 and cannot be implemented outside dart:ui). Kept UniformsSetter non-nullable and backwards-compatible.
    • animated_sampler_test: Updated test callbacks from 4 arguments to 3 arguments (expectAsync3) to match the AnimatedSamplerBuilder signature (the offset parameter was removed in 0.0.6).
    • Corrected test expectations in setColors w/ premultiply where a zero-alpha color was expected to have non-zero alpha.
  4. Style, Lints, & Docs:
    • Added public API documentation across UniformsSetter and ShaderInkFeatureFactory.
    • Cleaned up lints (prefer_foreach, control body newlines) and formatted all files with dart format.
  5. Release / Publish Prep:
    • Bumped version to 0.1.4 in pubspec.yaml.
    • Added a CHANGELOG.md entry describing the package transfer and changes.

Verification

  • dart run script/tool/bin/flutter_plugin_tools.dart validate --packages flutter_shaders passes.
  • dart run script/tool/bin/flutter_plugin_tools.dart analyze --packages flutter_shaders passes with 0 issues.
  • dart run script/tool/bin/flutter_plugin_tools.dart dart-test --packages flutter_shaders passes (all unit and example tests pass).
  • dart run script/tool/bin/flutter_plugin_tools.dart license-check passes.

Reminder: DO NOT ADD autosubmit.

jonahwilliams and others added 30 commits November 9, 2022 10:39
switch to flutter runtime effect
Fix docs for fragment shader breaking changes
Add support for configuring inkwell shader
…y frame

Call `AnimatedSampler.builder` from `Layer.addToScene` to update every frame
fix offset transforming animated sampler children off the screen
@flutter-dashboard flutter-dashboard Bot added the CICD Run CI/CD label Aug 25, 2026
@google-cla

google-cla Bot commented Aug 25, 2026

Copy link
Copy Markdown

Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

View this failed invocation of the CLA check for more information.

For the most up to date status, view the checks section at the bottom of the pull request.

@gemini-code-assist gemini-code-assist Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Code Review

This pull request imports the flutter_shaders package into the repository, providing utilities like AnimatedSampler, ShaderInkFeature, SetUniforms, and ShaderBuilder to simplify working with the FragmentProgram API. Feedback on the implementation highlights a native memory leak in AnimatedSampler due to an undisposed ui.Scene, an incorrect radius calculation in ShaderInkFeature that ignores the tap position, and an optimization opportunity in ShaderBuilder to prevent redundant concurrent asset loading.

Comment on lines +208 to +211
return builder.build().toImageSync(
(pixelRatio * bounds.width).ceil(),
(pixelRatio * bounds.height).ceil(),
);

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

high

The ui.Scene returned by builder.build() is a native resource that must be explicitly disposed to avoid memory leaks. Since _buildChildScene is called on every frame of an animation, this will cause a continuous native memory leak. Store the scene in a local variable and dispose it in a finally block.

Suggested change
return builder.build().toImageSync(
(pixelRatio * bounds.width).ceil(),
(pixelRatio * bounds.height).ceil(),
);
final ui.Scene scene = builder.build();
try {
return scene.toImageSync(
(pixelRatio * bounds.width).ceil(),
(pixelRatio * bounds.height).ceil(),
);
} finally {
scene.dispose();
}

Comment on lines +342 to +345
final Size size = rectCallback != null ? rectCallback().size : referenceBox.size;
final double d1 = size.bottomRight(Offset.zero).distance;
final double d2 = (size.topRight(Offset.zero) - size.bottomLeft(Offset.zero)).distance;
return math.max(d1, d2) / 2.0;

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

high

The _getTargetRadius function calculates the target radius as half of the diagonal of the bounding box, completely ignoring the tap position. If a user taps near a corner or edge, the splash will not expand enough to cover the entire widget. To ensure the splash covers the entire widget, the target radius should be the maximum distance from the tap position to the four corners of the bounding box.

  final Size size = rectCallback != null ? rectCallback().size : referenceBox.size;
  final double d1 = position.distance;
  final double d2 = (position - size.topRight(Offset.zero)).distance;
  final double d3 = (position - size.bottomLeft(Offset.zero)).distance;
  final double d4 = (position - size.bottomRight(Offset.zero)).distance;
  return math.max(math.max(d1, d2), math.max(d3, d4));

Comment on lines +80 to +118
static final Map<String, ui.FragmentProgram> _shaderCache = <String, ui.FragmentProgram>{};

@override
void initState() {
super.initState();
_loadShader(widget.assetKey);
}

@override
void didUpdateWidget(covariant ShaderBuilder oldWidget) {
super.didUpdateWidget(oldWidget);
if (oldWidget.assetKey != widget.assetKey) {
_loadShader(widget.assetKey);
}
}

void _loadShader(String assetKey) {
if (_shaderCache.containsKey(assetKey)) {
program = _shaderCache[assetKey];
shader = program!.fragmentShader();
return;
}

ui.FragmentProgram.fromAsset(assetKey).then(
(ui.FragmentProgram program) {
if (!mounted) {
return;
}
setState(() {
this.program = program;
shader = program.fragmentShader();
_shaderCache[assetKey] = program;
});
},
onError: (Object error, StackTrace stackTrace) {
FlutterError.reportError(FlutterErrorDetails(exception: error, stack: stackTrace));
},
);
}

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

medium

If multiple ShaderBuilder widgets with the same assetKey are initialized concurrently before the first one finishes loading, they will all trigger redundant ui.FragmentProgram.fromAsset calls. We can optimize this by keeping a map of pending futures to share the loading process, while still caching the completed FragmentProgram for synchronous access on subsequent builds.

  static final Map<String, ui.FragmentProgram> _shaderCache = <String, ui.FragmentProgram>{};
  static final Map<String, Future<ui.FragmentProgram>> _pendingLoads = <String, Future<ui.FragmentProgram>>{};

  @override
  void initState() {
    super.initState();
    _loadShader(widget.assetKey);
  }

  @override
  void didUpdateWidget(covariant ShaderBuilder oldWidget) {
    super.didUpdateWidget(oldWidget);
    if (oldWidget.assetKey != widget.assetKey) {
      _loadShader(widget.assetKey);
    }
  }

  void _loadShader(String assetKey) {
    if (_shaderCache.containsKey(assetKey)) {
      program = _shaderCache[assetKey];
      shader = program!.fragmentShader();
      return;
    }

    final Future<ui.FragmentProgram> future = _pendingLoads.putIfAbsent(
      assetKey,
      () => ui.FragmentProgram.fromAsset(assetKey),
    );

    future.then(
      (ui.FragmentProgram loadedProgram) {
        _shaderCache[assetKey] = loadedProgram;
        _pendingLoads.remove(assetKey);
        if (!mounted) {
          return;
        }
        setState(() {
          program = loadedProgram;
          shader = loadedProgram.fragmentShader();
        });
      },
      onError: (Object error, StackTrace stackTrace) {
        _pendingLoads.remove(assetKey);
        FlutterError.reportError(FlutterErrorDetails(exception: error, stack: stackTrace));
      },
    );
  }

@tarrinneal

Copy link
Copy Markdown
Contributor Author

The failing golden test was generated with material 2, the material 3 version is not very interesting. I can swap them, but it might be better to find something better maybe?

@tarrinneal

Copy link
Copy Markdown
Contributor Author
shaders inkwell_testImage

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

Labels

CICD Run CI/CD

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants