CesiumGS / cesium

An open-source JavaScript library for world-class 3D globes and maps :earth_americas:
https://cesium.com/cesiumjs/
Apache License 2.0
13.03k stars 3.51k forks source link

Fix for possible normalization issue for line corner points #12255

Closed p-skakun closed 1 month ago

p-skakun commented 1 month ago

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:

Author checklist

github-actions[bot] commented 1 month ago

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

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

p-skakun commented 1 month ago

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

ggetz commented 1 month ago

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