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

Accessor is missing "normalized" property #344

Closed
glome83 opened this issue Jan 6, 2015 · 37 comments
Closed

Accessor is missing "normalized" property #344

glome83 opened this issue Jan 6, 2015 · 37 comments

Comments

@glome83
Copy link

glome83 commented Jan 6, 2015

Accessor has all other parameters needed by vertexAttribPointer except "normalized" (boolean).

This property should be optional and should default to false.

It should be documented that this property is ignored for floating-point formats or when using the accessor for vertex indices.

@pjcozzi
Copy link
Member

pjcozzi commented Jan 7, 2015

I can't find the discussion, but I believe we thought that we did not need this given it would not be widely used. @fabrobinet do you remember?

@fabrobinet
Copy link
Contributor

Yeah, I remember we didn't find a compelling use case - in the context of glTF - for this property.

@fabrobinet
Copy link
Contributor

it's also something that could be added later if it was proving to be useful / requested from developer use cases, so for v1.0 I don't feel we should add it.

@fabrobinet
Copy link
Contributor

@glome83 are you OK with this plan, or do you have a specific use case in mind ?

@glome83
Copy link
Author

glome83 commented Jan 7, 2015

If normalized is not added, to avoid confusion, it should be documented, if accessors that contain non float values are to be interpreted to be normalized or not.

That is, do (unsigned) bytes 0 and 255 mean 0.0 and 255.0 (not normalized) or 0.0 and 1.0 (normalized).

@pjcozzi
Copy link
Member

pjcozzi commented Jan 7, 2015

Unsigned bytes would be unsigned bytes. If we automatically normalized them, I would document it, but I don't think we need to document that we don't modify the values.

@glome83
Copy link
Author

glome83 commented Jan 7, 2015

Just that it's common practice to specify colors as unsigned bytes 0-255.
So people will expect bytes to normalized and it should be documented that they are not.

@pjcozzi
Copy link
Member

pjcozzi commented Jan 7, 2015

@fabrobinet is that common from a modeling tool export and conversion perspective?

@glome83
Copy link
Author

glome83 commented Jan 8, 2015

Well it's recommended by the OpenGL wiki to do so:
https://www.opengl.org/wiki/Vertex_Specification_Best_Practices#Attribute_sizes

@fabrobinet
Copy link
Contributor

It can be an option of the tool that actually generate the glTF to generate normalized attributes, but I don't see much value to carry unnormalized attributes in glTF - if what you want is normalized ones - as it will use some bandwidth through GL to actually normalize the data eventually...

@fabrobinet
Copy link
Contributor

The justification I could think of, is if for some reason you need on client side non normalized attributes but you want to send only normalized data for GL. To move forward & evaluate how useful this would be a concrete use case would help.

@glome83
Copy link
Author

glome83 commented Jan 8, 2015

I think you are missing the point. The whole reason that it's common practice to use normalized bytes instead of floats is that they save bandwith. rgba color in floats is 4 * 4=16 bytes. The same color using (normalized) bytes is 4 * 1=4 bytes.

If the "normalized" attribute is not supported, it should be documented, so people will not try to do this and will use floats instead (or use bytes and normalize manually in the vertex shader).

@fabrobinet
Copy link
Contributor

@glome83 Just make sure that there is no ambiguity - "normalized" means here, that the data will have to be normalized by GL, it doesn't stand to declare wether if the data is already normalized or not.

I completely got the fact that normalized bytes save bandwidth, and that's why I was commenting previously that I don't see why you would carry in your glTF asset not normalized attributes to later on have them normalized via the GL API (which would make me understand why you need the "normalized" attribute).

Maybe that's not what you were suggesting, but then as you can just store attributes normalized as bytes you don't need the "normalized" attribute. You attribute will straight go to your shaders working on normalized attributes - as your shader doesn't "care" wether glVertexAttribPointer was called with normalized true/false provided your attribute is already normalized...

That is also why I was mentioning, that it could be an option of the glTF converter to force the generation of normalized byte attributes so we can save bandwidth and that's a good improvement for the converter, but this doesn't have to impact the specification.

