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

Apple A12 (iPad, iOS) CubeMap Seams (IBL) bug #1464

Open
issacclee opened this Issue Dec 26, 2018 · 13 comments

Comments

Projects
None yet
7 participants
@issacclee
Copy link
Contributor

issacclee commented Dec 26, 2018

There is a bug with CubeMap Seams (IBL) on A12 and A12X chips (all new iPhone's and iPad's).
Simple project that replicates it: https://playcanvas.com/project/594045/overview/cubemapissue

How it looks (wrong):

iPad Pro A12X 1
iPad Pro A12X 2

How it should look:

PC 1
PC 2

@issacclee

This comment has been minimized.

Copy link
Contributor Author

issacclee commented Dec 26, 2018

Small update on the issue: on A12 chips app.graphicsDevice.useTexCubeLod is true, if only on them it is forced to false it makes a workaround.
So if engine could detect A12 chip to flip such switch, it will do the job for now.

@willeastcott

This comment has been minimized.

Copy link
Contributor

willeastcott commented Dec 28, 2018

Hey @guycalledfrank - do you have any idea why the new iOS devices are exhibiting this behavior? The proposed workaround worries me a bit - what happens when A13 arrives etc?

@guycalledfrank

This comment has been minimized.

Copy link
Contributor

guycalledfrank commented Dec 28, 2018

Well the proposed PR is what I suggested to Max who suggested it to author :D I tested it on 5S and didn't notice anything bad, so seems like a problem with newer models. I don't really know any elegant way to detect if cross-face cubemap interpolation is supported apart from literally drawing and reading back stuff. WebGL doesn't seem to define any strict rules about this behaviour.

@Maksims

This comment has been minimized.

Copy link
Contributor

Maksims commented Jan 2, 2019

Looks like useTexCubeLod flag was purely based on observation that devices with extTextureLod and less than 16 available textures per shader would support cubemap smooth seams. @guycalledfrank as mentioned there is no specific docs in WebGL specs about such behaviour, and seems like browser implementations don't expose any flags to probe it.
Which left it open to weird implementations by vendors - what exactly happening with A12 chip.
As this workaround does fixes this particular problem and makes all A12 chips render properly, I would assume it is working fix now without negative impacts, so worth merging.

If weird behaviours happen further, then proper manual test code have to be implemented, where it would render cubemap, and check on results as @guycalledfrank suggested. Reason @issacclee needs this fix, is that many their apps right now rendering wrong on new iPhones, and they rely on IBL a lot in their products.

@willeastcott

This comment has been minimized.

Copy link
Contributor

willeastcott commented Jan 2, 2019

OK, thanks for your input @Maksims and @guycalledfrank. I'll merge this for now. But it will require a more comprehensive fix at some point...

@terence-wang

This comment has been minimized.

Copy link

terence-wang commented Jan 10, 2019

Small update on the issue: on A12 chips app.graphicsDevice.useTexCubeLod is true, if only on them it is forced to false it makes a workaround.
So if engine could detect A12 chip to flip such switch, it will do the job for now.

There are still problems with this solution, such as when there is a skybox in the scene, and the system will make a serious error when there is a texture in the diffuse

image

@Maksims

This comment has been minimized.

Copy link
Contributor

Maksims commented Jan 10, 2019

@terence-wang it has not been deployed to Editor yet.
And you sure it comes from this change? Do you have simple project to replicate an issue?

@terence-wang

This comment has been minimized.

Copy link

terence-wang commented Jan 10, 2019

@terence-wang it has not been deployed to Editor yet.
And you sure it comes from this change? Do you have simple project to replicate an issue?

I encountered this problem while using the engine development project. In the editor I mounted the script on the root and changed this property to make the problem appear.

https://playcanvas.com/project/596015/overview/material-issue

@Maksims

This comment has been minimized.

Copy link
Contributor

Maksims commented Jan 11, 2019

I can confirm that issue happens with this fix too, but it is weird as on some scenes it is popping out, and on others it is not. So it is somewhat scene related. Looks like it did not initialise some textures for cubemap. Maybe it is not using useTexCubeLod flag everywhere properly, and in some places it does own logic ignoring the flag or something.

Basically, there needs to be a fix for A12 chips, and it seems like there is another logic that defines textures based on some other flag.

Everyone with new iPhones and iPads is affected with PBR (IBL).

@willeastcott

This comment has been minimized.

Copy link
Contributor

willeastcott commented Jan 11, 2019

Could it be because iOS devices only support up to 8 texture uniforms in a fragment shader (which I think is the minimum supported by the WebGL spec)? That limit's easy to hit if you are using up 6 with cubemap faces.

@Maksims

This comment has been minimized.

Copy link
Contributor

Maksims commented Jan 16, 2019

Tested it more.

@willeastcott you are right, it is because A12 chips have limited WebGL support, and reporting (webglreport.com) that only 8 texture units are supported in fragment shader, while hardware is capable of more. Making it very limited.
And there is a degradation of A12: they removed cubemap seams filtering.
Screw Apple :D

If useTexCubeLod is false, it will fallback to scenario where it uses separate textures for faces instead of cubemap, to emulate manual cubemap seams filtering. But that uses 6 out of available 8 textures. With one light with shadow on scene and one diffuse texture it is already barely fitting. If you add another texture to that material, A12 chip just will refuse to render it leading to error mentioned by @terence-wang.

Seems like best to revert such change, at least it will not throw errors, but IBL looks bad :(

If there is a way to do cubemap path but with fixed seams - that would fix everything. @guycalledfrank have to add on that.

@Maksims

This comment has been minimized.

Copy link
Contributor

Maksims commented Jan 16, 2019

Added a PR #1473 with a fix suggested by @guycalledfrank

@lmx99

This comment has been minimized.

Copy link

lmx99 commented Feb 11, 2019

How is it going on? @Maksims your fix cause some weird problem , as the image
image
And the normal
image
@willeastcott

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.