-
Notifications
You must be signed in to change notification settings - Fork 6k
Conversation
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.
Some nits about the CMake rules but LGTM otherwise.
examples/glfw/CMakeLists.txt
Outdated
############################################################ | ||
# GLFW | ||
############################################################ | ||
find_path(GLFW_INCLUDE_PATH "glfw3.h" /usr/local/Cellar/glfw/3.3/include/GLFW/) |
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.
Please use find_package
to use pkg-config so this is cross platform.
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.
In order for that to work I think you need something like this: https://github.com/Kitware/CMake/blob/master/Modules/FindFLTK.cmake . They don't have one for GLFW.
Also brew doesn't install a .pc file for GLFW so I don't think pkg-config can work for the mac build.
I added extra paths that should catch standard installs on linux. I couldn't test it right now because unfortunately I'm locked out of my linux box. People can always edit the variables in ccmake.
# This is assuming you've built a local version of the Flutter Engine. If you | ||
# downloaded yours is from the internet you'll have to change this. | ||
include_directories(${CMAKE_SOURCE_DIR}/../../shell/platform/embedder) | ||
find_library(FLUTTER_LIB flutter_engine PATHS ${CMAKE_SOURCE_DIR}/../../../out/host_debug_unopt) |
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.
You can make the path a CMake option. Then its fine to default it to host_debug_unopt
.
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.
FLUTTER_LIB is already a variable. If I pull the engine name out into a different variable, editing it doesn't update FLUTTER_LIB on subsequent cmake calls. People should just edit FLUTTER_LIB.
examples/glfw/FlutterEmbedderGLFW.cc
Outdated
|
||
// If your flutter app is drawing at the wrong size you may need to adjust your | ||
// pixel ratio. | ||
static const double kPixelRatio = 2.0; |
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.
See this discussion on inferring the pixel ratio from GLFW calls. https://www.glfw.org/docs/latest/news.html#news_30_hidpi. You don't need to hardcode this.
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.
Done
examples/glfw/FlutterEmbedderGLFW.cc
Outdated
static const size_t kInitialWindowWidth = 800; | ||
static const size_t kInitialWindowHeight = 600; | ||
|
||
static_assert(FLUTTER_ENGINE_VERSION == 1, ""); |
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.
static_assert(FLUTTER_ENGINE_VERSION == 1,
"This Flutter Embedder was authored against the stable Flutter "
"API at version 1. There has been a serious breakage in the "
"API. Please read the ChangeLog and take appropriate action "
"before updating this assertion");
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.
Done
git@github.com:flutter/engine.git/compare/3a445edda924...5b31822 git log 3a445ed..5b31822 --no-merges --oneline 2019-10-08 bkonyi@google.com Roll src/third_party/dart aece1c1e92..f4a72bfc64 (4 commits) 2019-10-08 30870216+gaaclarke@users.noreply.github.com Added an embedder example (flutter/engine#12808) 2019-10-08 sebastian.roth@gmail.com Fix typo on channel buffer debug output. (flutter/engine#12960) 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 chinmaygarde@google.com on the revert to ensure that a human is aware of the problem. To report a problem with the AutoRoller itself, please file a bug: https://bugs.chromium.org/p/skia/issues/entry?template=Autoroller+Bug Documentation for the AutoRoller is here: https://skia.googlesource.com/buildbot/+/master/autoroll/README.md
git@github.com:flutter/engine.git/compare/3a445edda924...5b31822 git log 3a445ed..5b31822 --no-merges --oneline 2019-10-08 bkonyi@google.com Roll src/third_party/dart aece1c1e92..f4a72bfc64 (4 commits) 2019-10-08 30870216+gaaclarke@users.noreply.github.com Added an embedder example (flutter/engine#12808) 2019-10-08 sebastian.roth@gmail.com Fix typo on channel buffer debug output. (flutter/engine#12960) 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 chinmaygarde@google.com on the revert to ensure that a human is aware of the problem. To report a problem with the AutoRoller itself, please file a bug: https://bugs.chromium.org/p/skia/issues/entry?template=Autoroller+Bug Documentation for the AutoRoller is here: https://skia.googlesource.com/buildbot/+/master/autoroll/README.md
Relevant issue: flutter/flutter#41948
This PR transforms Chinmay's gist ( https://gist.github.com/chinmaygarde/8abf44921f7d87f6da7bf026267c4792 ) into a buildable and runnable example. It uses CMake which is a popular build system and has a shell script that will automate all the steps (including the Flutter steps) so that clients can see how everything works together. The intention is to add a LUCI script that makes sure that we don't break this example.