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

Fix polyline color issue involving duplicate positions #9676

Merged
merged 6 commits into from
Jul 19, 2021
Merged
Show file tree
Hide file tree
Changes from 3 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions CHANGES.md
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@
- Fixes an error with removing a CZML datasource when the clock interval has a duration of zero. [#9637](https://github.com/CesiumGS/cesium/pull/9637)
- Fixed the ability to set a material's image to `undefined` and `Material.DefaultImageId`. [#9644](https://github.com/CesiumGS/cesium/pull/9644)
- Fixed render crash when creating a `polylineVolume` with very close points. [#9669](https://github.com/CesiumGS/cesium/pull/9669)
- Fixed a bug in `PolylineGeometry` that incorrectly shifted colors when duplicate positions were removed. [#9676](https://github.com/CesiumGS/cesium/pull/9676)

### 1.83 - 2021-07-01

Expand Down
37 changes: 36 additions & 1 deletion Source/Core/PolylineGeometry.js
Original file line number Diff line number Diff line change
Expand Up @@ -309,10 +309,45 @@ PolylineGeometry.createGeometry = function (polylineGeometry) {
var j;
var k;

var removedIndices = [];
var positions = arrayRemoveDuplicates(
polylineGeometry._positions,
Cartesian3.equalsEpsilon
Cartesian3.equalsEpsilon,
false,
removedIndices
);

if (defined(colors) && removedIndices.length > 0) {
var removedArrayIndex = 0;
var length = colors.length;
var nextRemovedIndex = removedIndices[0];
var colorsScratch = [];
var ii;
j9liu marked this conversation as resolved.
Show resolved Hide resolved
if (colorsPerVertex) {
for (ii = 0; ii < length; ii++) {
if (ii === nextRemovedIndex) {
removedArrayIndex++;
nextRemovedIndex = removedIndices[removedArrayIndex];
} else {
colorsScratch.push(colors[ii]);
}
}
} else {
// If polyline is colored in segments, the color is counted
// if the endpoint hasn't been removed.
for (ii = 0; ii < length; ii++) {
if (ii + 1 === nextRemovedIndex) {
removedArrayIndex++;
nextRemovedIndex = removedIndices[removedArrayIndex];
} else {
colorsScratch.push(colors[ii]);
}
}
}

colors = colorsScratch;
}
j9liu marked this conversation as resolved.
Show resolved Hide resolved

var positionsLength = positions.length;

// A width of a pixel or less is not a valid geometry, but in order to support external data
Expand Down
75 changes: 53 additions & 22 deletions Source/Core/arrayRemoveDuplicates.js
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,8 @@ var removeDuplicatesEpsilon = CesiumMath.EPSILON10;
*
* @param {Array.<*>} [values] The array of values.
* @param {Function} equalsEpsilon Function to compare values with an epsilon. Boolean equalsEpsilon(left, right, epsilon).
* @param {Boolean} [wrapAround=false] Compare the last value in the array against the first value.
* @param {Boolean} [wrapAround=false] Compare the last value in the array against the first value. If they are equal, the last value is removed.
* @param {Array.<number>} [removedIndices=undefined] Store the indices that correspond to the duplicate items removed from the array, if there were any.
j9liu marked this conversation as resolved.
Show resolved Hide resolved
* @returns {Array.<*>|undefined} A new array of values with no adjacent duplicate values or the input array if no duplicates were found.
*
* @example
Expand All @@ -33,9 +34,25 @@ var removeDuplicatesEpsilon = CesiumMath.EPSILON10;
* new Cesium.Cartesian3(1.0, 1.0, 1.0)];
* var nonDuplicatevalues = Cesium.PolylinePipeline.removeDuplicates(values, Cartesian3.equalsEpsilon, true);
*
* @example
* // Returns [(1.0, 1.0, 1.0), (2.0, 2.0, 2.0), (3.0, 3.0, 3.0)]
* // removedIndices will be equal to [1, 3, 5]
* var values = [
* new Cesium.Cartesian3(1.0, 1.0, 1.0),
* new Cesium.Cartesian3(1.0, 1.0, 1.0),
* new Cesium.Cartesian3(2.0, 2.0, 2.0),
* new Cesium.Cartesian3(2.0, 2.0, 2.0),
* new Cesium.Cartesian3(3.0, 3.0, 3.0),
* new Cesium.Cartesian3(1.0, 1.0, 1.0)];
* var nonDuplicatevalues = Cesium.PolylinePipeline.removeDuplicates(values, Cartesian3.equalsEpsilon, true);
* @private
*/
function arrayRemoveDuplicates(values, equalsEpsilon, wrapAround) {
function arrayRemoveDuplicates(
values,
equalsEpsilon,
wrapAround,
removedIndices
) {
//>>includeStart('debug', pragmas.debug);
Check.defined("equalsEpsilon", equalsEpsilon);
//>>includeEnd('debug');
Expand All @@ -46,24 +63,41 @@ function arrayRemoveDuplicates(values, equalsEpsilon, wrapAround) {

wrapAround = defaultValue(wrapAround, false);

var storeRemovedIndices = defined(removedIndices);

var length = values.length;
if (length < 2) {
return values;
}

var i;
var v0;
var v0 = values[0];
var v1;

// We only want to create a new array if there are duplicates in the array (without considering wrapAround).
// As such, cleanedValues is undefined until it encounters the first duplicate, if it exists.
var cleanedValues;
var cleanedValuesDefined = false;
j9liu marked this conversation as resolved.
Show resolved Hide resolved

for (i = 1; i < length; ++i) {
v0 = values[i - 1];
v1 = values[i];
if (equalsEpsilon(v0, v1, removeDuplicatesEpsilon)) {
break;
if (!cleanedValuesDefined) {
cleanedValues = values.slice(0, i);
cleanedValuesDefined = true;
}
if (storeRemovedIndices) {
removedIndices.push(i);
}
} else {
if (cleanedValuesDefined) {
cleanedValues.push(v1);
}
v0 = v1;
}
}

if (i === length) {
if (!cleanedValuesDefined) {
if (
wrapAround &&
equalsEpsilon(
Expand All @@ -72,33 +106,30 @@ function arrayRemoveDuplicates(values, equalsEpsilon, wrapAround) {
removeDuplicatesEpsilon
)
) {
return values.slice(1);
if (storeRemovedIndices) {
removedIndices.push(values.length - 1);
}
values.length -= 1;
return values;
j9liu marked this conversation as resolved.
Show resolved Hide resolved
}
return values;
}

var cleanedvalues = values.slice(0, i);
for (; i < length; ++i) {
// v0 is set by either the previous loop, or the previous clean point.
v1 = values[i];
if (!equalsEpsilon(v0, v1, removeDuplicatesEpsilon)) {
cleanedvalues.push(v1);
v0 = v1;
}
}

if (
wrapAround &&
cleanedvalues.length > 1 &&
cleanedValues.length > 1 &&
equalsEpsilon(
cleanedvalues[0],
cleanedvalues[cleanedvalues.length - 1],
cleanedValues[0],
cleanedValues[cleanedValues.length - 1],
removeDuplicatesEpsilon
)
) {
cleanedvalues.shift();
if (storeRemovedIndices) {
removedIndices.push(values.length - 1);
}
cleanedValues.length -= 1;
}

return cleanedvalues;
return cleanedValues;
Copy link
Contributor

Choose a reason for hiding this comment

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

These two separate sections of code that handle wrapAround can be combined. It should be fine to check the first and last element of values rather than cleanedValues, the answer ought to be the same.

Even nicer would be to combine all the code into a single for loop, but only if it can be done cleanly.

}
export default arrayRemoveDuplicates;
35 changes: 35 additions & 0 deletions Specs/Core/PolylineGeometrySpec.js
Original file line number Diff line number Diff line change
Expand Up @@ -243,6 +243,41 @@ describe("Core/PolylineGeometry", function () {
).toBe(true);
});

it("createGeometry removes duplicate positions", function () {
j9liu marked this conversation as resolved.
Show resolved Hide resolved
var positions = [
new Cartesian3(1.0, 2.0, 3.0),
new Cartesian3(2.0, 2.0, 3.0),
new Cartesian3(2.0, 2.0, 3.0),
new Cartesian3(3.0, 2.0, 3.0),
new Cartesian3(4.0, 2.0, 3.0),
new Cartesian3(4.0, 2.0, 3.0),
new Cartesian3(4.0, 2.0, 3.0),
new Cartesian3(5.0, 2.0, 3.0),
];

var expectedPositions = [
new Cartesian3(1.0, 2.0, 3.0),
new Cartesian3(2.0, 2.0, 3.0),
new Cartesian3(3.0, 2.0, 3.0),
new Cartesian3(4.0, 2.0, 3.0),
new Cartesian3(5.0, 2.0, 3.0),
];

var line = PolylineGeometry.createGeometry(
new PolylineGeometry({
positions: positions,
width: 10.0,
vertexFormat: VertexFormat.POSITION_ONLY,
arcType: ArcType.NONE,
})
);

var numVertices = expectedPositions.length * 4 - 4;
expect(line.attributes.position.values.length).toEqual(numVertices * 3);
expect(line.attributes.prevPosition.values.length).toEqual(numVertices * 3);
expect(line.attributes.nextPosition.values.length).toEqual(numVertices * 3);
});

var positions = [
new Cartesian3(1, 2, 3),
new Cartesian3(4, 5, 6),
Expand Down
Loading