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

improve attribute reliability for ground primitive materials #7421

Merged
merged 10 commits into from
Sep 23, 2019

Conversation

likangning93
Copy link
Contributor

@likangning93 likangning93 commented Dec 17, 2018

Fixes #6739.

Ground Primitive materials don't work the same across all devices I've tried and basically don't work at all on many mobile devices that claim support for all required extensions. This is most easily observed on iOS. There's more notes in the issue, but I'm pretty sure what's happening is that floating point textures are unreliable for large-magnitude values, which is bad because we pack encoded positions on the surface of the Earth to floats in the batch table.

The floats are all within very specific ranges, so this PR naively packs each to 4 uint8 values. It's very wasteful, but memory use is actually the same and the "unpack" op looks pretty cheap, so the only real negative here is the additional code and the salty, salty tears of any information theorists who might look in here.

Note that this doesn't fix depth-texture related bugs, but it should at least make substituting corridors for polylines on terrain possible on iOS devices. I'll have to open an issue to say that the feature is merely "buggy" instead of completely broken.

@cesium-concierge
Copy link

Thanks for the pull request @likangning93!

  • ✔️ 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.

I am a bot who helps you make Cesium awesome! Contributions to my configuration are welcome.

🌍 🌎 🌏

@likangning93 likangning93 requested a review from bagnell December 17, 2018 17:35
@likangning93
Copy link
Contributor Author

snafu, update CHANGES.md in the wrong place...

@bagnell
Copy link
Contributor

bagnell commented Dec 17, 2018

This change looks OK to me, but there is a large increase in the number of texture reads from the batch table. Can we detect the issue and fall back to this instead of using it as the default behavior?

@likangning93
Copy link
Contributor Author

Can we detect the issue and fall back to this instead of using it as the default behavior?

Sorry for the radio silence. Going to try adding a compute pass to check floating point texture precision, but I can't get to that for at least another few days.

@cesium-concierge
Copy link

Thanks again for your contribution @likangning93!

No one has commented on this pull request in 30 days. Maintainers, can you review, merge or close to keep things tidy?

I'm going to re-bump this in 30 days. If you'd like me to stop, just comment with @cesium-concierge stop. If you want me to start again, just delete the comment.

5 similar comments
@cesium-concierge
Copy link

Thanks again for your contribution @likangning93!

No one has commented on this pull request in 30 days. Maintainers, can you review, merge or close to keep things tidy?

I'm going to re-bump this in 30 days. If you'd like me to stop, just comment with @cesium-concierge stop. If you want me to start again, just delete the comment.

@cesium-concierge
Copy link

Thanks again for your contribution @likangning93!

No one has commented on this pull request in 30 days. Maintainers, can you review, merge or close to keep things tidy?

I'm going to re-bump this in 30 days. If you'd like me to stop, just comment with @cesium-concierge stop. If you want me to start again, just delete the comment.

@cesium-concierge
Copy link

Thanks again for your contribution @likangning93!

No one has commented on this pull request in 30 days. Maintainers, can you review, merge or close to keep things tidy?

I'm going to re-bump this in 30 days. If you'd like me to stop, just comment with @cesium-concierge stop. If you want me to start again, just delete the comment.

@cesium-concierge
Copy link

Thanks again for your contribution @likangning93!

No one has commented on this pull request in 30 days. Maintainers, can you review, merge or close to keep things tidy?

I'm going to re-bump this in 30 days. If you'd like me to stop, just comment with @cesium-concierge stop. If you want me to start again, just delete the comment.

@cesium-concierge
Copy link

Thanks again for your contribution @likangning93!

No one has commented on this pull request in 30 days. Maintainers, can you review, merge or close to keep things tidy?

I'm going to re-bump this in 30 days. If you'd like me to stop, just comment with @cesium-concierge stop. If you want me to start again, just delete the comment.

@likangning93
Copy link
Contributor Author

@cesium-concierge stop

@hpinkos
Copy link
Contributor

hpinkos commented Sep 18, 2019

@likangning93 what's the plan for this PR?

@likangning93
Copy link
Contributor Author

I'm planning on finishing this up in time for next release

@likangning93 likangning93 requested review from lilleyse and removed request for bagnell September 20, 2019 14:36
@likangning93
Copy link
Contributor Author

likangning93 commented Sep 20, 2019

@lilleyse this is ready for review. State of the changes are now:

  • app does a quick compute command on Context construction to assess if float texture precision is sufficient
  • all code that works with high/low positions in the batch table for ground primitive materials can take either a path using values packed to uint8 or values in a conventional float texture

Sandcastle for testing

Compared to master

Copy link
Contributor

@lilleyse lilleyse left a comment

Choose a reason for hiding this comment

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

@likangning93 this change looks ok and at least gets us part of the way there for rendering ground primitive materials on these devices. Could you open an issue to eventually remove this workaround once hardware gets better?

Source/Renderer/Context.js Outdated Show resolved Hide resolved
Source/Renderer/checkFloatTexturePrecision.js Outdated Show resolved Hide resolved
Source/Shaders/CheckFloatTexturePrecisionFS.glsl Outdated Show resolved Hide resolved
@likangning93
Copy link
Contributor Author

@lilleyse updated!

@lilleyse
Copy link
Contributor

Thanks!

@lilleyse lilleyse merged commit 8bd1c61 into master Sep 23, 2019
@lilleyse lilleyse deleted the mtls-on-groundPrimitives-iOS branch September 23, 2019 21:19
@likangning93
Copy link
Contributor Author

likangning93 commented Sep 23, 2019 via email

@likangning93
Copy link
Contributor Author

Updated title for #6735 since the remaining iOS artifacts in both ground polylines and ground primitive materials have the same underlying cause.

@likangning93
Copy link
Contributor Author

@likangning93
Copy link
Contributor Author

@lilleyse from the continuation of the saga over in the webgl dev google group, looks like sampler2D precision is not covered by the precision highp float hint, looks like that was all we needed.

I'll open a PR to roll this back and do the precision updates in the batch table in time for the next release.

@mramato
Copy link
Contributor

mramato commented Oct 15, 2019

@likangning93 are there any other areas of the code where we were assuming highp for sample2D that should be updated as well?

@likangning93
Copy link
Contributor Author

Probably anywhere else we use floating point textures, I can take a look.

@mramato
Copy link
Contributor

mramato commented Oct 15, 2019

@likangning93 cool, maybe this will end up greatly improving compatibility across many devices 🤞 If it's something you aren't going to get to right away, please write up an issue so we don't forget about it.

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.

Materials for Ground Primitives don't work for small areas on newer mobile devices
6 participants