Skip to content
This repository was archived by the owner on Feb 25, 2025. It is now read-only.

[Impeller] Implement tile mode for linear gradient and radial gradient#35087

Merged
auto-submit[bot] merged 8 commits intoflutter:mainfrom
ColdPaleLight:gradient_tile_mode
Aug 4, 2022
Merged

[Impeller] Implement tile mode for linear gradient and radial gradient#35087
auto-submit[bot] merged 8 commits intoflutter:mainfrom
ColdPaleLight:gradient_tile_mode

Conversation

@ColdPaleLight
Copy link
Member

@ColdPaleLight ColdPaleLight commented Aug 2, 2022 •
edited
Loading

fix flutter/flutter#108810

gu5D3BG2IC

cOcwnTXeym

Pre-launch Checklist

  • I read the [Contributor Guide] and followed the process outlined there for submitting PRs.
  • I read the [Tree Hygiene] wiki page, which explains my responsibilities.
  • I read and followed the [Flutter Style Guide] and the [C++, Objective-C, Java style guides].
  • I listed at least one issue that this PR fixes in the description above.
  • I added new tests to check the change I am making or feature I am adding, or Hixie said the PR is test-exempt. See [testing the engine] for instructions on
    writing and running engine tests.
  • I updated/added relevant documentation (doc comments with ///).
  • I signed the [CLA].
  • All existing and new tests are passing.

@ColdPaleLight ColdPaleLight marked this pull request as draft August 2, 2022 13:08
@ColdPaleLight ColdPaleLight marked this pull request as ready for review August 3, 2022 07:16
@ColdPaleLight ColdPaleLight changed the title [Impeller] Implement tile mode for gradient [Impeller] Implement tile mode for linear gradient and radial gradient Aug 3, 2022
bdero
bdero reviewed Aug 3, 2022
View reviewed changes
Copy link
Contributor

@bdero bdero left a comment

Choose a reason for hiding this comment

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

Overall looking good, thanks! Just some comments about where things should live.

impeller/geometry/tile_mode.h Outdated

// An enum to define how to repeat, fold, or omit colors outside of the
// typically defined range of the source of the colors (such as the
// bounds of an image or the defining geoetry of a gradient).
Copy link
Contributor

@bdero bdero Aug 3, 2022

Choose a reason for hiding this comment

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

Please use /// for docstring comments.

Copy link
Member Author

@ColdPaleLight ColdPaleLight Aug 4, 2022

Choose a reason for hiding this comment

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

Done.

impeller/entity/shaders/tile_mode_utils.glsl Outdated
const float kTileModeMirror = 2;
const float kTileModeDecal = 3;

float GetInterpolantValue(float t, float tile_mode) {
Copy link
Contributor

@bdero bdero Aug 3, 2022

Choose a reason for hiding this comment

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

Please add a docstring that describes the intended domain and range of this function. Looking at it, the domain appears to be any value and the range is [0 to 1].

Copy link
Member Author

@ColdPaleLight ColdPaleLight Aug 4, 2022

Choose a reason for hiding this comment

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

Done.

impeller/entity/shaders/tile_mode_utils.glsl Outdated
@@ -0,0 +1,21 @@
// Copyright 2013 The Flutter Authors. All rights reserved.
Copy link
Contributor

@bdero bdero Aug 3, 2022

Choose a reason for hiding this comment

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

This seems like something that would best be placed in the shader library.

Although it happens to not be being used for a texture in this case. It seems like impeller/compiler/shader_lib/texture.glsl would be an appropriate home for this given its UV mapping behavior. Perhaps call it IPTileTextureCoords?

Copy link
Member Author

@ColdPaleLight ColdPaleLight Aug 4, 2022

Choose a reason for hiding this comment

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

Done.

impeller/geometry/tile_mode.h Outdated
@@ -0,0 +1,31 @@
// Copyright 2013 The Flutter Authors. All rights reserved.
Copy link
Contributor

@bdero bdero Aug 3, 2022

Choose a reason for hiding this comment

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

geometry is primarily used for math utilities. Please place this enum in the entity framework. Perhaps in the Entity declaration next to the BlendMode enum class.

Copy link
Member Author

@ColdPaleLight ColdPaleLight Aug 4, 2022

Choose a reason for hiding this comment

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

Done.

impeller/entity/shaders/tile_mode_utils.glsl Outdated
// Use of this source code is governed by a BSD-style license that can be
// found in the LICENSE file.

const float kTileModeClamp = 0;
Copy link
Contributor

@bdero bdero Aug 3, 2022

Choose a reason for hiding this comment

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

Please add a comment nearby indicating that these values must correspond to the order of the items in the Entity::TileMode enum class.

Copy link
Member Author

@ColdPaleLight ColdPaleLight Aug 4, 2022

Choose a reason for hiding this comment

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

Done.

@ColdPaleLight ColdPaleLight requested a review from bdero August 4, 2022 03:41
bdero
bdero approved these changes Aug 4, 2022
View reviewed changes
Copy link
Contributor

@bdero bdero left a comment

Choose a reason for hiding this comment

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

lgtm!

ColdPaleLight and others added 2 commits August 4, 2022 16:31
@ColdPaleLight @bdero
Update impeller/entity/entity.h
a407495 
Co-authored-by: Brandon DeRosier <[email protected]>
@ColdPaleLight @bdero
Update impeller/entity/entity.h
00642c6 
Co-authored-by: Brandon DeRosier <[email protected]>
@ColdPaleLight ColdPaleLight added autosubmit Merge PR when tree becomes green via auto submit App and removed autosubmit Merge PR when tree becomes green via auto submit App labels Aug 4, 2022
@auto-submit auto-submit bot merged commit 02c1ca7 into flutter:main Aug 4, 2022
@engine-flutter-autoroll engine-flutter-autoroll mentioned this pull request Aug 4, 2022
Merged
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Aug 4, 2022
@engine-flutter-autoroll
02c1ca7 [Impeller] Implement tile mode for linear gradient and radial…
5d7229b 
… gradient (flutter/engine#35087)
betrevisan pushed a commit to betrevisan/engine that referenced this pull request Aug 5, 2022
@ColdPaleLight @betrevisan
[Impeller] Implement tile mode for linear gradient and radial gradient (
8182566 
flutter#35087)
emilyabest pushed a commit to emilyabest/engine that referenced this pull request Aug 12, 2022
@ColdPaleLight @emilyabest
[Impeller] Implement tile mode for linear gradient and radial gradient (
e2ca668 
flutter#35087)
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

@chinmaygarde chinmaygarde Awaiting requested review from chinmaygarde

1 more reviewer

@bdero bdero bdero approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

@ColdPaleLight ColdPaleLight

Labels

autosubmit Merge PR when tree becomes green via auto submit App e: impeller

Projects

No open projects
Impeller
Archived in project

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

[Impeller] Implement tile mode for gradient

3 participants

@ColdPaleLight @bdero @chinmaygarde