Clipping planes updates #6045
Clipping planes updates #6045
Conversation
|
@ggetz, thanks for the pull request! Maintainers, we have a signed CLA from @ggetz, so you can review this at any time.
I am a bot who helps you make Cesium awesome! Contributions to my configuration are welcome. 🌍 🌎 🌏 |
Source/Core/Plane.js
Outdated
| //>>includeEnd('debug'); | ||
|
|
||
| if (!defined(result)) { | ||
| result = new Plane(Cartesian3.UNIT_X, 0.0); |
There was a problem hiding this comment.
just do return new Plane(plane.normal, plane.distance)
|
From a quick glace at the code, this looks great @ggetz! I'll play around with this a little bit tomorrow |
|
I seem to get the same result regardless of the value of |
| ClippingPlaneCollection.prototype.add = function(plane) { | ||
| this._planes.push(plane); | ||
|
|
||
| //>>includeStart('debug', pragmas.debug); |
There was a problem hiding this comment.
Check for the exception before calling push; it leaves the object in a better state.
| this.unionClippingRegions = defaultValue(options.unionClippingRegions, true); | ||
| } | ||
|
|
||
| function unionIntersectFunction (value) { |
There was a problem hiding this comment.
Throughout, no need for a space before ( for functions.
| return (value === Intersect.INSIDE); | ||
| }; | ||
| /** | ||
| * |
There was a problem hiding this comment.
Is this missing the description part of the reference doc?
| }; | ||
|
|
||
| function indexOf(planes, plane) { | ||
| for (var i = 0; i < planes.length; ++i) { |
There was a problem hiding this comment.
Not sure if the performance matters anymore in practice, but the Cesium convention is to assign planes.length to a local instead of dereferencing each iteration of the loop.
Source/Core/Plane.js
Outdated
| @@ -70,6 +70,7 @@ define([ | |||
| this.distance = distance; | |||
| } | |||
|
|
|||
|
|
|||
@hpinkos Updated. Tested this on models, tilesets, and the globe, and it was only happening with point clouds, and it's now resolved. |
| @@ -32,20 +34,19 @@ define([ | |||
| * @param {Plane[]} [options.planes=[]] An array of up to 6 {@link Plane} objects used to selectively disable rendering on the outside of each plane. | |||
| * @param {Boolean} [options.enabled=true] Determines whether the clipping planes are active. | |||
| * @param {Matrix4} [options.modelMatrix=Matrix4.IDENTITY] The 4x4 transformation matrix specifying an additional transform relative to the clipping planes original coordinate system. | |||
| * @param {Boolean} [options.combineClippingRegions=true] If true, the region to be clipped must be included in all planes in this collection. Otherwise, a region will be clipped if included in any plane in the collection. | |||
| * @param {Boolean} [options.unionClippingRegions=true] If true, the region to be clipped must be included in all planes in this collection. Otherwise, a region will be clipped if included in any plane in the collection. | |||
There was a problem hiding this comment.
I think this is backwards. If it's a UNION, the value is clipped if it lies in any clipping plane. If it's an INTERSECT, the value is clipped only if it lies in all of the planes.
There was a problem hiding this comment.
Got it, my mistake, I'll swap the values. I think we should default to the same behavior though, so union should initially be false
…r compilation in IE
|
@hpinkos Updated! This also fixes the broken shader in IE (#6038 (comment)), but does not resolve the clipping plane behavior problem in IE. I'll open a new issue. |
|
Awesome, thanks @ggetz! All of the bugs I found seem to be fixed, and the code looks great. Does anyone else want to take a look before I merge this in? |
| * @see ClippingPlaneCollection#get | ||
| */ | ||
| ClippingPlaneCollection.prototype.contains = function(plane) { | ||
| return indexOf(this._planes, plane) > -1; |
| var planes = this._planes; | ||
| var index = indexOf(planes, plane); | ||
|
|
||
| if (index < 0) { |
|
@hpinkos Fixed the edge styling issue here as well. |
|
Great work @ggetz, thanks! |
Added collection methods to
ClippingPlaneCollection, and general clipping planes and plane geometry fixes.@hpinkos addresses the issues brought up in the last PR, including the bugs listed there.