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

Image and image filter / mipmapping #40

Closed
pjcozzi opened this issue Mar 22, 2013 · 28 comments
Closed

Image and image filter / mipmapping #40

pjcozzi opened this issue Mar 22, 2013 · 28 comments

Comments

@pjcozzi
Copy link
Member

pjcozzi commented Mar 22, 2013

I'll have a spec proposal soon, but we need to be careful not to decouple images and image filters because this is not supported in WebGL and OpenGL ES without duplicating the image, which is painful on the client.

The fix could be as simple as the spec disallowing multiple textures to be bound at the same time with different filters. Otherwise, this requires GL_ARB_sampler_objects.

I'll think about it a bit more before proposing something...

@fabrobinet
Copy link
Contributor

Ok, I am curious why it shouldn't be handled with techniques/passes, I would prefer not adding too much concept especially if they can be handled generically.

@RemiArnaud
Copy link
Contributor

What I was saying too is that the surface information is important for the run-time, but the image indirection not so much.

In COLLADA it is important, as it is designed to enable decoupling the image format from the format in the surface.
For example the image may have a different size, texel format... But with a final format one can expect the images to have been pre-processed and correspond exactly to the format needed by the surface. So I think image can be removed from glTF and the reference to the data dIrectly stored in the surface.

@pjcozzi
Copy link
Member Author

pjcozzi commented Apr 17, 2013

Currently in WebGL, the sampler state (filters and repeat) is part of the texture object (however, they are separate in OpenGL ES 3.0). This means that we can't bind a texture twice, each with a different sampler state, for the same draw call. To do so, we have to duplicate the texture since the sampler state is part of the texture.

Currently, glTF decouples the image and the sampler state, making it possible to declare two parameters used in the same technique that use the same image, but different sampler state. Although this is a rare case, it puts an implementation burden on the client, who will have to detect this and duplicate the texture to create a conformant implementation.

To solve this, all I suggest is wording in the spec that explicitly disallows this for the WebGL 1.0.x profile. Assuming WebGL 2.0 adds sampler objects, the WebGL 2.0 profile would lift this restriction.

@RemiArnaud
Copy link
Contributor

yes, but also have the converted to duplicate automatically the texture.

On Wed, Apr 17, 2013 at 12:51 PM, Patrick Cozzi notifications@github.comwrote:

Currently in WebGL, the sampler state (filters and repeat) is part of the
texture object (however, they are separate in OpenGL ES 3.0). This means
that we can't bind a texture twice, each with a different sampler state,
for the same draw call. To do so, we have to duplicate the texture since
the sampler state is part of the texture.

Currently, glTF decouples the image and the sampler state, making it
possible to declare two parameters used in the same technique that use the
same image, but different sampler state. Although this is a rare case, it
puts an implementation burden on the client, who will have to detect this
and duplicate the texture to create a conformant implementation.

To solve this, all I suggest is wording in the spec that explicitly
disallows this for the WebGL 1.0.x profile. Assuming WebGL 2.0 adds sampler
objects, the WebGL 2.0 profile would lift this restriction.


Reply to this email directly or view it on GitHubhttps://github.com//issues/40#issuecomment-16531633
.

@fabrobinet
Copy link
Contributor

@pjcozzi ok, in my first comment I misunderstood what you meant here...

@fabrobinet
Copy link
Contributor

@pjcozzi weither you have or not sampler objects, you do not need to duplicate the texture you can change the filter and wrap mode for given texture on the fly OR duplicate the texture. It's a trade off & decision that has to be made on client side. It's easy to detect that different samplers are used for the same texture. In some situation when you have enormous images it's better to actually change the wrap mode, even if this happens each frames, but if you have reasonnably small images duplicating might be cleaner... It looks that you avoid the possibility of changing the wrap/filter mode on the fly but this does work (and even better with sampler objects)

@pjcozzi
Copy link
Member Author

pjcozzi commented Apr 18, 2013

AFAIK in WebGL 1.0, If you have one texture bound to two different texture units used in the same draw call, they must use the same sampler state because the sampler state is part of the texture.

This is one reason why GL_ARB_sampler_objects was introduced.

@fabrobinet
Copy link
Contributor

