Summary: | [chromium] Remove WebTransformationMatrix::mapPoint overrides | ||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | James Robinson <jamesr> | ||||||||
Component: | New Bugs | Assignee: | 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
James Robinson
2012-06-29 18:28:57 PDT
Created attachment 150287 [details]
Patch
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 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) Actually, I think I just don't understand the distinction in terminology between project + map in this code. 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 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 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. Created attachment 150456 [details]
map instead of project in drawTileQuad
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.
(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 on attachment 150456 [details]
map instead of project in drawTileQuad
R=me. Oh, whoops. You're totally right.
Created attachment 155382 [details]
Patch
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 on attachment 155382 [details]
Patch
R=me.
Committed r124105: <http://trac.webkit.org/changeset/124105> (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. |