Skip to content
This repository has been archived by the owner on Dec 22, 2023. It is now read-only.

Add TextureLinearInterpolationTest #297

Merged
merged 1 commit into from
May 10, 2021
Merged

Add TextureLinearInterpolationTest #297

merged 1 commit into from
May 10, 2021

Conversation

lexaknyazev
Copy link
Member

Further addresses KhronosGroup/glTF-Sample-Assets#68.

Spheres should look alike. Filament and UX3D's fork of the glTF-Sample-Viewer pass.

@bghgary
Copy link
Contributor

bghgary commented Feb 22, 2021

I understand the goal of this test, but in practice, this isn't likely going to result any meaningful difference. Is there a specific scenario that passing this test will address?

@lexaknyazev
Copy link
Member Author

Consider these:

  • The difference depends on texture content and resolution. The test highlights the most obvious case.
  • Approximating transfer function in a fragment shader steals a few GPU cycles from something more substantial while the dedicated module stays idle.
  • OpenGL's mipmap generation produces incorrect results unless proper texture formats are used.
  • All modern GPUs and APIs have dedicated support for this feature. The extension for WebGL 1.0 is widely available.

@bghgary
Copy link
Contributor

bghgary commented Feb 22, 2021

I'm not arguing whether this is a valid test, whether implementation should do it right, or that doing it right increases performance. The question for me is how important it is to call out. It's not obvious to me what real use cases this will solve.

@donmccurdy
Copy link
Contributor

All modern GPUs and APIs have dedicated support for this feature. The extension for WebGL 1.0 is widely available.

Referring to EXT_sRGB? According to https://developer.mozilla.org/en-US/docs/Web/API/EXT_sRGB it's still missing from Edge and IE11, although testing in Edge on macOS does list the extension so perhaps that's out of date.

@lexaknyazev
Copy link
Member Author

The question for me is how important it is to call out.

It's one more feature test that engines may use to evaluate their implementations. Just like with other feature tests from this repo, engines may assess it as they see fit.

it's still missing from Edge and IE11

It's supported in the new Edge (Chromium-based) that also supports WebGL 2.0. IE11 doesn't even fully support WebGL 1.0, so we should ignore it.

Copy link
Member

@emackey emackey left a comment

Choose a reason for hiding this comment

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

There's a bit of discussion that looks like it trailed off, but I don't think there's harm in merging this, and it does highlight an effect we want folks to understand. Thanks @lexaknyazev!

@emackey emackey merged commit a73ddec into master May 10, 2021
@emackey emackey deleted the interpolation branch May 10, 2021 13:22
@donmccurdy
Copy link
Contributor

It's likely that many engines will fail this test, since it's designed to magnify the (usually minor) difference in an implementation choice common in WebGL for years. FWIW, I do not think that three.js will make any attempt to pass this test. It has never been identified as a user-facing issue, and is not as trivial as you might imagine to change now.

I think it is worth a bit more of a disclaimer on the readme for this file, or we risk giving the impression that glTF has not solved more basic color management issues, for readers not familiar with the details here.

@lexaknyazev
Copy link
Member Author

For Web-based engines that require WebGL 2.0 or WebGPU, the interpolation difference should be regarded as a legitimate issue rather than an "implementation choice".

@emackey
Copy link
Member

emackey commented May 12, 2021

@donmccurdy @bghgary Was I too hasty in merging this? I merged a bunch of PRs yesterday morning, but it seems this one is the most controversial.

Ecosystem conformance is very poor. I can migrate this model back onto a new branch if needed. Thoughts?

@donmccurdy
Copy link
Contributor

For Web-based engines that require WebGL 2.0 or WebGPU, the interpolation difference should be regarded as a legitimate issue rather than an "implementation choice".

Currently these represent a very small percentage of 3D experiences on the web. I guess I see this as a tradeoff: we are adding a row to https://github.com/cx20/gltf-test that will be mostly filled with "❌", and if we do that enough — keeping a lot of sample models here with legitimate technical or historical reasons engines would choose not to support them — it will reflect poorly on the ecosystem. On the other hand it's a useful sample for identifying a subtle issue.

Proposed disclaimer in #313.

@cx20
Copy link
Contributor

cx20 commented May 12, 2021

I have added this model to the gltf-test with ⚠️warning marks.

https://github.com/cx20/gltf-test#feature-test-models
image

@bghgary
Copy link
Contributor

bghgary commented May 12, 2021

@emackey

Ecosystem conformance is very poor. I can migrate this model back onto a new branch if needed. Thoughts?

I would have preferred that we didn't merge this until more implementers are on board, but it's not a big deal. FYI, Babylon.js is planning on fixing this.

@donmccurdy
Copy link
Contributor

We're finding that with WebGL 1.0 and EXT_sRGB, we cannot generate mipmaps, and get the following error:

[.WebGL-000075BE002A8600] GL_INVALID_OPERATION: Texture format does not support mipmap generation.

Is this a known/expected error? The situation in WebGL 1.0 seems much less practical than in WebGL 2.0 so far.

@lexaknyazev
Copy link
Member Author

Is this a known/expected error?

Yes. This feature was removed from WebGL 1.0 (although it's supported in the underlying renderers) to follow OpenGL ES 2.0 semantics.

We should expect WebGL 2.0 to be more broadly available with the upcoming OS updates.

@donmccurdy
Copy link
Contributor

I see, thank you!

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants