-
Notifications
You must be signed in to change notification settings - Fork 3.5k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Clipping planes for models, tilesets, point clouds #5913
Clipping planes for models, tilesets, point clouds #5913
Conversation
@ggetz, thanks for the pull request! Maintainers, we have a signed CLA from @ggetz, so you can review this at any time. I noticed that CHANGES.md has not been updated. If this change updates the public API in any way, fixes a bug, or makes any non-trivial update, please add a bullet point to I am a bot who helps you make Cesium awesome! Thanks again. |
@lilleyse Can you take a look at this and make sure I'm on the right track? |
Sure, I will review soon. |
Source/Scene/Model.js
Outdated
gltf_colorBlend : createColorBlendFunction(model), | ||
gltf_numClippingPlanes: createNumClippingPlanesFunction(model), | ||
gltf_clipNormals: createClippingNormalsFunction(model), | ||
gltf_clipPositions: createClippingPositionsFunction(model) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we need to send over the clip positions and clip normals? Isn't sending just the plane fine?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I changed it to this and used dot(clipNormal, (positionWC.xyz - clipPosition)
, which cleared up a lot, but not all of the artifacting. I'll take another look once it's in eyespace.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If the eye-space plane approach works, we should go with that instead of eye space normals + positions. This would avoid the need to send two sets of arrays over to the shader.
Source/Scene/Model.js
Outdated
var plane = planes[i]; | ||
var localPosition = Cartesian3.add(Cartesian3.multiplyByScalar(plane.normal, plane.distance, scratchCartesian), model._boundingSphere.center, scratchCartesian); | ||
var positionWC = Matrix4.multiplyByPoint(model.modelMatrix, localPosition, scratchCartesian); | ||
packedPositions.push(positionWC); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Did you have any luck transforming the plane to eye space? I think this is the way to fix the artifacts that show up.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So defining the planes in model space in the API, then doing the transforms here and passing it to the shader in eyespace is all correct, right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think model space is not the way to go actually, as it causes some of the problems with tilesets. #5913 (comment)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
But converting to eye space here and then passing to shader is correct.
<meta charset="utf-8"> | ||
<meta http-equiv="X-UA-Compatible" content="IE=Edge,chrome=1"> <!-- Use Chrome Frame in IE --> | ||
<meta name="viewport" content="width=device-width, initial-scale=1, maximum-scale=1, minimum-scale=1, user-scalable=no"> | ||
<meta name="description" content="A sample BIM dataset rendered with 3D Tiles."> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fix description.
} | ||
|
||
function loadModel (url, height) { | ||
viewer.entities.removeAll(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also need to call viewer.scene.primitives.removeAll()
otherwise tilesets will continue to stay loaded.
To keep is simple, these cleanup lines can go in their own function called from within Cesium.knockout.getObservable(viewModel, 'exampleType').subscribe(
<td><select data-bind="options: exampleTypes, value: exampleType"></select></td> | ||
</tr> | ||
<tr> | ||
<td>+-x</td> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This name looks confusing. Just "x" is probably fine.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah the sandcastle example still needs updating. Should have put that on the TODO, sorry.
Source/Scene/Model.js
Outdated
@@ -3387,7 +3436,10 @@ define([ | |||
|
|||
uniformMap = combine(uniformMap, { | |||
gltf_color : createColorFunction(model), | |||
gltf_colorBlend : createColorBlendFunction(model) | |||
gltf_colorBlend : createColorBlendFunction(model), | |||
gltf_numClippingPlanes: createNumClippingPlanesFunction(model), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We usually prefer wording like gltf_clippingPlanesLength
.
Source/Scene/Model.js
Outdated
'uniform vec3 gltf_clipPositions[6]; \n' + | ||
'void main() \n' + | ||
'{ \n' + | ||
' gltf_clip_main(); \n' + |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The clipping planes code should not be run if there are no clipping planes. Put an if in here that checks gltf_numClippingPlanes > 0
Source/Scene/Model.js
Outdated
' vec4 positionWC = czm_inverseView3D * czm_windowToEyeCoordinates(gl_FragCoord); \n' + | ||
' vec3 clipNormal = vec3(0.0, 0.0, 0.0); \n' + | ||
' vec3 clipPosition = vec3(0.0, 0.0, 0.0); \n' + | ||
' for (int i = 0; i < 6; ++i) { \n' + |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For glsl we put curly braces on their own lines:
for (int i = 0; i < 6; ++i)
{
Source/Scene/Model.js
Outdated
' clipPosition = gltf_clipPositions[i]; \n' + | ||
' clipped = clipped || (dot(clipNormal, (positionWC.xyz - clipPosition)) > czm_epsilon7); \n' + | ||
' } \n' + | ||
' if (clipped) { discard; } \n' + |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Split this out into multiple lines:
if (clipped)
{
discard;
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same above for the break
.
@@ -521,6 +524,30 @@ define([ | |||
}, | |||
u_constantColor : function() { | |||
return content._constantColor; | |||
}, | |||
pc_numClippingPlanes : function() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same note about naming here. For consistency, use u_
like the other uniforms.
u_clippingPlanesLength
You could write a |
I think it would be fine to limit the max to 6 to start. However there should probably be a |
I'm pretty sure this is caused by the clipping plane being defined in local space. Each tile has its own local space so the clipping plane will clip differently for each tile. So either the clipping planes need to be defined local to the tileset or always be global. |
Also never too early to update |
Thanks for the review @lilleyse ! I will update this shortly. |
@lilleyse Cleaning up the Sandcastle example further, but other parts are updated. |
Cleaned up sandcastle as well, there's no way to render the planes without creating that new Plane primitive, correct? It looks like polygons/rectangles are meant to conform to the ellipsoid surface. |
Additionally, what's the plan for more arrays than 6 clipping planes? Do we use a texture? |
@lilleyse Can you look at this when you get the chance? |
Yeah, that's correct. |
If 6 is too small a range, we could increase the constant. Though if we get in the 10+ range a texture might be a good idea. We can wait until there is a use case for that. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The code looks really solid now and all the artifacts from before are gone.
@@ -0,0 +1,180 @@ | |||
<!DOCTYPE html> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Add an image for the demo.
<tr> | ||
<td>x</td> | ||
<td> | ||
<input type="range" min="-100" max="100" step="1" data-bind="value: xOffset, valueUpdate: 'input'"> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reset x, y, and z when a new model is selected. After playing around with the BIM tileset I switched to the airplane and couldn't see it because the values were just too big.
It might even be good to limit the range of these values per-model. The sliders are too shifty for the airplane model. Or just scale up the airplane if that's easier.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So setting the scale on the model causes the clipping planes to behave differently- It will not be relative to the model anymore and move around when the camera changes position. But only when the model is set to a scale other than 1. Do you know why that might be?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The scale component of the modelView3D
must be shifting the distance
portion of the plane in createClippingPlanesFunction
. Try removing the scale from the matrix, like: https://gamedev.stackexchange.com/questions/119702/fastest-way-to-neutralize-scale-in-the-transform-matrix
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Instead I inverted, muliplied by scale, and inverted back. Is that ok?
Source/Scene/Model.js
Outdated
var planes = model.clippingPlanes; | ||
var length = planes.length; | ||
var packedPlanes = new Array(length); | ||
for (var i = 0; i < length; ++i) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should probably be limited to 6. Check what happens if you have more than 6 clipping planes, there may be an error in setting the uniform.
* @name czm_clipPlanes | ||
* @glslFunction | ||
* | ||
* @param {vec4[]} clippingPlanes The array of planes used to clip, defined in in eyespace. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Typo: defined in in eyespace
* @param {vec4[]} clippingPlanes The array of planes used to clip, defined in in eyespace. | ||
* @param {int} clippingPlanesLength The number of planes in the array of clipping planes. | ||
*/ | ||
const int czm_maxClippingPlanes = 6; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Remove single space in front of the const
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The 6 will have to be defined somewhere in the JS side as well. Maybe this should be a property of UniformState
and AutomaticUniforms
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I checked the OpenGL documentation and it is fine to pass in more values than the uniform array holds, the extra are ignored. Ignore my other comments about limiting to 6. However it's still worth testing with more than 6 planes to confirm this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, passing in more than 6 just causes the additional planes to be discarded.
var planes = content._tileset.clippingPlanes; | ||
var length = planes.length; | ||
var packedPlanes = new Array(length); | ||
for (var i = 0; i < length; ++i) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Limit to 6 here as well.
@lilleyse Updated. |
@lilleyse Updated, just note #5913 (comment) |
@@ -0,0 +1,7 @@ | |||
/** | |||
* 6 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Provide a description here instead of just the number.
* 6 | ||
* | ||
* @name czm_maxClippingPlanes | ||
* @glslConstant |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Add @see czm_clipPlanes
Thanks @lilleyse, updated. The scaling works correctly for model and tilesets now, and the slider in the example is now smoother. However, it does not take into account |
It would be best if that worked. Is it possible to use Some more situations to try out are Columbus view, 2D, and orthographic camera. |
@lilleyse Updated, 2d/columbus view are not quite working, but we may want to address that in a later PR. |
Source/Scene/Model.js
Outdated
Cartesian3.multiplyByScalar(plane.normal, plane.distance, scratchCartesian); | ||
Matrix4.multiplyByPoint(scratchMatrix, scratchCartesian, scratchCartesian); | ||
scratchPlane.w = Cartesian3.dot(scratchPlane, scratchCartesian); | ||
packedPlanes[i] = scratchPlane.clone(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One last thing - instead of allocating the packedPlanes
array for each uniform call, store it as a property of Model and create the Cartesian4's in advance, and then clone directly into those. There may need to be some logic in update
to handle when the array length changes.
Source/Scene/Model.js
Outdated
@@ -339,6 +339,8 @@ define([ | |||
* @param {Number} [options.colorBlendAmount=0.5] Value used to determine the color strength when the <code>colorBlendMode</code> is <code>MIX</code>. A value of 0.0 results in the model's rendered color while a value of 1.0 results in a solid color, with any value in-between resulting in a mix of the two. | |||
* @param {Color} [options.silhouetteColor=Color.RED] The silhouette color. If more than 256 models have silhouettes enabled, there is a small chance that overlapping models will have minor artifacts. | |||
* @param {Number} [options.silhouetteSize=0.0] The size of the silhouette in pixels. | |||
* @param {Plane[]} [options.clippingPlanes=[]] An array of {@link Plane} used to clip the model. | |||
* @param {} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Remove this line
Ok, I added that to the roadmap: #5765 (comment) |
Thanks @lilleyse, updated for both models and point clouds since they followed the same pattern. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Two minor things, then this is ready to merge.
for (var i = 0; i < length; ++i) { | ||
this._packedClippingPlanes[i] = new Cartesian4(); | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Place this above the command list code. In general commands go last.
Source/Scene/Model.js
Outdated
var length = model.clippingPlanes.length; | ||
|
||
if (model._packedClippingPlanes.length !== length) { | ||
model._packedClippingPlanes= new Array(length); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Slight spacing tweak:
model._packedClippingPlanes = new Array(length);
Thanks @lilleyse made the fixes. |
Thanks @ggetz. Looking forward to the 3D Tiles traversal optimization next. |
var viewer = new Cesium.Viewer('cesiumContainer'); | ||
|
||
var defaultClippingPlanes = [ | ||
new Cesium.Plane(new Cesium.Cartesian3(-1, 0, 0), 0.0), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Throughout, add .0
to numbers that are intended to be floating point.
new Cesium.Plane(new Cesium.Cartesian3(0, 0, 1), 100.0) | ||
]; | ||
|
||
function loadTileset (url) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Throughout, no space before (
.
return tileset.clippingPlanes; | ||
} | ||
|
||
function loadModel (url) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It is an awkward design to have the load
functions return the clipping planes array.
In getObservable
, perhaps create a new
array and pass it to the load function. Basically defaultClippingPlanes
becomes a local.
@@ -18,6 +18,7 @@ Change Log | |||
* Fixed a 3D Tiles point cloud bug causing a stray point to appear at the center of the screen on certain hardware. [#5599](https://github.com/AnalyticalGraphicsInc/cesium/issues/5599) | |||
* Fixed removing multiple event listeners within event callbacks. [#5827](https://github.com/AnalyticalGraphicsInc/cesium/issues/5827) | |||
* Running `buildApps` now creates a built version of Sandcastle which uses the built version of Cesium for better performance. | |||
* Added `clippingPlanes` property to models and 3D Tilesets, which specify an array of planes to clip the object. [TODO]() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Put this in a new TODO
release that we can update before merging into master.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
models and 3D Tilesets
Explicitly name the Cesium classes.
@@ -54,6 +54,7 @@ define([ | |||
* @param {Property} [options.color=Color.WHITE] A Property specifying the {@link Color} that blends with the model's rendered color. | |||
* @param {Property} [options.colorBlendMode=ColorBlendMode.HIGHLIGHT] An enum Property specifying how the color blends with the model. | |||
* @param {Property} [options.colorBlendAmount=0.5] A numeric Property specifying the color strength when the <code>colorBlendMode</code> is <code>MIX</code>. A value of 0.0 results in the model's rendered color while a value of 1.0 results in a solid color, with any value in-between resulting in a mix of the two. | |||
* @param {Property} [options.clippingPlanes=[]] A property specifying an array of {@link Plane} used to clip the model. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It is currently limited to 6 planes, right? Document that here and throughout.
{ | ||
bool clipped = false; | ||
vec4 position = czm_windowToEyeCoordinates(gl_FragCoord); | ||
vec3 clipNormal = vec3(0.0, 0.0, 0.0); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Here and below, just: vec3(0.0)
- 0.0
will be assigned to each component.
|
||
for (int i = 0; i < czm_maxClippingPlanes; ++i) | ||
{ | ||
if (i >= clippingPlanesLength) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Will it ever be >
?
Just use ==
?
|
||
clipNormal = clippingPlanes[i].xyz; | ||
clipPosition = clippingPlanes[i].w * clipNormal; | ||
clipped = clipped || (dot(clipNormal, (position.xyz - clipPosition)) > czm_epsilon7); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why czm_epsilon7
? Either make it 0.0
or add a comment.
{ | ||
if (i >= clippingPlanesLength) | ||
{ | ||
break; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can't this early exist if clipped
is true? With WebGL 2 for sure, but does it still compile with WebGL 1, I think it should.
* @param {vec4[]} clippingPlanes The array of planes used to clip, defined in eyespace. | ||
* @param {int} clippingPlanesLength The number of planes in the array of clipping planes. | ||
*/ | ||
void czm_clipPlanes (vec4[czm_maxClippingPlanes] clippingPlanes, int clippingPlanesLength) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Name this a verb, e.g., discardIfClipped
.
First bit of clipping plane implementation. Added the API for models, batch tilesets, and point clouds and a quick sandcastle example to demo. Shader code is added in
model
andPointCloud3DTileContent
.TODO: