Bug 90329 – [chromium] Remove WebTransformationMatrix::mapPoint overrides

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[James Robinson]

[chromium] Remove WebTransformationMatrix::mapPoint overrides

[James Robinson]

Created attachment 150287 [details]
Patch

[WebKit Review Bot]

Please wait for approval from [e-mail], [e-mail], [e-mail], [e-mail] or [e-mail] before submitting, as this patch contains changes to the Chromium public API. See also https://trac.webkit.org/wiki/ChromiumWebKitAPI.

[James Robinson]

Comment on attachment 150287 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=150287&action=review

> Source/WebCore/platform/graphics/chromium/LayerRendererChromium.cpp:821
> +        bottomRight = CCMathUtil::projectPoint(deviceTransform, bottomRight, clipped);

deviceTransform has been to2dTransform()'d, so projection is unnecessary here (but still correct, I think)

[James Robinson]

Actually, I think I just don't understand the distinction in terminology between project + map in this code.

[James Robinson]

Comment on attachment 150287 [details]
Patch

I think this patch is OK, but please treat any math code I write with extreme skepticism.  I feel that I'm probably using the wrong name for something somewhere, but I think the clipping logic here is what we want.

[Dana Jansens]

Comment on attachment 150287 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=150287&action=review

>> Source/WebCore/platform/graphics/chromium/LayerRendererChromium.cpp:821
>> +        bottomRight = CCMathUtil::projectPoint(deviceTransform, bottomRight, clipped);
> 
> deviceTransform has been to2dTransform()'d, so projection is unnecessary here (but still correct, I think)

deviceTransform is not an inverse transform (it's the drawTransform + window/projection), so I think you want mapPoint. Then again you wrote the method so maybe you know something I don't :) /me defers to shawn.

The rule as I understand it is, apply a transform with map. Apply its inverse with project. Project takes into account you lose z-offsets during map, and applies inverse transform correctly.

[Shawn Singh]

Comment on attachment 150287 [details]
Patch


Unofficially looks good to me, except for the same point that Dana brought up. =)

View in context: https://bugs.webkit.org/attachment.cgi?id=150287&action=review

>>> Source/WebCore/platform/graphics/chromium/LayerRendererChromium.cpp:821
>>> +        bottomRight = CCMathUtil::projectPoint(deviceTransform, bottomRight, clipped);
>> 
>> deviceTransform has been to2dTransform()'d, so projection is unnecessary here (but still correct, I think)
> 
> deviceTransform is not an inverse transform (it's the drawTransform + window/projection), so I think you want mapPoint. Then again you wrote the method so maybe you know something I don't :) /me defers to shawn.
> 
> The rule as I understand it is, apply a transform with map. Apply its inverse with project. Project takes into account you lose z-offsets during map, and applies inverse transform correctly.

I think projectPoint works correctly, but for incorrect reasons (the reason being that we did to2dTransform so project and map are likely to be the same).   For correct concepts, though, we should probably keep mapPoint here, like Dana said...

Dana's rule seems correct to me, except that it might be better to say, "use map() when transforming up the hierarchy, from local space to screen space.  Use project() when transforming down the hierarchy, from screen space to local space" -- that wording would be correct regardless of which transform/inverse we have.

[James Robinson]

Created attachment 150456 [details]
map instead of project in drawTileQuad

[Adrienne Walker]

Comment on attachment 150456 [details]
map instead of project in drawTileQuad

Is there a reason to add a 3D version of mapPoint? It looks like all the mapPoint use z=0, so you can just use the existing 2D version.

[James Robinson]

(In reply to comment #9)
> (From update of attachment 150456 [details])
> Is there a reason to add a 3D version of mapPoint? It looks like all the mapPoint use z=0, so you can just use the existing 2D version.

The input Z is always zero in CCLayerSorter, but the transform might change the Z value so I do need the 3d version.  At least, if I use the 2d version CCLayerSorterTest tests fail.

[Adrienne Walker]

Comment on attachment 150456 [details]
map instead of project in drawTileQuad

R=me.  Oh, whoops.  You're totally right.

[James Robinson]

Created attachment 155382 [details]
Patch

[James Robinson]

Updated to ToT with one small change - instead of ASSERT(!clipping) in CCLayerSorter.cpp I just have a FIXME.  Otherwise, the ASSERT fails on CCLayerSorterTest.LayersUnderPathologicalPerspectiveTransform.  I'm not entirely sure why this is different now - perhaps due to http://trac.webkit.org/changeset/123628 (which touched CCLayerSorter.cpp)?  In any case, the behavior is the same as before - we don't do anything in particular if the map clips.

[Adrienne Walker]

Comment on attachment 155382 [details]
Patch

R=me.

[James Robinson]

Committed r124105: <http://trac.webkit.org/changeset/124105>

[Shawn Singh]

(In reply to comment #13)
> Updated to ToT with one small change - instead of ASSERT(!clipping) in CCLayerSorter.cpp I just have a FIXME.  Otherwise, the ASSERT fails on CCLayerSorterTest.LayersUnderPathologicalPerspectiveTransform.  I'm not entirely sure why this is different now - perhaps due to http://trac.webkit.org/changeset/123628 (which touched CCLayerSorter.cpp)?  In any case, the behavior is the same as before - we don't do anything in particular if the map clips.

LayerSorting has this known problem - we have to revise the sorting algorithm overall so that it doesn't map a point like that, because if it becomes clipped, we can't us it to determine the plane of the layer we're trying to sort.

So yeah, I don't think removing that ASSERT is a big deal.