1.71 KB,
text/html

Details  
23.26 KB,
patch

bjacob
:
review+

Details  Diff  Splinter Review 
A div is drawn here with z position 10. It rotates around and we render it incompletely (disappears, flickering) whenever it intersects with the view plane. This might be a clipping issue.
(Reporter)  
Comment 1•4 years ago


mattwoodrow: wonder if that’s a regression mattwoodrow: I can take a look later today mattwoodrow: The fact that it dissapears in chunks that are aligned to the nontransformed space is interesting mattwoodrow: sounds like we’re clipping/adjusting the visible region of the pretransform layer incorrectly
(Assignee)  
Comment 2•4 years ago


This is a bug with nsDisplayTransform::ComputeVisibility. We're taking the visible area of the window and converting it into the coordinate space of the children and getting really wrong results. These sort of bugs were meant to be fixed when we added gfx3DMatrix:UntransformBounds, but I guess that still isn't sufficient.
Comment 3•4 years ago


Created attachment 8448466 [details]
non animation html
it appears/disappears when resize width of browser
Comment 4•4 years ago


I can reproduce in Firefox10 on windows7. so, I think this is not a regression.
(Assignee)  
Comment 5•4 years ago


Thanks Alice! Quite a few bugs here actually. I've constructed a standalone testcase (using eigen) for the incorrect visibility calculations and sent it to bjacob to see if he can help me figure out where we're going wrong. I think it's because we're failing to account for the 4th component (perspective/w) of the transformed vectors. We're also failing to respect all the preserve3d properties, since we generate multiple frames for the #o frame and get confused.
(Assignee)  
Comment 6•4 years ago


