cesium icon indicating copy to clipboard operation
cesium copied to clipboard

Fix for possible normalization issue for line corner points

Open p-skakun opened this issue 1 year ago • 2 comments

Description

If points in PolylineVolume or Corridor are collinear, computation fails with error "Normalized result is not a number" in the line below, because sum of normalized forward and backward vectors is zero.

https://github.com/CesiumGS/cesium/blob/0e9a425b475cd3cfdd90f35e9cdbdda453e448d8/packages/engine/Source/Core/PolylineVolumeGeometryLibrary.js#L438

The cornerDirection variable is only used within the doCorner === true execution branch, so I moved it inside the branch for better scoping. From my understanding, the forward and backward projection checks within the doCorner condition exclude the computation of corners for collinear points. As a result, normalizing cornerDirection in this execution branch should not cause any errors.

Issue number and link

Fixes #12254

Testing plan

This issue is difficult to replicate. I encountered it by chance, with the data shown in Sandcastle example attached to related issue. I haven't yet found a way to generate synthetic data to reproduce the issue, mainly due to the scaleToSurface call, which modifies coordinates based on a specific location.

https://github.com/CesiumGS/cesium/blob/0e9a425b475cd3cfdd90f35e9cdbdda453e448d8/packages/engine/Source/Core/PolylineVolumeGeometryLibrary.js#L35

To test it, I followed this approach:

  • I created a simplified version of computePositions that omits the scaleToSurface call and other irrelevant operations. This resulted in a "Normalized result is not a number" error when passing a simple collinear line like [[0,0,0], [1,1,1], [2,2,2], [3,3,3]] as the positions input.
  • I also tested the problematic data locally, both before and after the changes. The error was resolved after applying the changes.

Author checklist

  • [x] I have submitted a Contributor License Agreement
  • [x] I have added my name to CONTRIBUTORS.md
  • [x] I have updated CHANGES.md with a short summary of my change
  • [x] I have added or updated unit tests to ensure consistent code coverage
  • [x] I have updated the inline documentation, and included code examples where relevant
  • [x] I have performed a self-review of my code

p-skakun avatar Oct 17 '24 15:10 p-skakun

Thank you for the pull request, @p-skakun!

:white_check_mark: We can confirm we have a CLA on file for you.

github-actions[bot] avatar Oct 17 '24 15:10 github-actions[bot]

I added a few tests, but the release-tests check is failing for an unrelated (at least, it appears that way) spec.

p-skakun avatar Oct 19 '24 14:10 p-skakun

Thanks @p-skakun! The CI test failure is due to https://github.com/CesiumGS/cesium/issues/11958, not this PR.

ggetz avatar Oct 29 '24 19:10 ggetz