1301818 - Prepare BSPTree for integration with the layers code

Status: RESOLVED FIXED

(Core :: Graphics: Layers, defect)

Description

[Miko Mynttinen]

These changes prepare BSPTree, BSPTree tests, and Polygon3D for integration with the layers code. I apologize for the large changeset, these classes are tightly-coupled, so splitting the patch is very difficult.

Here is a list of changes:

TestBSPTree.cpp
---------------
- Fix inverted logic in FuzzyCompare and rename it to FuzzyEquals.
- Add tests to verify that polygon/point comparison works properly.
- Fix tests that broke from surface normal change.
- Refactor tests to use the new LayerPolygon struct.


BSPTree.cpp / BSPTree.h
-----------------------
- Add new LayerPolygon struct to represent layers with arbitrary, convex geometry.
- Refactor the code to use LayerPolygon instead of Polygon3D.


Polygon.h
---------
- Add TransformToLayerSpace() and TransformToScreenSpace() functions to transform polygon vertices.
- Change how the polygon surface normal is calculated: Now the initial surface normal is defined during the construction. The surface normal is transformed along with the polygon (by using inverse transpose of the transform).
- Remove CalculateNormal() as obsolete.
- Add EnsurePlanarPolygon() for debug builds.

Comment 2

[Miko Mynttinen]

Attachment: Bug 1301818 - Prepare BSPTree for integration with the layers code

Review commit: https://reviewboard.mozilla.org/r/78030/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/78030/

Comment 3

[Miko Mynttinen]

Comment on attachment 8790054 [details]
Bug 1301818 - Prepare BSPTree for integration with the layers code

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/78030/diff/1-2/

Comment 4

[Miko Mynttinen]

Comment on attachment 8790054 [details]
Bug 1301818 - Prepare BSPTree for integration with the layers code

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/78030/diff/2-3/

Comment 5

[:kip (Kearwood Gilbert)]

Comment on attachment 8790054 [details]
Bug 1301818 - Prepare BSPTree for integration with the layers code

https://reviewboard.mozilla.org/r/78030/#review77414

::: gfx/2d/Polygon.h:91
(Diff revision 3)
> -      for (size_t i = 2; i < mPoints.Length() - 1; ++i) {
> -        mNormal += (mPoints[i] - mPoints[0]).CrossProduct(mPoints[i + 1] - mPoints[0]);
> -      }
>      }
>  
> -    mNormal.Normalize();
> +    normal.Normalize();

If the length of the normal vector here is 0, the Normalize() function may divide by zero.

A safety check that rejects this polygon rather than crashing could be useful here.  Perhaps check that at least one member is < -epsilon or > epsilon ?

::: gfx/layers/BSPTree.h:38
(Diff revision 3)
> +
> +// Represents a node in a BSP tree. The node contains at least one layer with
> +// associated geometry that is used as a splitting plane, and at most two child
> +// nodes that represent the splitting planes that further subdivide the space.
>  struct BSPTreeNode {
> -  explicit BSPTreeNode(gfx::Polygon3D && aPolygon)
> +  explicit BSPTreeNode(LayerPolygon && layer)

nit: Remove space between double-address operator and LayerPolygon

Comment 6

[:kip (Kearwood Gilbert)]

Comment on attachment 8790054 [details]
Bug 1301818 - Prepare BSPTree for integration with the layers code

https://reviewboard.mozilla.org/r/78030/#review77414

This is looking good, thanks!  r=me with those two fixes.

Comment 7

[:kip (Kearwood Gilbert)]

Comment on attachment 8790054 [details]
Bug 1301818 - Prepare BSPTree for integration with the layers code

r=me with the two changes in MozReview.

Comment 8

[Miko Mynttinen]

Comment on attachment 8790054 [details]
Bug 1301818 - Prepare BSPTree for integration with the layers code

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/78030/diff/3-4/

Comment 9

[:kip (Kearwood Gilbert)]

Comment on attachment 8790054 [details]
Bug 1301818 - Prepare BSPTree for integration with the layers code

https://reviewboard.mozilla.org/r/78028/#review78264

Comment 10

[:kip (Kearwood Gilbert)]

Comment on attachment 8790054 [details]
Bug 1301818 - Prepare BSPTree for integration with the layers code

https://reviewboard.mozilla.org/r/78028/#review78266

Comment 11

[:kip (Kearwood Gilbert)]

Comment on attachment 8790054 [details]
Bug 1301818 - Prepare BSPTree for integration with the layers code

https://reviewboard.mozilla.org/r/78030/#review78268

Ship it!

Comment 12

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/974defd493a3
Prepare BSPTree for integration with the layers code r=kip

Comment 13

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/974defd493a3