-
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
Materials on Ground Primitives #6393
Conversation
…acy but still jittery
…ne and spherical texcoords mode
@likangning93, thanks for the pull request! Maintainers, we have a signed CLA from @likangning93, so you can review this at any time.
I am a bot who helps you make Cesium awesome! Contributions to my configuration are welcome. 🌍 🌎 🌏 |
Aha, texture coordinate rotations don't work yet. Added to TODO:
|
At a high level, the code looks good. Let me know when this is ready for a full review. |
@@ -28,6 +28,76 @@ | |||
'use strict'; | |||
//Sandcastle_Begin | |||
var viewer = new Cesium.Viewer('cesiumContainer'); | |||
|
|||
var instance = new Cesium.GeometryInstance({ |
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 clean git history, please don't submit temp code to Sandcastle's Hello World (note that I use to do exactly this to the default Cesium app years ago)
<meta http-equiv="X-UA-Compatible" content="IE=edge"> | ||
<meta name="viewport" content="width=device-width, initial-scale=1, maximum-scale=1, minimum-scale=1, user-scalable=no"> | ||
<meta name="description" content="Use Viewer to start building new applications or easily embed Cesium into existing applications."> | ||
<meta name="cesium-sandcastle-labels" content="Beginner, Showcases"> |
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.
Should this be a "developer" Sandcastle example?
Also do not use abbreviations in file names.
@@ -0,0 +1,136 @@ | |||
<!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.
Overall looks like these Sandcastle examples are all temp so please bump when you have the right set that you want to show off this new features.
LICENSE.md
Outdated
@@ -670,6 +702,20 @@ https://github.com/KhronosGroup/glTF-WebGL-PBR | |||
>CONTRACT, TORT OR OTHERWISE, ARISING FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE | |||
>OR THE USE OR OTHER DEALINGS IN THE SOFTWARE. | |||
|
|||
### ShaderFastLibs (adapted code) |
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.
What does "adapted code" mean? Can you point me to it?
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 it looks like you just ported to JavaScript and GLSL. All good. Just remove "(adapted code)" here, I think it will cause confusion.
Source/Core/Math.js
Outdated
* @returns {Number} An approximation of atan(x) | ||
* @private | ||
*/ | ||
CesiumMath.fastApproximateAtan = function(x) { |
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 could be public IMO like much of the rest of this class already is.
float fastApproximateAtan2(float x, float y) { | ||
// atan approximations are usually only reliable over [-1, 1], or, in our case, [0, 1] due to modifications. | ||
// So range-reduce using abs and by flipping whether x or y is on top. | ||
float opposite, adjacent, t; // t used as swap and atan result. |
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, and put the float
declaration with the assignment statements below.
* | ||
* @returns {vec2} Approximate latitude and longitude spherical coordinates. | ||
*/ | ||
vec2 czm_approximateSphericalCoordinates(vec3 normal) { |
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.
Please make sure that somewhere in the reference documentation very clearly states that textured polygons on terrain are for notional patterns not for precisely mapping textures to the ground - for that case, use the single imagery provider.
LICENSE.md
Outdated
|
||
https://github.com/mourner/quickselect | ||
|
||
No license given, used by rbush |
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 it would fall under the rbrush license, but submitted mourner/quickselect#5
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 is the license: https://github.com/mourner/quickselect/blob/master/LICENSE
} | ||
|
||
// Used in place of ternaries to trade some math for branching | ||
float branchFree(bool comparison, float a, float b) { |
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 this is generally useful to other shaders, perhaps put in its own file and rename to czm_branchFreeTernary
.
} | ||
}); | ||
|
||
function createShadowVolumeAppearanceShader(appearance, extentsCull, planarExtents) { |
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 shader looks pretty fast all things consider, nice. 🏃
It is, however, spread across a few different files with all sorts of string concatenation and preprocessor defines. Maybe that is the nature of the beast...or perhaps there is a way to make this more cohesive?
Is this my video card? Intel/Mac/Chrome. When I zoom in close (perhaps the polygon enters the first frustum), I get this with the initial polygon in http://localhost:8080/Apps/Sandcastle/index.html |
Good start, @likangning93. The precision issue is important and should be addressed before anything else - maybe it is just my video card which I think has other ground primitive artifacts - we can test. Otherwise, this just needs a lot of clean up, Sandcastle examples, tests, etc. that you already have in the task list. Please iterate with @bagnell and I will try to take one last look before you merge if I have time. Also looking forward to the blog post(s). |
2cb5132
to
a76cfdd
Compare
I did something very bad and tried to use |
a76cfdd
to
2cb5132
Compare
On the subject of picking: I think |
Yes, that's how it works. When we pick, we create an off-center frustum and scissor test only covering a 3x3 pixel area around the pick position so performance isn't terrible.
Sounds good .I'd be interested in the performance comparison to the current approach when there are a lot of volumes. |
This trick might be practical if we wanted to implement KML Ground Overlays though, since those let you do weird things like stretch the image based on the positions of a quad using the |
We definitely want projected imagery like latlonquad long term, but it's probably not worth worrying about until we actually go to do it. If you have notes that would be relevant to implementation; maybe write up a new issue so we don't forget about them. |
FYI, trapezoidal texturing will be based on this when we get a chance to do it: http://help.agi.com/AGIComponentsJava/html/BlogTrapezoidalTextureProjectionWithOpenGL.htm |
@bagnell: "don't do that" |
@bagnell I think this is finally ready for another look! Sorry for the churn. |
'./PrimitiveType' | ||
'./Matrix2', | ||
'./Matrix3', | ||
'./Matrix4', |
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 gang's all here...
* @private | ||
* | ||
* @param {Rectangle} boundingRectangle Rectangle object that the points will approximately bound | ||
* @param {Rectangle} textureCoordinateRotationPoints TODO: doc |
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.
There are a few TODO
s left in the doc.
}); | ||
|
||
it('handles shared material being invalidated with geometry', function() { | ||
if (!GroundPrimitive.isSupported(scene) | !GroundPrimitive.supportsMaterials(scene)) { |
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 throughout, change this to ||
@likangning93 Just those two comments and this is good to go. |
@bagnell ready! |
I think what might be going on is that the stencil state takes the tileset into account but the tileset's depth isn't getting into |
Getting this fully working with 3D Tilesets is going to involve messing with passes that I'd like to discuss with @lilleyse and @bagnell, let's put that off for a future PR. For this PR I'm going to restrict materials on Color classification |
…roundPrimitives by falling back to TERRAIN
@bagnell ready again |
@likangning93 can you open an issue for this so we don't forget? I think once people see this working with the globe they're going to want it for 3D Tiles. |
Fixes #5025
Opening a PR for some early review, and also to say this is "stable" if anyone wants to play with it.
TODO before full review:
This PR adds support for materials on ground primitives and also adds batching for non-overlapping, "similarly sized" ground entities with the same material. Quick rundown of how things work:
Materials on Ground Primitives
Normals and texture coordinates and the like are computed in the fragment shader, almost like a post-process. Normals are easy - sample the depth texture (which has to be enabled for this to work), do some eye space math, lovely.
Texture coordinates are done in either of two ways:
Batching
We couldn't batch ground entities before because of overdraw problems - the shadow volumes used to color terrain often extend well beyond the area they're supposed to cover in screen space and sometimes overlap in the camera plane, leading to artifacts.
This PR adds the ability to use texture coordinates to clip fragments outside a lat/long volume (rectangle) in space, preventing overdraw as long as the batched primitives' rectangles on the globe don't overlap.
Ground entities with per-instance color are no longer batched by color but based on overlap, so it's now possible to render ~10,000 uniquely colored non-overlapping ground entities in a scene on a mid-range mobile GPU like the GT 750M without crashing. More performance anecdotes to follow.
We may also want to discuss if batching everything that's batchable is a little naive, since in some cases we can actually suffer from loss of culling benefits (I think...).