Bug 90329

Summary: [chromium] Remove WebTransformationMatrix::mapPoint overrides
Product: WebKit Reporter: James Robinson <jamesr>
Component: New BugsAssignee: James Robinson <jamesr>
Status: RESOLVED FIXED    
Severity: Normal CC: abarth, cc-bugs, dglazkov, fishd, shawnsingh, tkent+wkapi, webkit.review.bot
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Patch
none
map instead of project in drawTileQuad
none
Patch enne: review+

Description James Robinson 2012-06-29 18:28:57 PDT
[chromium] Remove WebTransformationMatrix::mapPoint overrides
Comment 1 James Robinson 2012-06-29 18:30:24 PDT
Created attachment 150287 [details]
Patch
Comment 2 WebKit Review Bot 2012-06-29 18:32:15 PDT
Please wait for approval from abarth@webkit.org, dglazkov@chromium.org, fishd@chromium.org, jamesr@chromium.org or tkent@chromium.org before submitting, as this patch contains changes to the Chromium public API. See also https://trac.webkit.org/wiki/ChromiumWebKitAPI.
Comment 3 James Robinson 2012-06-29 18:33:04 PDT
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)
Comment 4 James Robinson 2012-06-29 18:38:56 PDT
Actually, I think I just don't understand the distinction in terminology between project + map in this code.
Comment 5 James Robinson 2012-06-29 18:39:36 PDT
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.
Comment 6 Dana Jansens 2012-06-30 08:38:15 PDT
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.
Comment 7 Shawn Singh 2012-07-02 09:52:41 PDT
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.
Comment 8 James Robinson 2012-07-02 11:38:54 PDT
Created attachment 150456 [details]
map instead of project in drawTileQuad
Comment 9 Adrienne Walker 2012-07-02 11:56:13 PDT
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.
Comment 10 James Robinson 2012-07-30 15:18:27 PDT
(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.
Comment 11 Adrienne Walker 2012-07-30 15:22:39 PDT
Comment on attachment 150456 [details]
map instead of project in drawTileQuad

R=me.  Oh, whoops.  You're totally right.
Comment 12 James Robinson 2012-07-30 15:23:40 PDT
Created attachment 155382 [details]
Patch
Comment 13 James Robinson 2012-07-30 15:26:06 PDT
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.
Comment 14 Adrienne Walker 2012-07-30 15:31:08 PDT
Comment on attachment 155382 [details]
Patch

R=me.
Comment 15 James Robinson 2012-07-30 15:38:07 PDT
Committed r124105: <http://trac.webkit.org/changeset/124105>
Comment 16 Shawn Singh 2012-07-30 16:56:14 PDT
(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.