Skip to content
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

Merged
merged 44 commits into from
May 15, 2018
Merged

Conversation

likangning93
Copy link
Contributor

@likangning93 likangning93 commented Mar 30, 2018

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:

  • texcoord rotations
  • Support for 2D/CV
  • better picking?
  • fix log depth incompatibility?
  • cleanup/doc
  • tests
  • fix planar texture coordinates (see below)
  • IE fallback
  • update CHANGE.md, Sandcastle?
  • on 3D Tilesets (fallback)

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:

  • for entities large enough that the curve of the Earth is noticeable, a world position is computed from the depth texture and is then used to approximate the latitude/longitude at the fragment using spherical coordinates and approximate inverse trigonometry. This spherical coordinate is then compared with a spherical extent per-instance attribute to compute [0-1] range texcoords.
  • for small entities, we compute the distance from the fragment in eye space to a pair of planes, also in eye space, which are computed in the vertex shader from a double-precision position and a pair of offset vectors that form a bounding box in the local ENU coordinate system centered around the entity.

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...).

@cesium-concierge
Copy link

Signed CLA is on file.

@likangning93, thanks for the pull request! Maintainers, we have a signed CLA from @likangning93, 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 CHANGES.md and comment on this pull request so we know it was updated. For more info, see the Pull Request Guidelines.

⚠️ I noticed that a file in one of our ThirdParty folders (ThirdParty/, Source/ThirdParty/) has been added or modified. Please verify that it has a section in LICENSE.md and that its license information is up to date with this new version. Once you do, please confirm by commenting on this pull request.


I am a bot who helps you make Cesium awesome! Contributions to my configuration are welcome.

🌍 🌎 🌏

@likangning93
Copy link
Contributor Author

Aha, texture coordinate rotations don't work yet. Added to TODO:

viewer.entities.add({
    rectangle : {
        coordinates : Cesium.Rectangle.fromDegrees(-110.0, 20.0, -80.0, 25.0),
        material : '../images/Cesium_Logo_Color.jpg',
        stRotation : 10.0
    }
});

@bagnell
Copy link
Contributor

bagnell commented Apr 2, 2018

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({
Copy link
Contributor

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">
Copy link
Contributor

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>
Copy link
Contributor

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)
Copy link
Contributor

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?

Copy link
Contributor

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.

* @returns {Number} An approximation of atan(x)
* @private
*/
CesiumMath.fastApproximateAtan = function(x) {
Copy link
Contributor

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.
Copy link
Contributor

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) {
Copy link
Contributor

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
Copy link
Contributor

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

}

// Used in place of ternaries to trade some math for branching
float branchFree(bool comparison, float a, float b) {
Copy link
Contributor

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) {
Copy link
Contributor

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?

@pjcozzi
Copy link
Contributor

pjcozzi commented Apr 3, 2018

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

image

@pjcozzi
Copy link
Contributor

pjcozzi commented Apr 3, 2018

Not sure if this example is supposed to work, but "switch to color appearance" yields:

image

@pjcozzi
Copy link
Contributor

pjcozzi commented Apr 3, 2018

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).

@likangning93
Copy link
Contributor Author

I did something very bad and tried to use rebase to remove unintentional changes to Hello World... I'll fix this soon. Sorry!

@likangning93
Copy link
Contributor Author

likangning93 commented Apr 11, 2018

On the subject of picking:

I think ClassificationPrimitive used to handle picking for the limited batchable cases by rendering each geometry instance separately using offsets into the vertex buffers, which seems like something we don't want to do if we can avoid it.
Since we're enforcing non-overlap, I think we could also use the texcoord culling thing to get around this. @bagnell any thoughts?

@bagnell
Copy link
Contributor

bagnell commented Apr 11, 2018

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.

Since we're enforcing non-overlap, I think we could also use the texcoord culling thing to get around this.

Sounds good .I'd be interested in the performance comparison to the current approach when there are a lot of volumes.

@likangning93
Copy link
Contributor Author

likangning93 commented May 10, 2018

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 gx:latlonquad extension

@mramato
Copy link
Contributor

mramato commented May 10, 2018

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 gx:latlonquad extension

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.

@hpinkos
Copy link
Contributor

hpinkos commented May 10, 2018

FYI, trapezoidal texturing will be based on this when we get a chance to do it: http://help.agi.com/AGIComponentsJava/html/BlogTrapezoidalTextureProjectionWithOpenGL.htm

@likangning93
Copy link
Contributor Author

funny/stupid idea

@bagnell: "don't do that"

@bagnell bagnell mentioned this pull request May 10, 2018
4 tasks
@likangning93
Copy link
Contributor Author

@bagnell I think this is finally ready for another look! Sorry for the churn.

'./PrimitiveType'
'./Matrix2',
'./Matrix3',
'./Matrix4',
Copy link
Contributor Author

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
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There are a few TODOs left in the doc.

});

it('handles shared material being invalidated with geometry', function() {
if (!GroundPrimitive.isSupported(scene) | !GroundPrimitive.supportsMaterials(scene)) {
Copy link
Contributor

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 ||

@bagnell
Copy link
Contributor

bagnell commented May 11, 2018

@likangning93 Just those two comments and this is good to go.

@likangning93
Copy link
Contributor Author

@bagnell ready!

@lilleyse
Copy link
Contributor

I'm seeing some problems with ground primitives, textured or not, when overlaying over 3D Tiles. Classification Types demo:
3dtiles

@likangning93
Copy link
Contributor Author

I'm seeing some problems with ground primitives, textured or not, when overlaying over 3D Tiles. Classification Types demo:

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 globeDepth aka czm_globeDepthTexture. I'll take a deeper look in the morning.

@likangning93
Copy link
Contributor Author

likangning93 commented May 15, 2018

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 GroundPrimitives to terrain classification and have the Entity layer detect if a material is being used and automatically render assuming TERRAIN when encountering BOTH/CESIUM_3D_TILE.

Color classification BOTH/CESIUM_3D_TILE will fall back to original behavior.

@likangning93
Copy link
Contributor Author

@bagnell ready again

@bagnell bagnell merged commit 01c49d6 into CesiumGS:master May 15, 2018
@likangning93 likangning93 deleted the texcoordsOnTerrain branch May 15, 2018 19:07
@lilleyse
Copy link
Contributor

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.

@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.

@likangning93
Copy link
Contributor Author

#6636

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

GroundPrimitive support for other materials
7 participants