Ah yes, you're right. But I overlooked something else in my previous comment.
Different texture parameters for the shader can just point to the same image and have different wrap/filter mode.
Consequently, clients may then share the same image but create 2 different textures.

@pjcozzi
Copy link
Member Author

pjcozzi commented May 9, 2013

Consequently, clients may then share the same image but create 2 different textures.

Yes, this is exactly what I am saying, but we should make it easy for clients. This is a pain to have to implement; we've done it before for COLLADA. I suggest just explicitly disallowing this in the WebGL 1.0.x profile of glTF. I don't know of any models that do this in practice, so I don't see enough value in making clients think they have to support this. When GL_ARB_sampler_objects are supported, we can support this in a later profile.

@fabrobinet
Copy link
Contributor

The decision of duplicating or not the image is on the client side, and I don't think it is a pain to implement. You can just create a hascode with the sampler infos. If a client really wants to prevent re-applying wrap / filter mode then we probably need a textureID (as Remi suggested), and then we could decide in the converter if textures needs to be duplicated or not. Though, I am not sure yet how the schema will look...

@RemiArnaud
Copy link
Contributor

following the email discussion.
The 'images' indirection between an image ID and its path is not useful. In fact an image unique ID is better give by its path than by any other ID.

More to that point, I ran into a problem. Loading one glTF, and then another one. They had the same imageID, pointing to a different image. not using imageID and using the image path only fixed the problem immediately.

So I think that the spec should be changed to:

  • remove the 'images'. use the path directly when defining the texture
  • replace this indirection with a 'samplers' where the texture information is described. the material is very simplified now since all materials parameters contain 'type' and 'value'. there is no more special case for textures

before:

 "material.6": {
            "name": "artezin_bottle",
            "technique": "technique1",
            "techniques": {
                "technique1": {
                    "parameters": {
                        "ambient": {
                            "type": "FLOAT_VEC3",
                            "value": [
                                0.2,
                                0.2,
                                0.2
                            ]
                        },
                        "diffuse": {
                            "image": "image_0",
                            "magFilter": "LINEAR",
                            "minFilter": "LINEAR",
                            "type": "SAMPLER_2D",
                            "wrapS": "REPEAT",
                            "wrapT": "REPEAT"
                        },
                        "shininess": {
                            "type": "FLOAT",
                            "value": -1
                        }
                    }
                }
            }
        }
