Mozart: Implement hit testing.

Mozart: Implement hit testing.

Initial implementation of basic hit testing in the scene graph.
This is just enough to get us off the ground.

Changed the hit testing contract to report the Transform associated
with a hit rather than the intersection itself since that's more
useful for implementing input capture behavior.

BUG=
R=abarth@google.com

Committed: https://chromium.googlesource.com/external/mojo/+/a266528ef09d7b5d4df082819f9cc4da2707219e

[jeffbrown, 2016-03-09 02:19:31]

jeffbrown@google.com changed reviewers:
+ abarth@google.com

[abarth, 2016-03-09 04:02:49]

LGTM

https://codereview.chromium.org/1774373002/diff/1/mojo/services/gfx/compositi...
File mojo/services/gfx/composition/interfaces/hit_tests.mojom (right):

https://codereview.chromium.org/1774373002/diff/1/mojo/services/gfx/compositi...
mojo/services/gfx/composition/interfaces/hit_tests.mojom:111: // content space,
multiple this transformation matrix by the hit test point.
s/multiple/multiply/

https://codereview.chromium.org/1774373002/diff/1/services/gfx/compositor/gra...
File services/gfx/compositor/graph/node_def.cc (right):

https://codereview.chromium.org/1774373002/diff/1/services/gfx/compositor/gra...
services/gfx/compositor/graph/node_def.cc:247:
node_transform.postConcat(content_transform_.To<SkMatrix>());
I think you want to do this work in 4x4 matrices.

https://codereview.chromium.org/1774373002/diff/1/services/gfx/compositor/gra...
services/gfx/compositor/graph/node_def.cc:250:
node_transform.mapRect(&global_clip_rect, content_clip_->To<SkRect>());
This isn't very efficient because you're doing a matrix multiplication at every
level of recursion.  One pattern that works reasonably well for hit testing is
to pass the point you're hit testing in the parent's coordinate system.  You
always have that because you needed to test against the parent.  The child can
either re-use that point or can adjust it by the content_transform_.

https://codereview.chromium.org/1774373002/diff/1/services/gfx/compositor/gra...
services/gfx/compositor/graph/node_def.cc:291: void NodeDef::HitTestInner(
s/HitTestInner/HitTestChildren/ ?

https://codereview.chromium.org/1774373002/diff/1/services/gfx/compositor/gra...
services/gfx/compositor/graph/node_def.cc:315: if (*opaque)
In other systems, we've passed this value through the return value rather than
an out-param, but either works.

https://codereview.chromium.org/1774373002/diff/1/services/gfx/compositor/gra...
services/gfx/compositor/graph/node_def.cc:493: hits);
Do we need to test opaque after calling this function?

https://codereview.chromium.org/1774373002/diff/1/services/gfx/compositor/gra...
File services/gfx/compositor/graph/node_def.h (right):

https://codereview.chromium.org/1774373002/diff/1/services/gfx/compositor/gra...
services/gfx/compositor/graph/node_def.h:85: const SkMatrix& parent_transform,
Don't you want an SkMatrix44 here?

[jeffbrown, 2016-03-09 20:32:11]

https://codereview.chromium.org/1774373002/diff/1/mojo/services/gfx/compositi...
File mojo/services/gfx/composition/interfaces/hit_tests.mojom (right):

https://codereview.chromium.org/1774373002/diff/1/mojo/services/gfx/compositi...
mojo/services/gfx/composition/interfaces/hit_tests.mojom:111: // content space,
multiple this transformation matrix by the hit test point.
On 2016/03/09 04:02:48, abarth wrote:
> s/multiple/multiply/

Done.

https://codereview.chromium.org/1774373002/diff/1/services/gfx/compositor/gra...
File services/gfx/compositor/graph/node_def.cc (right):

https://codereview.chromium.org/1774373002/diff/1/services/gfx/compositor/gra...
services/gfx/compositor/graph/node_def.cc:247:
node_transform.postConcat(content_transform_.To<SkMatrix>());
On 2016/03/09 04:02:48, abarth wrote:
> I think you want to do this work in 4x4 matrices.

