[Impeller] add --enable-impeller-3d flag to support scene experimentation #37990
Conversation
|
It looks like this pull request may not have tests. Please make sure to add tests before merging. If you need an exemption to this rule, contact Hixie on the #hackers channel in Chat (don't just cc him here, he won't see it! He's on Discord!). If you are not sure if you need tests, consider this rule of thumb: the purpose of a test is to make sure someone doesn't accidentally revert the fix. Ask yourself, is there anything in your PR that you feel it is important we not accidentally revert back to how it was before your fix? Reviewers: Read the Tree Hygiene page and make sure this patch meets those guidelines before LGTMing. |
|
I could not find another way to do this without copying compositing. I could re-arrange compositing.dart to separate scenebuilder so that there is less duplicated code though |
lib/ui/dart_ui.gni
Outdated
|
|
||
| dart_ui_path = "//flutter/lib/ui/ui.dart" | ||
| if (!impeller_enable_3d) { |
There was a problem hiding this comment.
You can factor out the common lists and use + to add the ones that are different.
_common_dart_ui_files = [
...
]
if () {
dart_ui_files = _common_dart_ui_files + [
]
} else {
}Also, think positive:
if (impeller_enable_3d) {
} else {
}
lib/ui/dart_ui.gni
Outdated
| dart_ui_files = [ | ||
| "//flutter/lib/ui/annotations.dart", | ||
| "//flutter/lib/ui/channel_buffers.dart", | ||
| "//flutter/lib/ui/compositing.dart", |
There was a problem hiding this comment.
Does this need both compositing.dart and experiments/compositing_32.dart?
| @FfiNative<Void Function(Pointer<Void>, Double, Double, Double, Double, Int64)>('SceneBuilder::addPlatformView', isLeaf: true) | ||
| external void _addPlatformView(double dx, double dy, double width, double height, int viewId); | ||
|
|
||
| void addModelLayer( |
There was a problem hiding this comment.
Could experimental/compositing_3d.dart just contain an extension method on SceneBuilder, and leave the rest of this file in compositing.dart? (An exception to the ban on extension methods seems less bad than duplicating all the code, in this case...)
There was a problem hiding this comment.
extension method no, but I could try to make this a static method. Different API on the Flutter side but that doesn't really matter for this. Good call
There was a problem hiding this comment.
oh wait, no that won't work. Needs to be an instance method
There was a problem hiding this comment.
what if we leave the method here, but make it un-reachable without a separate static method?
|
Updated to use static method and everything seems to work. Made the current implementation just append a transform layer to verify it works correctly |
|
I removed the changes to dart_ui.gni since that is only for constructing the sdk structure for analysis |
|
Gold has detected about 1 new digest(s) on patchset 8. |
|
Golden file changes have been found for this pull request. Click here to view and triage (e.g. because this is an intentional change). If you are still iterating on this change and are not ready to resolve the images on the Flutter Gold dashboard, consider marking this PR as a draft pull request above. You will still be able to view image results on the dashboard, commenting will be silenced, and the check will not try to resolve itself until marked ready for review. |
jonahwilliams
force-pushed
the
fix_perspective
branch
from
dbbbb8b
to
a4c3b41
Compare
jonahwilliams
added
autosubmit
…116447) * bebc12fd0 Roll Skia from 28c1bbab82b8 to 6fdf7181e374 (16 revisions) (flutter/engine#38046) * 2c1e5efae [Impeller] add --enable-impeller-3d flag to support scene experimentation (flutter/engine#37990) * 8e11659a8 [canvaskit] Fix Shader program tests (flutter/engine#37644) * 850c4ad68 [canvaskit] Fix Surface test (flutter/engine#37636) * f7df812d2 save (flutter/engine#37107)
…lutter#116447) * bebc12fd0 Roll Skia from 28c1bbab82b8 to 6fdf7181e374 (16 revisions) (flutter/engine#38046) * 2c1e5efae [Impeller] add --enable-impeller-3d flag to support scene experimentation (flutter/engine#37990) * 8e11659a8 [canvaskit] Fix Shader program tests (flutter/engine#37644) * 850c4ad68 [canvaskit] Fix Surface test (flutter/engine#37636) * f7df812d2 save (flutter/engine#37107)
…lutter#116447) * bebc12fd0 Roll Skia from 28c1bbab82b8 to 6fdf7181e374 (16 revisions) (flutter/engine#38046) * 2c1e5efae [Impeller] add --enable-impeller-3d flag to support scene experimentation (flutter/engine#37990) * 8e11659a8 [canvaskit] Fix Shader program tests (flutter/engine#37644) * 850c4ad68 [canvaskit] Fix Surface test (flutter/engine#37636) * f7df812d2 save (flutter/engine#37107)
…lutter#116447) * bebc12fd0 Roll Skia from 28c1bbab82b8 to 6fdf7181e374 (16 revisions) (flutter/engine#38046) * 2c1e5efae [Impeller] add --enable-impeller-3d flag to support scene experimentation (flutter/engine#37990) * 8e11659a8 [canvaskit] Fix Shader program tests (flutter/engine#37644) * 850c4ad68 [canvaskit] Fix Surface test (flutter/engine#37636) * f7df812d2 save (flutter/engine#37107)
Off by default. Analysis doesn't quite work.