....
 "images": {
        "image_0": {
            "path": "artezin_bottle.jpg"
        },

after:

 "material.6": {
            "name": "artezin_bottle",
            "technique": "technique1",
            "techniques": {
                "technique1": {
                    "parameters": {
                        "ambient": {
                            "type": "FLOAT_VEC3",
                            "value": [
                                0.2,
                                0.2,
                                0.2
                            ]
                        },
                        "diffuse": {
                            "type": "SAMPLER_2D",
                            "value": "texture_0"
                        },
                        "shininess": {
                            "type": "FLOAT",
                            "value": -1
                        }
                    }
                }
            }
        }
....
 "textures": {
        "texture_0": {
            "path": "artezin_bottle.jpg"
            "magFilter": "LINEAR",
            "minFilter": "LINEAR",
            "wrapS": "REPEAT",
            "wrapT": "REPEAT"
        },

@fabrobinet
Copy link
Contributor

Thanks Remi.

I like most of your proposal.
I agree for the textures, it's probably good to unify uniform of type sampler the way you suggest.
There was also that thread we had about surface that I need to re-check, but this looks mostly good to me.

About the image indirection I have to disagree:

  • we envisioned that images could also be concatenated in a single blob, I don't see how to handle this without the indirection.
  • the fact that the same id can come up when you load another file is common for all resources (same for meshes...) so the "problem" cannot be resolved by putting the datas straight in all objects. Instead you should clear your map of images when you load a new scene and that should fix your problem, and you don't want to accumulate all these textures anyway. I.e I believe it's more a problem with your code than with the spec about images.
  • As an aside, the image (within texture) property will drop eventually to something more generic like "source" , it will still be an indirection and it will point to either a video id or an image id.

@tparisi
Copy link
Contributor

tparisi commented May 10, 2013

I agree with @fabricerobinet about image ID "leakage" between scenes: it
seems more like an implementation issue than a spec issue.

We should definitely allow for images to be packed ("concatenated"), this
is a very important high-performance web technique, not to mention a
long-established graphics programming trick.

Tony

On Fri, May 10, 2013 at 9:16 AM, Fabrice Robinet
notifications@github.comwrote:

Thanks Remi.

I like most of your proposal.
I agree for the textures, it's probably good to unify uniform of type
sampler the way you suggest.
There was also that thread we had about surface that I need to re-check,
but this looks mostly good to me.

About the image indirection I have to disagree:

  • we envisioned that images could also be concatenated in a single
    blob, I don't see how to handle this without the indirection.
  • the fact that the same id can come up when you load another file is
    common for all resources (same for vertices...) so the "problem" cannot be
    resolved by putting the datas straight in all objects. Instead you should
    clear your map of images when you load a new scene and that should fix your
    problem, and you don't want to accumulate all these textures anyway. I.e I
    believe it's more a problem with your code than with the spec about images.
  • As an aside, the image property will drop eventually to something
    more generic like "source" , it will still be an indirection and it will
    point to either a video id or an image id.


Reply to this email directly or view it on GitHubhttps://github.com//issues/40#issuecomment-17729611
.

Tony Parisi tparisi@gmail.com
CTO at Large 415.902.8002
Skype auradeluxe
Follow me on Twitter! http://twitter.com/auradeluxe
Read my blog at http://www.tonyparisi.com/
Learn WebGL http://learningwebgl.com/

Read my book! WebGL, Up and Running
http://shop.oreilly.com/product/0636920024729.do
http://www.amazon.com/dp/144932357X

@fabrobinet
Copy link
Contributor

@RemiArnaud @pjcozzi @tparisi I can move forward on the converter about this.
To sum up: The points where we agreed here is to have textures being a root property so that we can share them and also unify parameters and have parameter.value point to a texture. This way all parameters have same properties...cool.

I explained above why we should keep the image property, it should be seen more as a way to share images client side than an indirection (among other reasons above).. But eventually, texture should probably point to a "source" property that could be either an "image" or a "video" (still to be introduced in the spec). We will discuss this in another issue. (about source for media...).

@pjcozzi can you remind me what was your proposal ? I see you prefer to not allow to specify different filter for a given texture and the updated proposal does not (not easily, but still possible if developer really want to).
Looks like an alternativ to be more generic would be to create yet another object to hold filter and wrap mode but I would hold off on that for now.

@pjcozzi
Copy link
Member Author

pjcozzi commented Jun 7, 2013

I agree with @fabricerobinet about image ID "leakage" between scenes: it seems more like an implementation issue than a spec issue. We should definitely allow for images to be packed ("concatenated"), this is a very important high-performance web technique, not to mention a long-established graphics programming trick. Tony

👍

@pjcozzi can you remind me what was your proposal ? I see you prefer to not allow to specify different filter for a given texture and the updated proposal does not (not easily, but still possible if developer really want to).

Currently in WebGL, the sampler state (filters and repeat) is part of the texture object (however, they are separate in OpenGL ES 3.0). This means that we can't bind a texture twice, each with a different sampler state, for the same draw call. To do so, we have to duplicate the texture since the sampler state is part of the texture.

Currently, glTF decouples the image and the sampler state, making it possible to declare two parameters used in the same technique that use the same image, but different sampler state. Although this is a rare case, it puts an implementation burden on the client, who will have to detect this and duplicate the texture to create a conformant implementation.

To solve this, all I suggest is wording in the spec that explicitly disallows this for the WebGL 1.0.x profile. Assuming WebGL 2.0 adds sampler objects, the WebGL 2.0 profile would lift this restriction.

Looks like an alternativ to be more generic would be to create yet another object to hold filter and wrap mode but I would hold off on that for now.

Later, we would do this to mirror sampler_objects, which are in ES 3.0.

@fabrobinet
Copy link
Contributor

@pjcozzi actually, I was referring to the latest iteration based on Remi's feedback to introduce texture. As discussed above it would unify parameters. Then having the sampler right in the texture object solve the problem of having different samplers per texture.... but then, It will be harder to support sampler objects eventually. So maybe to make things right from the beginning we should add samplers (yet another property :()) and for WebGL 1.0 the file would be invalid (due to consistency check) if 2 samplers refer the same texture.

The other change a bit on side of this that I would like to introduce is to use source in texture instead of image, so that we could refer to either an image or video.

@ghost ghost assigned pjcozzi Jun 15, 2013
@fabrobinet
Copy link
Contributor

Just spent some time trying to put all this in JSON...
One major annoyance is to handle target and levels for textures.
In the following proposal default is for the base level (0).
It shows:

  • a 2d texture that refers to an image
  • a texture cube for which sources refer to the 6 images in the cube map. The proposed order should be specified in glTF SPEC.
  • a blank texture, to be used for render target purpose.
    note: samplers are included to get this example to make it more complete, but there are not detailed.
samplers" : {
  "sampler0" : {
    "minFilter" : ..
    "magFilter" : ..
    "wrapS" : ..
    "wrapT" : ..
  }
},

"textures" : {
  "texture0" : {
    "target" : "TEXTURE_2D",
    "internalFormat" : "RGBA",
    "format" : "RGBA"   
    "source" : "image_0",
    "sampler" : "sampler0"
  },
  "texture1" : {
    "target" : "TEXTURE_CUBE",
    "internalFormat" : "RGBA",
    "format" : "RGBA"   
    "sources" ["img1_negX","img1_posX","img1_negY","img1_posY","img1_negZ","img1_posZ"],
    "sampler" : "sampler1"
  },
  "texture2" : {
    "target" : "TEXTURE_2D",
    "internalFormat" : "RGBA",
    "format" : "RGBA",
    "width": 512,
    "height": 512,
    "sampler" : "sampler0"
  },
}

but... then what if someone wants to pass manually the mip levels?, I propose this:
note2: level is implicit starts at 0 and is increment for each entries in nextLevels

samplers" : {
  "sampler0" : {
    "minFilter" : ..
    "magFilter" : ..
    "wrapS" : ..
    "wrapT" : ..
  }
},

