Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2883713002:
Document some transform node animation fields. (Closed)
DescriptionDocument some transform node animation fields.
Also add links to cc/README.md for two recent talks on property trees
that provide helpful overview background on the current and future
state of things.
BUG=709137
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel
Review-Url: https://codereview.chromium.org/2883713002
Cr-Commit-Position: refs/heads/master@{#472302}
Committed: https://chromium.googlesource.com/chromium/src/+/a6fc2f20dd92dbf5ea9b73ae877708efd0f432d4
Patch Set 1 #Patch Set 2 : Fix comment on node_and_ancestors_are_animated_or_invertible. #
Total comments: 17
Patch Set 3 : Feedback. #Patch Set 4 : More cleanup. #Patch Set 5 : Sync to head. #Messages
Total messages: 26 (9 generated)
Description was changed from ========== Document some transform node animation fields. Also add links to cc/README.md for two recent talks on property trees that provide helpful overview background on the current and future state of things. BUG=709137 ========== to ========== Document some transform node animation fields. Also add links to cc/README.md for two recent talks on property trees that provide helpful overview background on the current and future state of things. BUG=709137 CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel ==========
wkorman@chromium.org changed reviewers: + chrishtr@chromium.org, enne@chromium.org, pdr@chromium.org
Still more to do for effect node, this is focused on transform node but it also
had more fields.
I only referenced methods ("see xxx") in comments when the field in question was
essentially set in that one place, and with some complex logic that warranted
review for someone looking to understand how it's computed.
Open to being more or less verbose and any other wordsmithing.
https://codereview.chromium.org/2883713002/diff/20001/cc/trees/property_tree.h File cc/trees/property_tree.h (right): https://codereview.chromium.org/2883713002/diff/20001/cc/trees/property_tree.... cc/trees/property_tree.h:304: // TODO(wkorman): Consider renaming cached_data_ to better describe what's Not sure if this needs to be a TODO in the code. https://codereview.chromium.org/2883713002/diff/20001/cc/trees/transform_node.h File cc/trees/transform_node.h (right): https://codereview.chromium.org/2883713002/diff/20001/cc/trees/transform_node... cc/trees/transform_node.h:74: // invertible transform. Defaults to true. See also Defaults to true is I think a bit misleading here, and below. It's defaulted to true in the constructor, but PropertyTreeBuilder will only leave it true under some conditions. I think all of these should be defaulted to false, or else they are inconsistent with each other. I recommend doing it in this patch. https://codereview.chromium.org/2883713002/diff/20001/cc/trees/transform_node... cc/trees/transform_node.h:85: // of exact timeline) transform animation. Defaults to false. Defaults to false, vs true above for node_and_ancestors_are_animated_or_invertible, is pretty weird..
LGTM, documentation++! https://codereview.chromium.org/2883713002/diff/20001/cc/trees/transform_node.h File cc/trees/transform_node.h (right): https://codereview.chromium.org/2883713002/diff/20001/cc/trees/transform_node... cc/trees/transform_node.h:76: bool node_and_ancestors_are_animated_or_invertible : 1; This might be cleaner if we just tracked ancestor_has_potential_animation and ancestors_are_invertible separately. Anyway, not for this patch.
https://codereview.chromium.org/2883713002/diff/20001/cc/trees/transform_node.h File cc/trees/transform_node.h (right): https://codereview.chromium.org/2883713002/diff/20001/cc/trees/transform_node... cc/trees/transform_node.h:76: bool node_and_ancestors_are_animated_or_invertible : 1; On 2017/05/13 at 02:25:33, pdr. wrote: > This might be cleaner if we just tracked ancestor_has_potential_animation and ancestors_are_invertible separately. Anyway, not for this patch. Agreed on both counts. https://codereview.chromium.org/2883713002/diff/20001/cc/trees/transform_node... cc/trees/transform_node.h:88: // animation. Defaults to false. I'm not sure I really like all this "defaults to false". Can we initialize these members in the header and let the code be the documentation? https://codereview.chromium.org/2883713002/diff/20001/cc/trees/transform_node... cc/trees/transform_node.h:96: // translations. Defaults to true. Used when computing animation I'm not sure that explaining where this is used adds a ton here. I think knowing that a node has only translations allows for simplified decomposition of the transform matrix, but that's also kind of implicit.
Replying with notes while I continue working to fix behavior change from revising defaults to match chrishtr@ request. https://codereview.chromium.org/2883713002/diff/20001/cc/trees/property_tree.h File cc/trees/property_tree.h (right): https://codereview.chromium.org/2883713002/diff/20001/cc/trees/property_tree.... cc/trees/property_tree.h:304: // TODO(wkorman): Consider renaming cached_data_ to better describe what's On 2017/05/13 01:07:18, chrishtr wrote: > Not sure if this needs to be a TODO in the code. Done. https://codereview.chromium.org/2883713002/diff/20001/cc/trees/transform_node.h File cc/trees/transform_node.h (right): https://codereview.chromium.org/2883713002/diff/20001/cc/trees/transform_node... cc/trees/transform_node.h:74: // invertible transform. Defaults to true. See also On 2017/05/13 01:07:18, chrishtr wrote: > Defaults to true is I think a bit misleading here, and below. > It's defaulted to true in the constructor, but > PropertyTreeBuilder will only leave it true under some conditions. > > I think all of these should be defaulted to false, or else they > are inconsistent with each other. I recommend doing it in this > patch. Brief update -- working on this since this morning. Changing these defaults not too surprisingly breaks a batch of tests including failure mode in form of infinite hang. Exploring currently. I had assumed these defaults (what's in ctor vs what builder or transform node helper does later) were sort of "the initial value that one would expect to be most common, or intuitive, for the member data semantic meaning in question." However per feedback on this change it's obviously not really the case, since for example has_only_translation_animations is initialized to true but in common case we have no animations. So I think this grew organically to have whatever values were convenient perhaps even for unit tests. I'm following your instruction re: making them all default to false, but I think a case could be made that having them default to the "common/expected value" may make as much or more sense. Avoid setting again when unneeded, etc. Next bit was written before enne@ feedback but leaving for posterity: Also on reflection, am not sure stating the default in comment is useful, so removed that bit but open to thought. If we want to say something perhaps we're better off summarizing what logic property tree or builder applies to compute, and perhaps when in overview of frame lifecycle. https://codereview.chromium.org/2883713002/diff/20001/cc/trees/transform_node... cc/trees/transform_node.h:76: bool node_and_ancestors_are_animated_or_invertible : 1; On 2017/05/13 02:25:33, pdr. wrote: > This might be cleaner if we just tracked ancestor_has_potential_animation and > ancestors_are_invertible separately. Anyway, not for this patch. Acknowledged. https://codereview.chromium.org/2883713002/diff/20001/cc/trees/transform_node... cc/trees/transform_node.h:85: // of exact timeline) transform animation. Defaults to false. On 2017/05/13 01:07:18, chrishtr wrote: > Defaults to false, vs true above for > node_and_ancestors_are_animated_or_invertible, is pretty weird.. Acknowledged. https://codereview.chromium.org/2883713002/diff/20001/cc/trees/transform_node... cc/trees/transform_node.h:88: // animation. Defaults to false. On 2017/05/15 17:14:51, enne wrote: > I'm not sure I really like all this "defaults to false". Can we initialize > these members in the header and let the code be the documentation? Removed all notes about defaults in these comments. It doesn't look like a bit-field member can have an in-class initializer: In file included from ../../cc/trees/transform_node.cc:9: ../../cc/trees/transform_node.h:75:58: error: bit-field member cannot have an in-class initializer bool node_and_ancestors_are_animated_or_invertible : 1 = false; More here: http://stackoverflow.com/questions/16520701/bit-fields-in-class-initializatio... https://codereview.chromium.org/2883713002/diff/20001/cc/trees/transform_node... cc/trees/transform_node.h:96: // translations. Defaults to true. Used when computing animation On 2017/05/15 17:14:51, enne wrote: > I'm not sure that explaining where this is used adds a ton here. I think > knowing that a node has only translations allows for simplified decomposition of > the transform matrix, but that's also kind of implicit. Removed this and similar above.
On 2017/05/15 at 19:48:27, wkorman wrote: > Replying with notes while I continue working to fix behavior change from revising defaults to match chrishtr@ request. > > https://codereview.chromium.org/2883713002/diff/20001/cc/trees/property_tree.h > File cc/trees/property_tree.h (right): > > https://codereview.chromium.org/2883713002/diff/20001/cc/trees/property_tree.... > cc/trees/property_tree.h:304: // TODO(wkorman): Consider renaming cached_data_ to better describe what's > On 2017/05/13 01:07:18, chrishtr wrote: > > Not sure if this needs to be a TODO in the code. > > Done. > > https://codereview.chromium.org/2883713002/diff/20001/cc/trees/transform_node.h > File cc/trees/transform_node.h (right): > > https://codereview.chromium.org/2883713002/diff/20001/cc/trees/transform_node... > cc/trees/transform_node.h:74: // invertible transform. Defaults to true. See also > On 2017/05/13 01:07:18, chrishtr wrote: > > Defaults to true is I think a bit misleading here, and below. > > It's defaulted to true in the constructor, but > > PropertyTreeBuilder will only leave it true under some conditions. > > > > I think all of these should be defaulted to false, or else they > > are inconsistent with each other. I recommend doing it in this > > patch. > > Brief update -- working on this since this morning. Changing these defaults not too surprisingly breaks a batch of tests including failure mode in form of infinite hang. Exploring currently. > > I had assumed these defaults (what's in ctor vs what builder or transform node helper does later) were sort of "the initial value that one would expect to be most common, or intuitive, for the member data semantic meaning in question." However per feedback on this change it's obviously not really the case, since for example has_only_translation_animations is initialized to true but in common case we have no animations. > > So I think this grew organically to have whatever values were convenient perhaps even for unit tests. If it ends up being non-trivial to fix the tests, totally ok for another patch w/TODO. > > I'm following your instruction re: making them all default to false, but I think a case could be made that having them default to the "common/expected value" may make as much or more sense. Avoid setting again when unneeded, etc. > > Next bit was written before enne@ feedback but leaving for posterity: Also on reflection, am not sure stating the default in comment is useful, so removed that bit but open to thought. If we want to say something perhaps we're better off summarizing what logic property tree or builder applies to compute, and perhaps when in overview of frame lifecycle. > > https://codereview.chromium.org/2883713002/diff/20001/cc/trees/transform_node... > cc/trees/transform_node.h:76: bool node_and_ancestors_are_animated_or_invertible : 1; > On 2017/05/13 02:25:33, pdr. wrote: > > This might be cleaner if we just tracked ancestor_has_potential_animation and > > ancestors_are_invertible separately. Anyway, not for this patch. > > Acknowledged. > > https://codereview.chromium.org/2883713002/diff/20001/cc/trees/transform_node... > cc/trees/transform_node.h:85: // of exact timeline) transform animation. Defaults to false. > On 2017/05/13 01:07:18, chrishtr wrote: > > Defaults to false, vs true above for > > node_and_ancestors_are_animated_or_invertible, is pretty weird.. > > Acknowledged. > > https://codereview.chromium.org/2883713002/diff/20001/cc/trees/transform_node... > cc/trees/transform_node.h:88: // animation. Defaults to false. > On 2017/05/15 17:14:51, enne wrote: > > I'm not sure I really like all this "defaults to false". Can we initialize > > these members in the header and let the code be the documentation? > > Removed all notes about defaults in these comments. > > It doesn't look like a bit-field member can have an in-class initializer: > > In file included from ../../cc/trees/transform_node.cc:9: > ../../cc/trees/transform_node.h:75:58: error: bit-field member cannot have an in-class initializer > bool node_and_ancestors_are_animated_or_invertible : 1 = false; > > More here: > > http://stackoverflow.com/questions/16520701/bit-fields-in-class-initializatio... > > https://codereview.chromium.org/2883713002/diff/20001/cc/trees/transform_node... > cc/trees/transform_node.h:96: // translations. Defaults to true. Used when computing animation > On 2017/05/15 17:14:51, enne wrote: > > I'm not sure that explaining where this is used adds a ton here. I think > > knowing that a node has only translations allows for simplified decomposition of > > the transform matrix, but that's also kind of implicit. > > Removed this and similar above.
https://codereview.chromium.org/2883713002/diff/20001/cc/trees/transform_node.h File cc/trees/transform_node.h (right): https://codereview.chromium.org/2883713002/diff/20001/cc/trees/transform_node... cc/trees/transform_node.h:76: bool node_and_ancestors_are_animated_or_invertible : 1; Current logic in TransformTree::UpdateNodeAndAncestorsAreAnimatedOrInvertible relies on node_and_ancestors_are_animated_or_invertible being initialized to true. I spent time but am not yet sure what changes are needed to fix. Potentially revising TransformTree::SetRootTransformsAndScales but I made a naive initial effort there. At this point I've more time into this than I think warranted currently, especially since from above thread it sounds like we'd prefer to track these separately. But then on that front, don't we already have cached values for is_invertible and has_potential_animation, is_currently_animating, etc.? Something in logic must be different re: computing this value. Tests involved in the parent node node_and_ancestors_are_animated_or_invertible check are for the record: LayerTreeHostCommonTest.LayerSkippingInSubtreeOfSingularTransform LayerTreeHostCommonTest.SkippingLayerImpl I consulted with Jayadev on chat re: the above. Suggest pursuing this in person together at some point. It's not clear to me that just having them all default to false, but then carefully having logic to identify when we're constructing/initializing for first time in property tree or elsewhere, is better than having the mixed false/true values we have here today.
jaydasika@chromium.org changed reviewers: + jaydasika@chromium.org
https://codereview.chromium.org/2883713002/diff/20001/cc/trees/transform_node.h File cc/trees/transform_node.h (right): https://codereview.chromium.org/2883713002/diff/20001/cc/trees/transform_node... cc/trees/transform_node.h:76: bool node_and_ancestors_are_animated_or_invertible : 1; On 2017/05/15 19:48:27, wkorman wrote: > On 2017/05/13 02:25:33, pdr. wrote: > > This might be cleaner if we just tracked ancestor_has_potential_animation and > > ancestors_are_invertible separately. Anyway, not for this patch. > > Acknowledged. We already have the separate bools you have mentioned (to_screen_is_potentially_animated and ancestors_are_invertible). The reason you still need this bool also is to distinguish the two cases: 1. T1(singular)->T2(animating) 2. T1(singular and animating)->T2. Notice that in both these cases, for T2 : ancestors_are_invertible = false and ancestors_are_animating = true. But we want ancestors_are_invertible_or_animating to be false for case 1 and true for case 2. So, you can't infer this bool from the other 2 bools.
https://codereview.chromium.org/2883713002/diff/20001/cc/trees/transform_node.h File cc/trees/transform_node.h (right): https://codereview.chromium.org/2883713002/diff/20001/cc/trees/transform_node... cc/trees/transform_node.h:76: bool node_and_ancestors_are_animated_or_invertible : 1; On 2017/05/15 23:53:54, jaydasika wrote: > On 2017/05/15 19:48:27, wkorman wrote: > > On 2017/05/13 02:25:33, pdr. wrote: > > > This might be cleaner if we just tracked ancestor_has_potential_animation > and > > > ancestors_are_invertible separately. Anyway, not for this patch. > > > > Acknowledged. > > We already have the separate bools you have mentioned > (to_screen_is_potentially_animated and ancestors_are_invertible). The reason you > still need this bool also is to distinguish the two cases: > 1. T1(singular)->T2(animating) > 2. T1(singular and animating)->T2. > > Notice that in both these cases, for T2 : ancestors_are_invertible = false and > ancestors_are_animating = true. > But we want ancestors_are_invertible_or_animating to be false for case 1 and > true for case 2. > So, you can't infer this bool from the other 2 bools. Is the below equivalent? T2.node_and_ancestors_are_animated_or_invertible = (T1.to_screen_is_potentially_animated && T2.ancestors_are_invertible) If so, do we not rely on above just because we want to avoid having to reference T1 when we are looking for the value for T2?
https://codereview.chromium.org/2883713002/diff/20001/cc/trees/transform_node.h File cc/trees/transform_node.h (right): https://codereview.chromium.org/2883713002/diff/20001/cc/trees/transform_node... cc/trees/transform_node.h:76: bool node_and_ancestors_are_animated_or_invertible : 1; On 2017/05/16 00:28:01, wkorman wrote: > On 2017/05/15 23:53:54, jaydasika wrote: > > On 2017/05/15 19:48:27, wkorman wrote: > > > On 2017/05/13 02:25:33, pdr. wrote: > > > > This might be cleaner if we just tracked ancestor_has_potential_animation > > and > > > > ancestors_are_invertible separately. Anyway, not for this patch. > > > > > > Acknowledged. > > > > We already have the separate bools you have mentioned > > (to_screen_is_potentially_animated and ancestors_are_invertible). The reason > you > > still need this bool also is to distinguish the two cases: > > 1. T1(singular)->T2(animating) > > 2. T1(singular and animating)->T2. > > > > Notice that in both these cases, for T2 : ancestors_are_invertible = false and > > ancestors_are_animating = true. > > But we want ancestors_are_invertible_or_animating to be false for case 1 and > > true for case 2. > > So, you can't infer this bool from the other 2 bools. > > Is the below equivalent? > > T2.node_and_ancestors_are_animated_or_invertible = > (T1.to_screen_is_potentially_animated && > T2.ancestors_are_invertible) > > If so, do we not rely on above just because we want to avoid having to reference > T1 when we are looking for the value for T2? It's not equivalent. For case 2 in my example, if T0(not animating, invertible) is parent of T1: T1.node_and_ancestors_are_animated_or_invertible should be true, but (T0.to_screen_is_potentially_animated && T1.ancestors_are_invertible) = false
cc owners PTAL. This change is purely documentation update.
lgtm
lgtm
The CQ bit was checked by wkorman@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from pdr@chromium.org, enne@chromium.org Link to the patchset: https://codereview.chromium.org/2883713002/#ps80001 (title: "Sync to head.")
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by wkorman@chromium.org
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 80001, "attempt_start_ts": 1494983710792740,
"parent_rev": "2c8d005df63e8ef30df1ed4f0afe469cca335b2c", "commit_rev":
"a6fc2f20dd92dbf5ea9b73ae877708efd0f432d4"}
Message was sent while issue was closed.
Description was changed from ========== Document some transform node animation fields. Also add links to cc/README.md for two recent talks on property trees that provide helpful overview background on the current and future state of things. BUG=709137 CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel ========== to ========== Document some transform node animation fields. Also add links to cc/README.md for two recent talks on property trees that provide helpful overview background on the current and future state of things. BUG=709137 CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel Review-Url: https://codereview.chromium.org/2883713002 Cr-Commit-Position: refs/heads/master@{#472302} Committed: https://chromium.googlesource.com/chromium/src/+/a6fc2f20dd92dbf5ea9b73ae8777... ==========
Message was sent while issue was closed.
Committed patchset #5 (id:80001) as https://chromium.googlesource.com/chromium/src/+/a6fc2f20dd92dbf5ea9b73ae8777... |
