Closed Bug 1301818 Opened 6 years ago Closed 6 years ago

Prepare BSPTree for integration with the layers code

Categories

(Core :: Graphics: Layers, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla52
Tracking Status
firefox52 --- fixed

People

(Reporter: miko, Assigned: miko)

References

Details

Attachments

(1 file)

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.
Duplicate of this bug: 1295723
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 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 on attachment 8790054 [details]
Bug 1301818 - Prepare BSPTree for integration with the layers code

r=me with the two changes in MozReview.
Attachment #8790054 - Flags: review?(kgilbert) → review+
Comment on attachment 8790054 [details]
Bug 1301818 - Prepare BSPTree for integration with the layers code

https://reviewboard.mozilla.org/r/78028/#review78264
Comment on attachment 8790054 [details]
Bug 1301818 - Prepare BSPTree for integration with the layers code

https://reviewboard.mozilla.org/r/78028/#review78266
Comment on attachment 8790054 [details]
Bug 1301818 - Prepare BSPTree for integration with the layers code

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

Ship it!
Attachment #8790054 - Flags: review?(kgilbert) → review+
Pushed by kgilbert@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/974defd493a3
Prepare BSPTree for integration with the layers code r=kip
https://hg.mozilla.org/mozilla-central/rev/974defd493a3
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla52
You need to log in before you can comment on or make changes to this bug.