"textures" : {
  "texture0" : {
    "target" : "TEXTURE_2D",
    "internalFormat" : "RGBA",
    "format" : "RGBA",  
    "source" : "image_0",
    "nextLevels" : [ { source: "image_1_level1" }, { source: ""image_1_level2"}]
    "sampler" : "sampler0"
  },
  "texture1" : {
    "target" : "TEXTURE_CUBE",
    "internalFormat" : "RGBA",
    "format" : "RGBA",  
    "sources" ["img1_negX","img1_posX","img1_negY","img1_posY","img1_negZ","img1_posZ"],
        "nextLevels" : [ { sources : ["image_1_level1",...] }, { sources: ["image_1_level2",..]}]
    "sampler" : "sampler1"
  },
  "texture2" : {
    "target" : "TEXTURE_2D",
    "internalFormat" : "RGBA",
    "format" : "RGBA",
    "width": 512,
    "height": 512,
    "sampler" : "sampler0"
  },
}

@pjcozzi
Copy link
Member Author

pjcozzi commented Jul 1, 2013

I really like this direction. For specifying the mip levels:

Why the name nextLevels? Why not mipLevels?

Because we describe level 0 as the base level. miplevels would work too but it might be confusing if it includes every level but the base level.

Or even if mipLevels is provided then it can be all mip levels > and source/sources is not required.

having sources are needed regardless of the levels, they are required for texture cube, where you need 6 images even if you specify just one level for each of them.

Do we require that all mip levels be provided? I can look more carefully at the WebGL spec to see its requirements, > but since we can't render to a non-zero mip level in WebGL AFAIK I suspect we would want all mip levels.

Yes, If you provide mip levels, you need to provide them all till you reach 1x1 size otherwise the mip map chain will be considered incomplete.

How do we specify for the app to generate the levels ala generateMipmap?

I would that if filtering specify mip mapping AND there is no nextLevels or mipLevels (whatever we decide to call it), then it means that generate mipmap should be called by the client.

@pjcozzi
Copy link
Member Author

pjcozzi commented Jul 2, 2013

Concretely, I'm proposing something like this:

2D texture. No mipmapping:

  "texture0" : {
    "target" : "TEXTURE_2D",
    "internalFormat" : "RGBA",
    "format" : "RGBA"   
    "source" : "image_0",
    "sampler" : "sampler0"
  }

2D texture. Mipmapping:

  "texture0" : {
    "target" : "TEXTURE_2D",
    "internalFormat" : "RGBA",
    "format" : "RGBA",  
    "mipLevels" : [ "image_0_level0", "image_0_level1", "image_0_level2" ],
    "sampler" : "sampler0"
  }

2D texture. Client-side generated mipmaps:

  "texture0" : {
    "target" : "TEXTURE_2D",
    "internalFormat" : "RGBA",
    "format" : "RGBA"   
    "source" : "image_0",
    "sampler" : "sampler0",
    "generateMipmap" : true
  }

As for cube maps...

Cube map. No mipmapping.

  "texture1" : {
    "target" : "TEXTURE_CUBE",
    "internalFormat" : "RGBA",
    "format" : "RGBA"   
    "sources" ["img1_negX","img1_posX","img1_negY","img1_posY","img1_negZ","img1_posZ"],
    "sampler" : "sampler1"
  },

Cube map. Mipmapping.

  "texture1" : {
    "target" : "TEXTURE_CUBE",
    "internalFormat" : "RGBA",
    "format" : "RGBA"   
    "mipLevels" [["img1_negX_level0", "img1_negX_level1", "img1_negX_level2"],["img1_posX_level0", ...],["img1_negY_level0", ...],["img1_posY_level0", ...],["img1_negZ_level0", ...],["img1_posZ_level0", ...]],
    "sampler" : "sampler1"
  },

Cube map. Client-side mipmaps.

  "texture1" : {
    "target" : "TEXTURE_CUBE",
    "internalFormat" : "RGBA",
    "format" : "RGBA"   
    "sources" ["img1_negX","img1_posX","img1_negY","img1_posY","img1_negZ","img1_posZ"],
    "sampler" : "sampler1",
    "generateMipmap" : true
  },

My only questions here are:

  • This is not flexible enough to (client-side) generate mipmaps for some cube map faces and not others. I don't think this matters in practice.
  • I made mipLevels an array of array of strings - as opposed to an array of array of objects with source properties. I think this is cleaner, but was there a good reason to use an object instead of a string originally?

@fabrobinet
Copy link
Contributor

Thanks for putting this together Patrick.

I have 2 issues:

  • I find confusing the switch from source to mipLevels. (more comments about it in last question answered below)
  • based on my previous input I do not see why we need to specify generateMipMap , if mipLevels is not specified and a filter mode in the sampler specify mip mapping, then generateMipMap map should be called, simple.

This is not flexible enough to (client-side) generate mipmaps for some cube map faces and not others. I don't think >this matters in practice.

Same here, I agree with this trade-off

I made mipLevels an array of array of strings - as opposed to an array of array of objects with source properties. I >think this is cleaner, but was there a good reason to use an object instead of a string originally?

I was not particularly happy to have an object but removing source is a short cut, and is confusing because what you refer to is source per level and thus having an object is IMO more appropriate.
This short-cut imply that mipLevel == source which is wrong.

@pjcozzi
Copy link
Member Author

pjcozzi commented Jul 3, 2013

based on my previous input I do not see why we need to specify generateMipMap , if mipLevels is not specified and a filter mode in the sampler specify mip mapping, then generateMipMap map should be called, simple.

OK with me since it is minimal and complete, but this is likely to be a source of bugs. Users will just use the sampler without thinking to generate mipmaps. GL developers already do it all the time, and now we are making it really easy for them.

We need to document it prominently.

I find confusing the switch from source to mipLevels. (more comments about it in last question answered below)

Here's the code for it:

if (typeof texture.source !== 'undefined) {
// call texImage2D with level 0
} else {
    for each texture.mipLevels {
        // call texImage2D with each level
    }
}

Compared to the original proposal:

// call texImage2D with level 0
if (typeof texture.nextLevels === 'undefined) {
    for each texture.nextLevels {
        // call texImage2D with level
    }
}

The original is not as bad as I thought. Maybe rename nextLevels to additionalLevels?

As for string vs. object, if source is a string, and nextLevels is an array of sources, then why can't it be an array of strings?

@fabrobinet
Copy link
Contributor

As for string vs. object, if source is a string, and nextLevels is an array of sources, then why can't it be an array of > strings?

If we keep source in the base level which make it complete and handle the remaining levels in additionalLevels it will be fine with me. My biggest concern was to "jump" from source to `xxxLevels" for the base level depending if we have multiple levels or not...

So I could live with additionalLevels being strings but this will bring limitations as in reality a level is actually not just a single string.
I will give a pathological example to show why in theory this is wrong, but I admit this is far from being the typical use case: What if a user decide to use glTF to just initializes the size (width and height) of each additionalLevel ? (so that he can feed the mip maps individually at runtime with say some CPU generated content..)

Remember what a level is, in the base level ( == texture [*]) it's not just a source, if you do not specify a source you could specify a width and height, hence the need for an object in this case. This said, I admit the additionalLevels are a simpler type than the base type because internalFormat and format should not be respecified.

My point is to show that level != source and source is a sub part of level.

[*] I did this simplification to avoid an indirection right under texture, as in most case we'll just deal with the base level.

@pjcozzi
Does this make more clear my concern ?

@pjcozzi
Copy link
Member Author

pjcozzi commented Jul 5, 2013

@fabrobinet keeping it as an object is fine with me; I'm just trying to avoid lots of indirection as we've grown glTF into more levels of indirection that I expected and that our original charter called for. However, so far, it hasn't been a pain to implement.

Leave the object or leave the string and a later glTF version can allow either a string or an object with multiple properties.

@fabrobinet
Copy link
Contributor

Thank you, I will implement this right after the skinning...

@fabrobinet
Copy link
Contributor

done f4d8f40 . Removing the converter keyword then.

@fabrobinet
Copy link
Contributor

flagging as resolved.

@pjcozzi pjcozzi changed the title Image and image filter Image and image filter / mipmapping Apr 28, 2014
@fabrobinet
Copy link
Contributor

The WG agreed that providing images of mipmaps wasn't critical for V1.0 and that just specifying the flag for mipmapping generation would be enough. Watchers of the project are welcome to provide feedback about this decision.

@fabrobinet fabrobinet removed their assignment Mar 19, 2015
@pjcozzi pjcozzi removed this from the Spec 1.0 milestone Aug 27, 2015
@pjcozzi
Copy link
Member Author

pjcozzi commented Sep 16, 2015

@tparisi for the draft 1.0 spec add:

Mipmapping Implementation Note: When a sampler's minification filter (sampler.minFilter) uses mipmapping (NEAREST_MIPMAP_NEAREST, NEAREST_MIPMAP_LINEAR, LINEAR_MIPMAP_NEAREST, or LINEAR_MIPMAP_LINEAR), any texture referencing the sampler needs to have mipmaps, e.g., by calling GL's generateMipmap function.

Non-Power-Of-Two Texture Implementation Note: glTF does not guarantee that a texture's dimensions are a power-of-two. At runtime, if a texture's width or height is not a power-of-two, the texture needs to be resized so its dimensions are powers-of-two if the sampler the texture references:

  • Has wrapping mode (either sampler.wrapS or sampler.wrapT) equal to REPEAT or MIRRORED_REPEAT, or
  • Has a minification filter (sampler.minFilter) that uses mipmapping (NEAREST_MIPMAP_NEAREST, NEAREST_MIPMAP_LINEAR, LINEAR_MIPMAP_NEAREST, or LINEAR_MIPMAP_LINEAR).

@tparisi if you want to see the code in Cesium, see https://github.com/AnalyticalGraphicsInc/cesium/blob/master/Source/Scene/Model.js#L1476

tparisi added a commit that referenced this issue Sep 16, 2015
@tparisi tparisi closed this as completed Sep 16, 2015
javagl pushed a commit to javagl/glTF that referenced this issue Dec 9, 2021
…-feature-ids

Clarification for only using feature IDs
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants