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

[Impeller] replaced playground macros with functions #50602

Merged
merged 3 commits into from
Feb 13, 2024

Conversation

gaaclarke
Copy link
Member

testing: Refactor only

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 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.

If you need help, consider asking for advice on the #hackers-new channel on Discord.

Color::White(), Color::White());
static PlaygroundPoint point_a(Point(50, 50), 30, Color::White());
static PlaygroundPoint point_b(Point(300, 200), 30, Color::White());
auto [a, b] = DrawPlaygroundLine(point_a, point_b);
Copy link
Member

Choose a reason for hiding this comment

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

If the intention behind this is change is make tests where these are used golden-able, would just changing the macros to forward defaults when IMPELLER_GOLDEN_TESTS is enabled work?

Copy link
Member Author

Choose a reason for hiding this comment

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

The intention is to get this code compliant with the c++ style guide: https://google.github.io/styleguide/cppguide.html#Preprocessor_Macros

My next PR will get these running in golden image tests. This is just cleanup. That's an interesting idea with IMPELLER_GOLDEN_TESTS. I will probably avoid latching more decisions on preprocessor macros and instead rely on AiksPlayground vs GoldenPlayground.

Copy link
Member Author

Choose a reason for hiding this comment

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

Oh cool, I was able to just gate the logic on the presence of an ImGui context. I'll include you in the follow up PR.

Copy link
Member Author

Choose a reason for hiding this comment

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

Here it is: #50606

Copy link
Member

Choose a reason for hiding this comment

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

Makes sense... At some point we could change this to use ImGui's actual identity/storage system so that they can remain as one function without the statics. Added an issue here.

Copy link
Contributor

@matanlurey matanlurey left a comment

Choose a reason for hiding this comment

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

Thank you so much for doing this! LGTM!

@gaaclarke gaaclarke merged commit 7814e5d into flutter:main Feb 13, 2024
26 checks passed
@bdero
Copy link
Member

bdero commented Feb 13, 2024

I was late to the party, but this was also LGTM ;)

engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Feb 13, 2024
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Feb 13, 2024
auto-submit bot pushed a commit to flutter/flutter that referenced this pull request Feb 13, 2024
…143393)

flutter/engine@8742f1c...0091a49

2024-02-13 jonahwilliams@google.com [Impeller] remove denylist entry. (flutter/engine#50605)
2024-02-13 zanderso@users.noreply.github.com [engine_build_configs] Use dart:ffi Abi to determine the host cpu (flutter/engine#50604)
2024-02-13 jason-simmons@users.noreply.github.com Update embedder support for Impeller/OpenGL to load some missing shaders and configure a depth attachment (flutter/engine#50416)
2024-02-13 skia-flutter-autoroll@skia.org Roll Skia from 30bba7419898 to f7e3a5395fe1 (1 revision) (flutter/engine#50603)
2024-02-13 30870216+gaaclarke@users.noreply.github.com [Impeller] replaced playground macros with functions (flutter/engine#50602)
2024-02-13 matanlurey@users.noreply.github.com Allow deprecated members from the Dart SDK to roll in. (flutter/engine#50575)
2024-02-13 68449066+zijiehe-google-com@users.noreply.github.com [Fuchsia] Run tests with test arguments (flutter/engine#50478)

If this roll has caused a breakage, revert this CL and stop the roller
using the controls here:
https://autoroll.skia.org/r/flutter-engine-flutter-autoroll
Please CC jsimmons@google.com,rmistry@google.com,zra@google.com on the revert to ensure that a human
is aware of the problem.

To file a bug in Flutter: https://github.com/flutter/flutter/issues/new/choose

To report a problem with the AutoRoller itself, please file a bug:
https://issues.skia.org/issues/new?component=1389291&template=1850622

Documentation for the AutoRoller is here:
https://skia.googlesource.com/buildbot/+doc/main/autoroll/README.md
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants