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

KHR_texture_transform shouldn't emulate sampler repeat options #8076

Merged
merged 6 commits into from
Aug 16, 2019

Conversation

emackey
Copy link
Contributor

@emackey emackey commented Aug 15, 2019

The KHR_texture_transform extension was looking at texture repeat options and trying to factor them into the transformed texture. This is not needed, as WebGL will do the correct thing with sampler repeat options regardless of whether the coordinates have been transformed or not.

Fixes #7916

Alternative solution to #8065

@cesium-concierge
Copy link

Thanks for the pull request @emackey!

  • ❌ Missing CLA.
  • 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.

@emackey
Copy link
Contributor Author

emackey commented Aug 15, 2019

Missing CLA??

@emackey
Copy link
Contributor Author

emackey commented Aug 15, 2019

Not sure if a unit test is needed. This is strictly a removal of code, there's nothing new to test.

However, I can demonstrate that the clamping and mirroring features work fine even with this code removed:

TexRotateTest_clamp.zip

TexRotateTest_mirror.zip

The original test model shows repeating, which is the 3rd option and the default.

@mramato
Copy link
Contributor

mramato commented Aug 15, 2019

@OmarShehata can you look into why @emackey's CLA is missing?

@lilleyse is this good with you?

@mramato
Copy link
Contributor

mramato commented Aug 15, 2019

Closes #8065

@emackey
Copy link
Contributor Author

emackey commented Aug 15, 2019

Just for visual reference, this is the original test model from #7916, which doesn't specify a sampler and thus ends up with the default REPEAT behavior. The texture coordinates are axis-aligned, but, the KHR_texture_transform extension introduces a small rotation, producing a slight diagonal edge.

The rotation reveals an area where the texture coordinates begin repeating. revealing block A1 and its neighbors.

Sampler_Repeat_sm

With the samplers set to CLAMP_TO_EDGE, the texture rotation reveals an area that appears smeared. This is intentional, from the texture coordinates being clamped along the edges after the rotation has taken place.

Sampler_Clamp_sm

Finally, the text model above with samplers set to MIRROR will show some reverse blocks, such as blocks G8 and F8 being shown from mirrored texture coordinates.

Sampler_Mirror_sm

The upshot of all this is, Cesium's implementation of the KHR_texture_transform extension was trying to emulate two of these options with shader code, but, the underlying WebGL sampler enums are still in effect from the main model, so it's safe to remove this code without losing the functionality.

@mramato
Copy link
Contributor

mramato commented Aug 16, 2019

This all sounds correct to me, but I'm not qualified to merge. @lilleyse or @likangning93 any thoughts here? Please merge if you are good with this.

@lilleyse
Copy link
Contributor

Yup this looks good to me. I'll merge after the last merge commit passes CI.

@lilleyse
Copy link
Contributor

Thanks @emackey!

@lilleyse lilleyse merged commit d6d7381 into master Aug 16, 2019
@lilleyse lilleyse deleted the tex-transform-repeat branch August 16, 2019 14:25
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.

Crash when loading glTF's KHR_texture_transform without a sampler
4 participants