Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1994213002:
cc: Update effect_tree backface visibility when transform is updated. (Closed)
Descriptioncc: Update effect_tree backface visibility when transform is updated.
We should update effect_tree as well in UpdatePropertyTrees when an
animation cause a change in transform tree. Failing to update it may
cause draw_property_utils incorrectly skip layers.
This CL forces the update and added a related unit test.
BUG=610280
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel
Committed: https://crrev.com/59dd7daef0b5f11d6f8dd6dd5141967ab0deb0b4
Cr-Commit-Position: refs/heads/master@{#394846}
Patch Set 1 #
Total comments: 12
Patch Set 2 : Resolve comments in unit test #
Messages
Total messages: 19 (6 generated)
Description was changed from ========== cc: Update effect_tree backface visibility when transform is updated. BUG=610280 ========== to ========== cc: Update effect_tree backface visibility when transform is updated. BUG=610280 CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel ==========
Description was changed from ========== cc: Update effect_tree backface visibility when transform is updated. BUG=610280 CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel ========== to ========== cc: Update effect_tree backface visibility when transform is updated. We should update effect_tree as well in UpdatePropertyTrees when an animation cause a change in transform tree. Failing to update it may cause draw_property_utils incorrectly skip layers. This CL forces the update and added a related unit test. BUG=610280 CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel ==========
sunxd@chromium.org changed reviewers: + ajuma@chromium.org, jaydasika@chromium.org, petermayo@chromium.org
https://codereview.chromium.org/1994213002/diff/1/cc/trees/draw_property_util... File cc/trees/draw_property_utils.cc (right): https://codereview.chromium.org/1994213002/diff/1/cc/trees/draw_property_util... cc/trees/draw_property_utils.cc:793: property_trees->effect_tree.set_needs_update(true); I think it will be better if we call set_needs_update on effect tree only when its actually needed. One possible way to do that would be to check in OnTransformAnimated if bf-visibility can change and set_needs_update on effect tree to true there itself if it can change. (For instance, if the old and the new transforms in OnTransformAnimated are both translations, then bf-visibility cannot change. On the other hand, if one of them is rotation, it can change). WDYT?
Just some nits about the test. https://codereview.chromium.org/1994213002/diff/1/cc/trees/layer_tree_host_co... File cc/trees/layer_tree_host_common_unittest.cc (right): https://codereview.chromium.org/1994213002/diff/1/cc/trees/layer_tree_host_co... cc/trees/layer_tree_host_common_unittest.cc:6401: gfx::Transform rotateAboutY; nit: rotate_about_y https://codereview.chromium.org/1994213002/diff/1/cc/trees/layer_tree_host_co... cc/trees/layer_tree_host_common_unittest.cc:6423: render_surface2->test_properties()->double_sided = false; SetLayerPropertiesForTesting already takes arguments (flatten_transform and is_3d_sorted) equivalent to should_flatten_transform and whether the layer has sorting context id 1, so these can be set as part of the SetLayerPropertiesForTesting calls above. https://codereview.chromium.org/1994213002/diff/1/cc/trees/layer_tree_host_co... cc/trees/layer_tree_host_common_unittest.cc:6437: root->layer_tree_impl()->property_trees(), true); Calling ExecuteCalculateDrawProperties instead is more similar to what happens outside of test code. https://codereview.chromium.org/1994213002/diff/1/cc/trees/layer_tree_host_co... cc/trees/layer_tree_host_common_unittest.cc:6445: root->layer_tree_impl()->property_trees(), true); Call ExcecuteCalculateDrawProperties here.
https://codereview.chromium.org/1994213002/diff/1/cc/trees/draw_property_util... File cc/trees/draw_property_utils.cc (right): https://codereview.chromium.org/1994213002/diff/1/cc/trees/draw_property_util... cc/trees/draw_property_utils.cc:793: property_trees->effect_tree.set_needs_update(true); On 2016/05/19 17:07:58, jaydasika wrote: > I think it will be better if we call set_needs_update on effect tree only when > its actually needed. One possible way to do that would be to check in > OnTransformAnimated if bf-visibility can change and set_needs_update on effect > tree to true there itself if it can change. (For instance, if the old and the > new transforms in OnTransformAnimated are both translations, then bf-visibility > cannot change. On the other hand, if one of them is rotation, it can change). > WDYT? That's a good point. If we did that, we'd also have to check this in other places where the transform can change (e.g., when updating newly-pushed property trees with animated transforms from the existing tree). Given that this CL needs to get merged to 51, I think it's safest to do the simple thing first and merge that, and then implement your suggestion in a follow-up.
https://codereview.chromium.org/1994213002/diff/1/cc/trees/draw_property_util... File cc/trees/draw_property_utils.cc (right): https://codereview.chromium.org/1994213002/diff/1/cc/trees/draw_property_util... cc/trees/draw_property_utils.cc:793: property_trees->effect_tree.set_needs_update(true); On 2016/05/19 17:07:58, jaydasika wrote: > I think it will be better if we call set_needs_update on effect tree only when > its actually needed. One possible way to do that would be to check in > OnTransformAnimated if bf-visibility can change and set_needs_update on effect > tree to true there itself if it can change. (For instance, if the old and the > new transforms in OnTransformAnimated are both translations, then bf-visibility > cannot change. On the other hand, if one of them is rotation, it can change). > WDYT? It is not true that a translation can never change the visible face. For example, when looking down a long tunnel with perspective, if you translate one side to the other you see the other face. This is similar to the error of using the z component to determine backface visibility.
https://codereview.chromium.org/1994213002/diff/1/cc/trees/draw_property_util... File cc/trees/draw_property_utils.cc (right): https://codereview.chromium.org/1994213002/diff/1/cc/trees/draw_property_util... cc/trees/draw_property_utils.cc:793: property_trees->effect_tree.set_needs_update(true); On 2016/05/19 17:18:14, ajuma wrote: > On 2016/05/19 17:07:58, jaydasika wrote: > > I think it will be better if we call set_needs_update on effect tree only when > > its actually needed. One possible way to do that would be to check in > > OnTransformAnimated if bf-visibility can change and set_needs_update on effect > > tree to true there itself if it can change. (For instance, if the old and the > > new transforms in OnTransformAnimated are both translations, then > bf-visibility > > cannot change. On the other hand, if one of them is rotation, it can change). > > WDYT? > > That's a good point. If we did that, we'd also have to check this in other > places where the transform can change (e.g., when updating newly-pushed property > trees with animated transforms from the existing tree). Given that this CL needs > to get merged to 51, I think it's safest to do the simple thing first and merge > that, and then implement your suggestion in a follow-up. Agreed, we should do the simple thing as this needs to be merged to 51. https://codereview.chromium.org/1994213002/diff/1/cc/trees/draw_property_util... cc/trees/draw_property_utils.cc:793: property_trees->effect_tree.set_needs_update(true); On 2016/05/19 17:33:20, Peter Mayo wrote: > On 2016/05/19 17:07:58, jaydasika wrote: > > I think it will be better if we call set_needs_update on effect tree only when > > its actually needed. One possible way to do that would be to check in > > OnTransformAnimated if bf-visibility can change and set_needs_update on effect > > tree to true there itself if it can change. (For instance, if the old and the > > new transforms in OnTransformAnimated are both translations, then > bf-visibility > > cannot change. On the other hand, if one of them is rotation, it can change). > > WDYT? > > It is not true that a translation can never change the visible face. For > example, when looking down a long tunnel with perspective, if you translate one > side to the other you see the other face. This is similar to the error of using > the z component to determine backface visibility. Ya, we will need to consider perspective also.
https://codereview.chromium.org/1994213002/diff/1/cc/trees/draw_property_util... File cc/trees/draw_property_utils.cc (right): https://codereview.chromium.org/1994213002/diff/1/cc/trees/draw_property_util... cc/trees/draw_property_utils.cc:793: property_trees->effect_tree.set_needs_update(true); On 2016/05/19 17:52:18, jaydasika wrote: > On 2016/05/19 17:18:14, ajuma wrote: > > On 2016/05/19 17:07:58, jaydasika wrote: > > > I think it will be better if we call set_needs_update on effect tree only > when > > > its actually needed. One possible way to do that would be to check in > > > OnTransformAnimated if bf-visibility can change and set_needs_update on > effect > > > tree to true there itself if it can change. (For instance, if the old and > the > > > new transforms in OnTransformAnimated are both translations, then > > bf-visibility > > > cannot change. On the other hand, if one of them is rotation, it can > change). > > > WDYT? > > > > That's a good point. If we did that, we'd also have to check this in other > > places where the transform can change (e.g., when updating newly-pushed > property > > trees with animated transforms from the existing tree). Given that this CL > needs > > to get merged to 51, I think it's safest to do the simple thing first and > merge > > that, and then implement your suggestion in a follow-up. > > Agreed, we should do the simple thing as this needs to be merged to 51. Yes, we can discuss about that after this CL. https://codereview.chromium.org/1994213002/diff/1/cc/trees/layer_tree_host_co... File cc/trees/layer_tree_host_common_unittest.cc (right): https://codereview.chromium.org/1994213002/diff/1/cc/trees/layer_tree_host_co... cc/trees/layer_tree_host_common_unittest.cc:6423: render_surface2->test_properties()->double_sided = false; On 2016/05/19 17:13:09, ajuma wrote: > SetLayerPropertiesForTesting already takes arguments (flatten_transform and > is_3d_sorted) equivalent to should_flatten_transform and whether the layer has > sorting context id 1, so these can be set as part of the > SetLayerPropertiesForTesting calls above. Done. https://codereview.chromium.org/1994213002/diff/1/cc/trees/layer_tree_host_co... cc/trees/layer_tree_host_common_unittest.cc:6437: root->layer_tree_impl()->property_trees(), true); On 2016/05/19 17:13:10, ajuma wrote: > Calling ExecuteCalculateDrawProperties instead is more similar to what happens > outside of test code. A problem of calling ExecuteCDP here is that UpdatePropertyTrees isn't called by the function. But there is also a place where we can add force effect tree update in ComputeVisibleRects. Both a single fix in UpdatePropertyTrees and a single fix in ComputeVisibleRects can fix the bug.
lgtm
The CQ bit was checked by sunxd@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1994213002/20001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1994213002/20001
Message was sent while issue was closed.
Description was changed from ========== cc: Update effect_tree backface visibility when transform is updated. We should update effect_tree as well in UpdatePropertyTrees when an animation cause a change in transform tree. Failing to update it may cause draw_property_utils incorrectly skip layers. This CL forces the update and added a related unit test. BUG=610280 CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel ========== to ========== cc: Update effect_tree backface visibility when transform is updated. We should update effect_tree as well in UpdatePropertyTrees when an animation cause a change in transform tree. Failing to update it may cause draw_property_utils incorrectly skip layers. This CL forces the update and added a related unit test. BUG=610280 CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel ==========
Message was sent while issue was closed.
Committed patchset #2 (id:20001)
Message was sent while issue was closed.
Description was changed from ========== cc: Update effect_tree backface visibility when transform is updated. We should update effect_tree as well in UpdatePropertyTrees when an animation cause a change in transform tree. Failing to update it may cause draw_property_utils incorrectly skip layers. This CL forces the update and added a related unit test. BUG=610280 CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel ========== to ========== cc: Update effect_tree backface visibility when transform is updated. We should update effect_tree as well in UpdatePropertyTrees when an animation cause a change in transform tree. Failing to update it may cause draw_property_utils incorrectly skip layers. This CL forces the update and added a related unit test. BUG=610280 CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel Committed: https://crrev.com/59dd7daef0b5f11d6f8dd6dd5141967ab0deb0b4 Cr-Commit-Position: refs/heads/master@{#394846} ==========
Message was sent while issue was closed.
Patchset 2 (id:??) landed as https://crrev.com/59dd7daef0b5f11d6f8dd6dd5141967ab0deb0b4 Cr-Commit-Position: refs/heads/master@{#394846} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