(In reply to Matt Woodrow (:mattwoodrow) from comment #5) > We're also failing to respect all the preserve3d properties, since we > generate multiple frames for the #o frame and get confused. Actually, this is fine, it's just a really weird testcase. We've got transformstyle:preserve3d, overflow:hidden, and perspective: 536.929px; all on the same frame (#o). overflow:hidden disables preserve3d (http://www.w3.org/TR/csstransforms1/#propdeftransformstyle), and perspective is only applied to child elements so doesn't really make sense in the middle of a preserve3d chain.
(Assignee)  
Comment 7•4 years ago


Created attachment 8451973 [details] [diff] [review] Only include untransformed points that are above the w=0 plane Roc, this has problems with nsLayoutUtils::TransformPoints which assumes that if one of the points can be transformed then all of them can. This isn't true when we're going backwards through a projective transform. I've left the code asis for now, but it has the potential to get some crazy numbers for points that can't be mapped to any value in the destination coordinate space.
(Assignee)  
Comment 8•4 years ago


I see that the nsLayoutUtils::TransformPoints interface matches the GeometryUtils one proposed by cssomview, we might need a change to spec too then.
Comment 9•4 years ago


Here is a quick explanation of what is going on here geometrically. For the sake of getting a simple example that still reflects the core issue at hand, let us work in 1 dimension. So our space is a line, with a single coordinate, "x". Let us use uppercase letters for homogeneous coordinates: (X, W). I choose the letter W for the last homogeneous coord so that we feel on familiar grounds here. Let us choose as usual W=0 for the projective infinity, so that as usual the correspondence between the affine x coordinate and projective (X, W) coordinates is given by t x = X / W. Note that the projective 1D space, being the set of all lines through the origin in the (X, W) plane, is a circle with opposite points identified, which is again a circle. Another way to see that the projective 1D space is a circle, is to see it as the line with one "point at infinity" adjoined, and the two "ends" of the line glued together at that point. In our affine 1D world, which is a line, a "rect" is just an interval, [a .. b]. So we are used to representing rects by just giving the two endpoints. We are used, then, to transforming rects by just transforming endpoints. No more information than the resulting two points, is needed to encode the transformed rect. In our projective 1D world, which is a circle, a "rect" is still going to be an interval, i.e. a set of points contained in between two endpoints on the circle. But it is no longer true that a rect is uniquely determined by its two endpoints. Indeed, on a circle, there are two ways to go from one point to another: clockwise or counterclockwise. So our whole approach of transforming rects by just transforming the endpoints and constructing the result rect from those transformed endpoints, breaks down. In order to properly transform rects under projective transformations, we would need to carry some extra information to tell which side is inside. The approach of the patch here is to cut down the domain of homogeneous coordinates: instead of allowing arbitrary (X, W) in the entire plane, we now restrict to the W >= 0 halfplane. This allows to remove the abovedescribed ambiguity as follows. Take two points in the projective 1D space, say (X1, W1) and (X2, W2). Normalize these coords so that they have norm 1, so we are looking at points on the unit circle in the (X, W) plane. Normally, there is still an ambiguity as (X1, W1) and (X1, W1) both satisy this condition and represent the same point in projective space. But that's where our W>=0 restriction kicks in: it removes one of these two candidates. Thus each point in projective space is now represented by only exactly one (X, W) homogeneous coordinate, except for the point at infinity (W=0). Concretely, we've now represented the projective 1D space as a halfcircle with its two endpoints glued. This gives us a welldefined choice of what's the "rect" delimited by two endpoints: just go in the direction that doesn't go through the endpoints of the halfcircle. What, then, if a projective transform is expressed by a matrix that acts on (X, W) in a way that does not preserve the W>=0 condition? The approach of the patch at hand there is then to just clamp the edge to stop before W becomes negative. Actually, this patch does something more hacky, less meaningful than just described. Ideally, it would stop at exactly W=0 and deal properly with pointsatinfinity thus obtained. But instead, because it is a bunch of code that is not designed to handle pointsatinfinity gracefully (i.e. that code expects to be able to represent the result rect in affine coordinates), it has to stop not at W=0 but "just before": at a very small positive W like W=0.000001 or some such. That's the ugly, arbitrary, fragile part. I'm going to r+ because it fixes a concrete bug already and Matt said it's what Chromium does. But please, file a followup bug about that. That followup bug should at least be about properly clamping at W=0 and dealing with the resulting infinities (e.g. the transformed rect could be a halfplane or a wedge between two halflines). Ideally though, we would find a way to deal with arbitrary projective transforms.
Comment 10•4 years ago


Comment on attachment 8451973 [details] [diff] [review] Only include untransformed points that are above the w=0 plane Review of attachment 8451973 [details] [diff] [review]:  Haven't carefully checked all the math, just the parts that looked involved enough to have room to let typos slip in. Basically, I'm trusting that we have enough reftests for any gross wrong behavior to be caught... let me know if you would like more careful reviewing of specific parts. ::: gfx/layers/apz/src/APZCTreeManager.cpp @@ +1103,5 @@ > > AsyncPanZoomController* result = nullptr; > // This walks the tree in depthfirst, reverse order, so that it encounters > // APZCs fronttoback on the screen. > + if (hitTestPointForChildLayers.AbovePerspectivePlane()) { I would say HasPositiveWCoord() to be very specific and downtoearth here. AbovePerspectivePlane has two issues: it's a 3D "hyperplane" not a 2D plane, and it's called "at infinity" not "perspective" afaik. ::: gfx/thebes/gfx3DMatrix.cpp @@ +774,5 @@ > + // the positive side of the w=0 plane as possible. > + > + // Since we know what we want the w component to be, we can rearrange the > + // interpolation equation and solve for t. > + float w = 0.00001f; Please file that followup bug as described above, and reference it from this comment. This probably deserves a FIXME since it could plausibly be a cause of bugs in the future  either excessive clipping or loss of precision.
(Assignee)  
Comment 11•4 years ago


Awesome explanation Benoit, thanks for adding that.
(Assignee)  
Updated•4 years ago

(Assignee)  
Comment 12•4 years ago


https://hg.mozilla.org/integration/mozillainbound/rev/15b924cf6eab
Thanks bjacob! We don't really want to deal with rects that transform to unbounded shapes. I'm pretty sure that is never going to be useful, it's always going to be confusing, and we don't want our geometric data types to have to handle those cases. So I think we need to always choose the projection that excludes pointsatinfinity. I think this means (but I could be wrong) that in nsLayoutUtils::TransformPoints, we should return an error if any two of the transformed points have differentlysigned W values or if any one of the transformed points has a zero W value. I.e. if they're all negative or all positive I guess we're OK. Is that right?
Comment 14•4 years ago


https://hg.mozilla.org/mozillacentral/rev/15b924cf6eab
(Assignee)  
Comment 15•4 years ago


(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #13) > Thanks bjacob! > > We don't really want to deal with rects that transform to unbounded shapes. > I'm pretty sure that is never going to be useful, it's always going to be > confusing, and we don't want our geometric data types to have to handle > those cases. So I think we need to always choose the projection that > excludes pointsatinfinity. > > I think this means (but I could be wrong) that in > nsLayoutUtils::TransformPoints, we should return an error if any two of the > transformed points have differentlysigned W values or if any one of the > transformed points has a zero W value. I.e. if they're all negative or all > positive I guess we're OK. Is that right? That's one option, and probably is correct re the current spec. I'm not sure what the actual uses cases for this interface are, but the way gecko uses it internally this wouldn't be useful behaviour. Since our input rect is usually the window size, it's quite common to have some of the input points not exist in the transformed plane. We only ever need the rectangle (not a quad) and we can restrict it to the bounds of the child element so we get around it that way.
Description
•