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 wall negative height #9022

Closed
wants to merge 12 commits into from
Closed

Conversation

baothientran
Copy link
Contributor

Fixes #4274

Currently there is a check to prevent maxHeights from being negative in the function removeDuplicates() in WallGeometryLibrary.js file. I remove it, and walls with negative height are rendered correctly. But with the old method when the first member of maximumHeights array is equal to 0.0, normal vector can't be calculated. It's because vertices of the geometry are clamped to the earth's surface first before being used to calculate normal. So when the vertices themselves are on the surface, they and the clamped values have the same coordinates, which leads to undefined normal vectors. for example, there is an exception thrown when running this Sandcastle example. I'm not sure if we should let maximumHeight == minimumHeight. But since it works with value other than 0.0 in the master branch, I think I should fix it as well.

For the undefined normal vector problem above, instead of clamping the top vertex to the earth surface, I use the bottom vertex to calculate normal. If the top and bottom positions have the same coordinates at the beginning (aka maximumHeight[0] == minimumHeights[0]), I just assign Y_Axis to normal, X_Axis to tangent, and Z_Axis to bitangent. If the top and bottom positions in the middle of the wall are overlapped, I just assigned the last valid normal to it. Same with tangent and bitangent. So that should prevent all of those vectors from having zero length, which leads to the error above. I had to cleanup the WallGeometry.createGeometry() method more than I expected to accommodate the changes better. Please let me know if you have a better way.

There is also a bug where texture coordinate seems to be larger than 1.0 for this sandcastle's example. This is the wall with checkerboard texture rendered in the master branch:

Screen Shot 2020-07-07 at 10 43 32 PM

This is the fix in the PR:

Screen Shot 2020-07-07 at 10 45 03 PM

@hpinkos @lilleyse Can you please take a look at it? Please let me know if there are anything I need to test

@baothientran baothientran requested review from lilleyse and hpinkos July 8, 2020 02:50
@cesium-concierge
Copy link

Thanks for the pull request @baothientran!

  • ✔️ Signed CLA found.
  • CHANGES.md was not updated.
    • If this change updates the public API in any way, please add a bullet point to CHANGES.md.

Reviewers, don't forget to make sure that:

  • Cesium Viewer works.
  • Works in 2D/CV.
  • Works (or fails gracefully) in IE11.

]),
maximumHeights: [40000.0, 60000.0, 60000.0, 50000.0],
minimumHeights: [60000.0, 60000.0, 60000.0, 50000.0],
})
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm not sure if maximumHeights should always be larger than minimumHeights. This seems to work in the master, so I assume I should test as well

@hpinkos
Copy link
Contributor

hpinkos commented Jul 9, 2020

Thanks @baothientran! I'll try to give this a review early next week

@lilleyse
Copy link
Contributor

@hpinkos will you be able to look at this PR soon?

@hpinkos
Copy link
Contributor

hpinkos commented Jul 17, 2020

@lilleyse, yes, this and a few other CesiumJS PRs are on my list to review

@hpinkos
Copy link
Contributor

hpinkos commented Jul 20, 2020

@baothientran thanks for the PR, but the amount of re-factoring you did along the way made this impossible to review. It's important that each PR only contain code changes related to a single issue instead of adding in changes to catch other things you find along the way.

I've opened a PR here to fix the negative height issue: #9041
I've also opened this issue related to the incorrect texture coordinate: #9042

It would be much appreciated if you could open a new PR for fixing #9042. Thanks!

@hpinkos hpinkos closed this Jul 20, 2020
@hpinkos hpinkos deleted the fix-wall-negative-height branch March 27, 2021 14:44
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.

Wall geometries with all negative heights do not render
4 participants