@RemiArnaud
Copy link
Contributor

Just looking at the semantic here:

An attribute of an object, especially if this
attribute name is an adjective or a past tense verb as in 'normalized' naturally provide information about the object itself.
So the interpretation made that normalized indicate that the object is normalized is logical.
The most difficult thing about specification is to avoid various interpretations, and i would certainly recommend tuning the spec in this case, thanks to early finding of ambiguity.

I an writing this without looking at the spec, so this is a theoretical suggestion l: 'normalized' should not be an attribute of the data, but an attribute of the shader input. 'normalize' as a present tense verb would tell that the input values need to be normalized for the shader to work.

-- Rémi

On Jan 8, 2015, at 1:55 PM, Fabrice Robinet [email protected] wrote:

@glome83 Just make sure that there is no ambiguity - "normalized" means here, that the data will to be normalized when send over to GL, it doesn't stand to declare wether if the data is already normalized or not.

I completely got the fact that normalized bytes save bandwidth, and that's why I was commenting previously that I don't see why you would carry in your glTF asset not normalized attributes to later on have them normalized via the GL API (which would make me understand why you need this the "normalized" attribute).

Maybe that's not what you were suggesting, but then as you can just store attributes normalized as bytes you don't need the "normalized" attribute. You attribute will straight go to your shaders working on normalized attributes - as your shader doesn't "care" wether glVertexAttribPointer was called with normalized true/false provided your attribute is already normalized...

That is also why I was mentioning, that it could be an option of the glTF converter to force the generation of normalized byte attributes so we can save bandwidth and that's a good improvement for the converter, but this doesn't have to impact the specification.


Reply to this email directly or view it on GitHub.

@fabrobinet
Copy link
Contributor

that's why I find confusing that normalized as an argument of glVertexPointerAttrib means "to be" normalized, that I added a note at the beginning of my previous comment.

@pjcozzi pjcozzi added this to the Spec 1.0 milestone Feb 16, 2015
@pjcozzi
Copy link
Member

pjcozzi commented Feb 16, 2015

@fabrobinet what is the conclusion here?

@fabrobinet
Copy link
Contributor

@pjcozzi I still don't believe we need to change the spec and add normalized at this point, but as improvements we could convert attributes to smaller representations it would be a converter option, as using bytes for normals (that will be upfront normalized during convertion process).

@pjcozzi
Copy link
Member

pjcozzi commented Feb 16, 2015

Agreed. You are welcome to submit a separate converter optimization issue for that.

@pjcozzi pjcozzi closed this as completed Feb 16, 2015
@mre4ce
Copy link

mre4ce commented Jul 19, 2016

It seems odd not to expose 'normalized' one way or the other. It's common to store vertex attributes as bytes to save memory while wanting normalized [0, 1] floating-point values in the shader. Arguably you could manually scale in the shader but then you'd have to know the attribute format which is undesirable.

@pjcozzi
Copy link
Member

pjcozzi commented Jul 21, 2016

@mre4ce @lexaknyazev we can add this in a backwards compatible way.

@mre4ce if this is urgent, we could consider it for 1.0.1; otherwise, it is no problem for the following version. What do you think?

@mre4ce
Copy link

mre4ce commented Jul 21, 2016

@pjcozzi It's not urgent for us.

@significant-bit
Copy link

There are more than two sides to this coin. Instead of a simple "normalized" boolean, how about something along these lines?

enum class VertexFetchMode
    {
    KEEP_FLOAT,
    KEEP_INT,
    NORMALIZE_INT_TO_FLOAT, // 127 (ubyte) -> 0.5 (and so on for other int types)
    CONVERT_INT_TO_FLOAT // 127 (any int type) -> 127.0
    };

KEEP_FLOAT is only for float values, and is the only valid fetch mode for floats.

Integer values must use one of the other fetch modes. KEEP_INT is for newer GLSL that supports integer attributes. NORMALIZE vs CONVERT map to the proposed "normalized" bool.

I'm also not in a hurry for this feature, but yes it's needed.

@lexaknyazev
Copy link
Member