You're probably right.  Noted for later since this affects the drawing pipeline
too.

https://codereview.chromium.org/1774373002/diff/1/services/gfx/compositor/gra...
services/gfx/compositor/graph/node_def.cc:250:
node_transform.mapRect(&global_clip_rect, content_clip_->To<SkRect>());
On 2016/03/09 04:02:48, abarth wrote:
> This isn't very efficient because you're doing a matrix multiplication at
every
> level of recursion.  One pattern that works reasonably well for hit testing is
> to pass the point you're hit testing in the parent's coordinate system.  You
> always have that because you needed to test against the parent.  The child can
> either re-use that point or can adjust it by the content_transform_.

I previously wrote the code that way.  The trade-off is that in order to
calculate that point, you need to apply the inverse matrix.  I figured 4
matrix*vector multiplications only at nodes which have clips was better than 1
matrix inversion at every node.

In retrospect, this isn't so bad if I just cache the inverse matrix.

https://codereview.chromium.org/1774373002/diff/1/services/gfx/compositor/gra...
services/gfx/compositor/graph/node_def.cc:291: void NodeDef::HitTestInner(
On 2016/03/09 04:02:48, abarth wrote:
> s/HitTestInner/HitTestChildren/ ?

Strictly speaking it's not just children since referenced scenes are handled
here too.  Agree that it's confusing.

https://codereview.chromium.org/1774373002/diff/1/services/gfx/compositor/gra...
services/gfx/compositor/graph/node_def.cc:315: if (*opaque)
On 2016/03/09 04:02:48, abarth wrote:
> In other systems, we've passed this value through the return value rather than
> an out-param, but either works.

SceneContent::HitTest returns a SceneHitPtr as its result so it ended up with an
opaque out-param, and that ultimately determined the shape of the parameter list
deeper in the recursion.

Since the opaque flag governs more of the flow control than that thing, I think
I'll flip it around.

https://codereview.chromium.org/1774373002/diff/1/services/gfx/compositor/gra...
services/gfx/compositor/graph/node_def.cc:493: hits);
On 2016/03/09 04:02:48, abarth wrote:
> Do we need to test opaque after calling this function?

Initially I didn't think so since I was imagining the reference scene as being
part of its parent node and should therefore get a shot at the hit test even if
opaque children were hit.  But on reflection that might be confusion to folks so
let's change it.

https://codereview.chromium.org/1774373002/diff/1/services/gfx/compositor/gra...
File services/gfx/compositor/graph/node_def.h (right):

https://codereview.chromium.org/1774373002/diff/1/services/gfx/compositor/gra...
services/gfx/compositor/graph/node_def.h:85: const SkMatrix& parent_transform,
On 2016/03/09 04:02:48, abarth wrote:
> Don't you want an SkMatrix44 here?

I didn't know that existed!  ha!  I'll make a note for later.

[jeffbrown, 2016-03-09 22:23:00]

Description was changed from

==========
Mozart: Implement hit testing.

Initial implementation of basic hit testing in the scene graph.
This is just enough to get us off the ground.

Changed the hit testing contract to report the Transform associated
with a hit rather than the intersection itself since that's more
useful for implementing input capture behavior.

BUG=
==========

to

==========
Mozart: Implement hit testing.

Initial implementation of basic hit testing in the scene graph.
This is just enough to get us off the ground.

Changed the hit testing contract to report the Transform associated
with a hit rather than the intersection itself since that's more
useful for implementing input capture behavior.

BUG=
R=abarth@google.com

Committed:
https://chromium.googlesource.com/external/mojo/+/a266528ef09d7b5d4df082819f9...
==========

[jeffbrown, 2016-03-09 22:23:01]

Committed patchset #2 (id:20001) manually as
a266528ef09d7b5d4df082819f9cc4da2707219e (presubmit successful).