KhronosGroup / glTF-Sample-Viewer

Physically-Based Rendering in glTF 2.0 using WebGL
Apache License 2.0
1.28k stars 235 forks source link

No tiling GL_REPEAT supported #180

Closed MrDChristop closed 5 years ago

MrDChristop commented 5 years ago

Hi i notice that model textures that are tiled are clamped. Even if the gltf inside specifies GL_REPEAT as the texture wrapping factor. Its as if the code always uses GL_CLAMP for wrapping. Here is a test model for you to test. You can see the stretching at the end. This models show up fine in https://gltf-viewer.donmccurdy.com/ or the Babylon gltf viewer.

https://www.dropbox.com/s/62haofa3zbmis4q/Pillow.glb?dl=0

image

Thanks Dimi

MrDChristop commented 5 years ago

Ok i found the issue. The problem was that if a texture had its width != height (was not square) the code flagged it as a rectangle texture and the wrap mode was hardcoded to CLAMP. This is not accurate since even if the texture is not square the texture does not have to be a rectangle texture if its dimensions are power of two. I changed the code to reflect this in line 110 of webgl.js

if ((image.image.width != image.image.height) && !(this.isPowerOf2(image.image.width) && (this.isPowerOf2(image.image.height))))
{
    rectangleImage = true;
    generateMips = false;
}

further above in the same file i added the isPowerOf2 function

    isPowerOf2(n)
    {
        return n && (n & (n - 1)) === 0;
    }
ghost commented 5 years ago

This is not accurate since even if the texture is not square the texture does not have to be a rectangle texture if its dimensions are power of two.

I disagree with this sentance. rectangleImage simply stands for “not square” which is correctly represented by the check. If we want to change the check, we have to change the name or introduce an additional variable.

ghost commented 5 years ago

Logically, your fix seems to be good though!

MrDChristop commented 5 years ago

Since my fix is working. webgl can support non square power of two textures . Which in my experience are the majority of textures. The name in opengl rectangle fuctionslity currently stands for non power of two textures. This is why it was introduced for. Because originally opengl could not handle non power of two textures at all .Please accept my answer the code is too  limiting and wrong currently.DimiOn 1 Mar 2019 1:39 pm, Benjamin Schmithüsen notifications@github.com wrote:

This is not accurate since even if the texture is not square the texture does not have to be a rectangle texture if its dimensions are power of two.

I disagree with this sentance. rectangleImage simply stands for “not square” which is correctly represented by the check. If we want to change the check, we have to change the name or introduce an additional variable.

—You are receiving this because you authored the thread.Reply to this email directly, view it on GitHub, or mute the thread.

ghost commented 5 years ago

I implemented your fix (slightly different) in #181 which I'm going to merge now so this can be closed.