@significant-bit Maybe, simple bool property could be enough for all cases:

KEEP_FLOAT: normalized doesn't matter, can be omitted from JSON, false by default is OK
NORMALIZE_INT_TO_FLOAT: "normalized": true
CONVERT_INT_TO_FLOAT: "normalized": false

For newer GL/GLSL profiles, I think, it would be more consistent to add {I,U}VEC{2,3,4} to accessor.type enum.

@mre4ce
Copy link

mre4ce commented Jul 21, 2016

I'm not a huge fan of the OpenGL setup with a size (number of components), type and normalized flag, in particular because various combinations are invalid. From a specification perspective I would prefer the Vulkan approach where a single VkFormat is used. While you could use an OpenGL sized internal format, that doesn't help to setup your vertex attribute pointers through glVertexAttribPointer and this is "gl"TF after all.

@lexaknyazev
Copy link
Member

@mre4ce At the moment, glTF accessors look like

"accessor_108": {
  "bufferView": "bufferView_658",
  "byteOffset": 119640,
  "byteStride": 12,
  "componentType": 5126, // GL_FLOAT constant
  "count": 3672, 
  "type": "VEC3" // glTF string enum
},

Are you talking about replacing type, componentType, and possible normalized with just format? I don't know if it's possible until glTF 2.0.

@pjcozzi What do you think?

@mre4ce
Copy link

mre4ce commented Jul 21, 2016

@lexaknyazev I think for now we should do what lines up best with OpenGL. I was just pointing out that I like the Vulkan approach of just a single format but that doesn't make it any easier to load the data using OpenGL.

@lexaknyazev lexaknyazev reopened this Jul 21, 2016
@lexaknyazev
Copy link
Member

For now, normalized seems to be the only way to have non-float normalized buffers when KHR_materials_common (or any other extension with its own shaders) is used.
It can also simplify decodeMatrix for WEB3D_quantized_attributes.

CC @tparisi @lasalvavida @mlimper

@lasalvavida
Copy link
Contributor

It can also simplify decodeMatrix for WEB3D_quantized_attributes.

Could you elaborate on that point? What about it could be simplified?

@lexaknyazev
Copy link
Member

Could you elaborate on that point? What about it could be simplified?

Could we omit storing 1/65535 in matrix elements, when normalized is true?

@pjcozzi
Copy link
Member

pjcozzi commented Aug 22, 2016

@lexaknyazev wanted to see this make 1.0.1. The only way I think that is reasonable is to still with a simple boolean flag that mirrors the WebGL API. @lexaknyazev what do you think?

As glTF is fully explored for Vulkan, this could, of course, change.

@lexaknyazev
Copy link
Member

Yep, just a bool flag for 1.0.1.

@lexaknyazev
Copy link
Member

@pjcozzi When normalized is true, should accessor.min/max values be pre-normalized or should we keep them as ints? In either case, spec should be clear about it.

@lexaknyazev
Copy link
Member

Also, normalized must be false when accessor is used for animation data.

@pjcozzi
Copy link
Member

pjcozzi commented Sep 7, 2016

When normalized is true, should accessor.min/max values be pre-normalized or should we keep them as ints?

My initial thought is that max/min should be the max and min of the actual values stored, not of the final values the GPU computed. I think that is the most intuitive. I'm open if there is a strong case otherwise.

I also agree the spec should be explicit.

Also, normalized must be false when accessor is used for animation data.

Good catch!

Please open a PR.

@lexaknyazev
Copy link
Member

My initial thought is that max/min should be the max and min of the actual values stored, not of the final values the GPU computed. I think that is the most intuitive. I'm open if there is a strong case otherwise.

Agree. For bounding boxes in case of quantized accessors we've got decodedMin/Max.

Please open a PR.

Sure.

@pjcozzi
Copy link
Member

pjcozzi commented Sep 7, 2016

Sounds good, please include in PR.

@pjcozzi
Copy link
Member

pjcozzi commented Jun 15, 2017

Updated in #826

@pjcozzi pjcozzi closed this as completed Jun 15, 2017
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

8 participants