Geometry offset attribute #6607

hpinkos · 2018-05-21T21:33:10Z

Part of #6434

Added a new offsetAttribute per vertex attribute to CorridorGeometry, EllipseGeometry, PolygonGeometry and RectangleGeometry. This is a boolean attribute used to specify which vertices the offset should be applied to.
Added OffsetGeometryInstanceAttribute which has a Cartesian3 value used to apply an offset to geometry vertices
Added Primitive._appendOffsetToShader which modifies the shader to add the offset vector to each vertex where applyOffset is true

cesium-concierge · 2018-05-21T21:33:11Z

@hpinkos, thanks for the pull request! Maintainers, we have a signed CLA from @hpinkos, 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 am a bot who helps you make Cesium awesome! Contributions to my configuration are welcome.

🌍 🌎 🌏

hpinkos · 2018-05-22T16:33:56Z

Source/Scene/Primitive.js

+                var wc = primitive._boundingSphereWC[i];
+
+                //TODO: associate BS with instance.  Maybe be multiple BS for splitlongitude
+                var offset = primitive._batchTable.getBatchedAttribute(i, offsetIndex, new Cartesian3());


@bagnell I could use some suggestions for what to do here. Sometimes there are two bounding spheres for one geometry instance when it's split across the IDL. Indexing into getBatchedAttribute with i doesn't work in this case.

hpinkos · 2018-06-06T16:02:07Z

@bagnell This is ready for review. I still need to update the specs, but I'll do that after you take a closer look at this

bagnell · 2018-06-06T20:03:17Z

Source/Core/GeometryPipeline.js

+                var origBS = BoundingSphere.clone(boundingSphere, offsetBoundingSphereScratch1);
+                var offsetBS = BoundingSphere.clone(boundingSphere, offsetBoundingSphereScratch2);
+                offsetBS.center = Cartesian3.add(offsetBS.center, offset, offsetBS.center);
+                boundingSphere = BoundingSphere.union(origBS, offsetBS, boundingSphere);


Keep the original bounding sphere and have the primitive update them after creation. The union here could cause the bounding sphere to be huge and we could lose benefits from culling.

bagnell · 2018-06-06T20:04:30Z

Source/Core/GeometryPipeline.js

