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 for possible normalization issue for line corner points #12255

Merged
merged 6 commits into from
Oct 30, 2024

Conversation

p-skakun
Copy link
Contributor

@p-skakun p-skakun commented Oct 17, 2024

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.

cornerDirection = Cartesian3.normalize(cornerDirection, cornerDirection);

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.

function scaleToSurface(positions, ellipsoid) {

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

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

Copy link

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

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

Add reference to pull request into change.md
Copy link
Contributor

@ggetz ggetz left a comment

Choose a reason for hiding this comment

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

Thank you for the fix @p-skakun! The approach here all makes sense to me.

Could we please add a unit test for this case for both CorridorGeometryLibrary.computePositions and PolylineVolumeGeometryLibrary.computePositions? I know there are no existing tests, but adding even one to confirm this edge case would be a huge benefit.

If you are feeling generous, we would of course appreciate additional unit tests for those functions, but that is certainly not expected to get this PR merged. 😄

@p-skakun
Copy link
Contributor Author

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

@p-skakun p-skakun requested a review from ggetz October 28, 2024 15:12
@ggetz
Copy link
Contributor

ggetz commented Oct 29, 2024

Thanks @p-skakun! The CI test failure is due to #11958, not this PR.

@ggetz ggetz merged commit 6e6ed43 into CesiumGS:main Oct 30, 2024
4 checks passed
@p-skakun p-skakun deleted the fix-collinear-lines-corners branch October 30, 2024 14:03
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.

PolylineVolume corner normalization error.
2 participants