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

Fixing highp issue - Batch Tables #8805

Merged
merged 7 commits into from
Apr 29, 2020
Merged

Conversation

YVin3D
Copy link
Contributor

@YVin3D YVin3D commented Apr 27, 2020

Fixes #8311

@cesium-concierge
Copy link

cesium-concierge commented Apr 27, 2020

Thanks for the pull request @YoussefV!

  • ✔️ 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.
  • ❔ Unit tests were not updated.
    • Make sure you've updated tests to reflect your changes, added tests for any new code, and ran the code coverage tool.

Reviewers, don't forget to make sure that:

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

@YVin3D YVin3D changed the title fixing highp issue, testing using CI Fixing highp issue - Batch Tables Apr 27, 2020
@mramato
Copy link
Contributor

mramato commented Apr 27, 2020

@likangning93 mentions in the linked issue that we should roll back #7421, is that still the case?

@YVin3D
Copy link
Contributor Author

YVin3D commented Apr 28, 2020

@likangning93 mentions in the linked issue that we should roll back #7421, is that still the case?

@mramato Just addressed this in the latest commit. The answer turned out to be yes, but not exactly. Because #7421 was fairly old, it needed careful rolling back. Latest push should have fixed it. I needed to only remove code that was related to the high precision issue, which was a little involved

@likangning93
Copy link
Contributor

Screenshot_2020-04-28_18-00-23

👏👏👏

CHANGES.md Outdated Show resolved Hide resolved
Copy link
Contributor

@likangning93 likangning93 left a comment

Choose a reason for hiding this comment

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

Thanks @YoussefV! Just one small code comment.

Also, since the fix here is at the batch-table level it should also help anyone who's storing float attributes with primitives on mobile, can you also update CHANGES.md for that?

Specs/Scene/ShadowVolumeAppearanceSpec.js Show resolved Hide resolved
@likangning93 likangning93 merged commit 63dea9c into master Apr 29, 2020
@likangning93 likangning93 deleted the highp-batch-tables-fix branch April 29, 2020 18:41
@dennisadams dennisadams mentioned this pull request Jul 27, 2020
5 tasks
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.

Use highp sampler2D when using float textures for tables
4 participants