@@ -1986,6 +2006,26 @@ define([
            Cartesian3.pack(direction, currentAttributes.extrudeDirection.values, insertedIndex * 3);
        }

+        if (defined(applyOffsets)) {


Doesn't #6644 handle this?

No, since it's a boolean property we don't want to interpolate it. The value should always be 1.0 or 0.0. It took me forever to figure out the correct values here, I really don't think it can be generalized.

Why can't you interpolate it? If all vertices have a 0.0, the interpolation will be 0.0. If all vertices have 1.0, the interpolation will be 1.0, If the interpolation is between 0.0 and 1.0, it should be 1.0 so set it to Math.ceil(interpolatedValue).

Okay, I figured it out. It was just an epsilon problem that was causing the barycentric interpolation to go wrong

bagnell · 2018-06-06T20:06:27Z

Source/Core/GeometryPipeline.js

@@ -1949,7 +1965,11 @@ define([
    var p1Scratch = new Cartesian3();
    var p2Scratch = new Cartesian3();
    var barycentricScratch = new Cartesian3();
-    function computeTriangleAttributes(i0, i1, i2, point, positions, normals, tangents, bitangents, texCoords, extrudeDirections, currentAttributes, customAttributeNames, customAttributesLength, allAttributes, insertedIndex) {
+    var s0Scratch = new Cartesian2();


These aren't used.

bagnell · 2018-06-06T20:10:33Z

Source/Core/GeometryPipeline.js

@@ -2233,13 +2277,31 @@ define([
                }

                var offsetPoint = Cartesian3.add(intersection, offset, offsetPointScratch);
-                insertSplitPoint(p0Attributes, p0Indices, p0IndexMap, indices, i, p0);
-                insertSplitPoint(p0Attributes, p0Indices, p0IndexMap, indices, -1, offsetPoint);
+                if (defined(applyOffset)) {


Can you make something similar to computeTriangleAttributes or make it more general?

bagnell · 2018-06-06T20:11:06Z

Source/Core/OffsetGeometryInstanceAttribute.js

@@ -0,0 +1,151 @@
+define([
+    './Check',


Whitespace.

bagnell · 2018-06-06T20:11:58Z

Source/Core/OffsetGeometryInstanceAttribute.js

+    'use strict';
+
+    /**
+     * Value and type information for per-instance geometry attribute that determines if the geometry instance has a distance display condition.


This should be private since GeometryOffsetAttribute is.

bagnell · 2018-06-06T20:20:33Z

Source/Scene/Primitive.js

+            return vertexShaderSource;
+        }
+
+        var attr = 'attribute float batchId;\nattribute float applyOffset;';


After a new line in a string, continue it on the next line. Same below.

bagnell · 2018-06-06T20:25:14Z

Source/Scene/Primitive.js

+        var attr = 'attribute float batchId;\nattribute float applyOffset;';
+        var modifiedShader = vertexShaderSource.replace(/attribute\s+float\s+batchId;/g, attr);
+
+        var str = 'vec4 p = czm_computePosition();\n    p = p + vec4(czm_batchTable_offset(batchId) * applyOffset, 0.0);';


Is p always defined here? I would capture the variable name just to be safe.

I think it's always defined? At least we don't check if it's defined in any of the other shaders that use czm_computePosition

Sorry, I used the wrong term. The value returned by czm_computePosition is always defined (it must be since it's GLSL). I meant you should use regex captures to get the name of the variable, in this case p.

bagnell · 2018-06-06T20:29:14Z

Source/Scene/Primitive.js

+        }
+
+        if (result.length !== primitive._boundingSpheres.length) {
+            throw new DeveloperError('whoops');


Has this ever happened?

Not yet. I put this in here as I was debugging just to check, but I'll remove it before merging this into master

bagnell · 2018-06-06T20:33:45Z

Source/Scene/Primitive.js

+            var origBS = BoundingSphere.clone(boundingSphere, offsetBoundingSphereScratch1);
+            var offsetBS = BoundingSphere.clone(boundingSphere, offsetBoundingSphereScratch2);
+            offsetBS.center = Cartesian3.add(offsetBS.center, offset, offsetBS.center);
+            boundingSphere = BoundingSphere.union(origBS, offsetBS, boundingSphere);


This should take into account what the offset type is. If it's ALL, then the whole bounding sphere moves. If it's TOP, then it would be the union like you have here.

That's what the extend param to the function does

bagnell · 2018-06-06T20:34:04Z

Source/Scene/Primitive.js

+                    var modelMatrix = primitive.modelMatrix;
+                    var offset = properties.offset;
+                    if (defined(offset)) {
+                        boundingSphere.center = Cartesian3.add(Cartesian3.fromArray(offset, 0, offsetScratch), boundingSphere.center, boundingSphere.center);


Shouldn't this be the same as https://github.com/AnalyticalGraphicsInc/cesium/pull/6607/files#diff-74b9ecf8fb5af0a22b41c6d17e3a8aadR1892

bagnell · 2018-06-06T20:35:32Z

Source/Scene/PrimitivePipeline.js

@@ -45,6 +47,11 @@ define([
            var modelMatrix = instances[0].modelMatrix;

            for (i = 1; i < length; ++i) {
+                var offset = instances[i].attributes.offset;
+                if (defined(offset)) {
+                    toWorld = true;


This will break things that depend of changing the model matrix, like boxes and ellipsoids.

bagnell · 2018-06-06T20:39:32Z

I see 7 test failures with ClassificationPrimitive, GroundPrimitive and Primitive.

bagnell · 2018-06-06T20:41:06Z

@hpinkos That's all I have for now. I didn't run through and test all of the geometry. Let me know when this is ready again and I will.

hpinkos · 2018-06-07T20:50:42Z

Thanks for the feedback @bagnell! This is ready for a full review now

bagnell · 2018-06-08T19:42:12Z

Source/Scene/Primitive.js

+
+        var str = 'vec4 $1 = czm_computePosition();\n';
+        str += '    $1 = $1 + vec4(czm_batchTable_offset(batchId) * applyOffset, 0.0);';
+        modifiedShader = modifiedShader.replace(/vec4\s+([A-Za-z]+)\s+=\s+czm_computePosition\(\);/g, str);


Add 0-9 and _ to the variable regex.

bagnell · 2018-06-08T19:43:03Z

Source/Scene/PrimitivePipeline.js

@@ -274,6 +276,8 @@ define([
        var length = instances.length;
        var pickOffsets;

+        var offsetInstanceExtend;// = new Array(length);


Remove comment.

bagnell · 2018-06-08T19:44:38Z

Source/Core/GeometryOffsetAttribute.js

@@ -0,0 +1,18 @@
+define([
+    '../Core/freezeObject'


bagnell · 2018-06-08T19:45:44Z

@hpinkos Just those minor comments and this is ready to merge.

hpinkos · 2018-06-08T19:53:11Z

Thanks @bagnell! Ready

Geometry offset attribute

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature.

GPG key ID: B5690EEEBB952194

Verified
Learn about vigilant mode

54dbdc6

hpinkos added 2 commits May 22, 2018 12:28

Merge branch 'master' into offset-attribute

b8bb189

eslint

4854c0e

hpinkos commented May 22, 2018

View reviewed changes

hpinkos added 2 commits May 25, 2018 17:05

fix split longitude

da22f8e

Merge branch 'master' into offset-attribute

693e63e

hpinkos mentioned this pull request May 30, 2018

Polylines on terrain via Primitive API #6615

Merged

5 tasks

hpinkos added 5 commits June 4, 2018 16:44

Merge branch 'master' into offset-attribute

ec1a8f0

bounding sphere progress [ci skip]

d28aedc

cleanup [ci skip]

fa510bf

columbus view bounding sphere? [ci skip]

4cc715a

Merge branch 'master' into offset-attribute [ci skip]

2fe6d4a

bagnell reviewed Jun 6, 2018

View reviewed changes

Source/Core/OffsetGeometryInstanceAttribute.js Outdated

@@ -0,0 +1,151 @@

define([

'./Check',

Copy link

Contributor

bagnell Jun 6, 2018

Choose a reason for hiding this comment

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

Whitespace.

bagnell reviewed Jun 6, 2018

View reviewed changes

bagnell closed this Jun 6, 2018

bagnell reopened this Jun 6, 2018

hpinkos added 5 commits June 7, 2018 13:28

cleanup step 1 [ci skip]

f09cbf2

cleanup2 [ci skip]

995dee0

specs

932d870

more cleanup

ae14534

regex capture

07120b5

bagnell reviewed Jun 8, 2018

View reviewed changes

Source/Core/GeometryOffsetAttribute.js Outdated

@@ -0,0 +1,18 @@

define([

'../Core/freezeObject'

Copy link

Contributor

bagnell Jun 8, 2018

Choose a reason for hiding this comment

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

Whitespace

cleanup

96f15b7

bagnell merged commit f73a9f6 into master Jun 8, 2018

bagnell deleted the offset-attribute branch June 8, 2018 20:54

hpinkos mentioned this pull request Jun 12, 2018

Fix get geometry instance bounding sphere #6679

Merged

hpinkos mentioned this pull request Jun 27, 2018

Geometry offset attribute cleanup #6742

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Geometry offset attribute #6607

Geometry offset attribute #6607

hpinkos commented May 21, 2018 •

edited

Loading

cesium-concierge commented May 21, 2018

hpinkos May 22, 2018

hpinkos commented Jun 6, 2018

bagnell Jun 6, 2018

bagnell Jun 6, 2018

hpinkos Jun 7, 2018

bagnell Jun 7, 2018

hpinkos Jun 7, 2018

bagnell Jun 6, 2018

bagnell Jun 6, 2018

bagnell Jun 6, 2018

bagnell Jun 6, 2018

bagnell Jun 6, 2018

bagnell Jun 6, 2018

hpinkos Jun 7, 2018

bagnell Jun 7, 2018

bagnell Jun 6, 2018

hpinkos Jun 7, 2018

bagnell Jun 6, 2018

hpinkos Jun 7, 2018

bagnell Jun 6, 2018

bagnell Jun 6, 2018

bagnell commented Jun 6, 2018

bagnell commented Jun 6, 2018

hpinkos commented Jun 7, 2018

bagnell Jun 8, 2018

bagnell Jun 8, 2018

bagnell Jun 8, 2018

bagnell commented Jun 8, 2018

hpinkos commented Jun 8, 2018

Geometry offset attribute #6607

Geometry offset attribute #6607

Conversation

hpinkos commented May 21, 2018 • edited Loading

cesium-concierge commented May 21, 2018

Choose a reason for hiding this comment

hpinkos commented Jun 6, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bagnell commented Jun 6, 2018

bagnell commented Jun 6, 2018

hpinkos commented Jun 7, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bagnell commented Jun 8, 2018

hpinkos commented Jun 8, 2018

hpinkos commented May 21, 2018 •

edited

Loading