-
Notifications
You must be signed in to change notification settings - Fork 1.1k
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
Thoughts on tightening up the spec #563
Comments
Thanks @lexaknyazev! This is great feedback. I created a |
Let's say we have an "technique0": {
"attributes": {
"a_1": "param1",
"a_2": "param2"
},
"parameters": {
"param1": {
"type": 35665 // FLOAT_VEC3
},
"param2": {
"count": 3,
"type": 5126 // FLOAT
},
}
} |
Spec has a fixed list of valid attribute semantics. So how can we have custom attributes? |
I don't think the spec defines this yet (or at least not well), but perhaps only
Semantics are optional: https://github.com/KhronosGroup/glTF/tree/master/specification#techniqueparameters-1 So a custom attribute would need to rely on a well-known attribute name - not semantic - to conform to the spec as it is written now. I think the question should be: can we have custom semantics, right? I think the answer should be yes, but we need to think about how to do it in a forward-compatible way, and how important that would be in practice (e.g., if an asset uses a custom semantic and then a later version of glTF defines that semantic, but it is different than the custom one's meaning; given that the future glTF version could have other breaking changes, it might be OK). |
Please correct me if I'm wrong, we have: |
There is no way to specify texture's width and height. This is needed for using raw binary texture data (not web-compressed formats). |
Imho, the simpliest way to achieve forward-compatibility is to require all custom semantics to have some prefix (such as |
Looking at the extensions.json example I think we need to have some standardized way to define external extensions schemes. I see at least three possible approaches here:
And for extensions like |
After more digging, I propose to add optional
|
Looking at this again, I agree there is a disconnect here that we need to address in the spec. Will need to brainstorm if it is as simple as requiring the semantic.
That could work.
Each extension defines the JSON schema for the extra JSON properties, e.g., see https://github.com/KhronosGroup/glTF/blob/master/extensions/Vendor/CESIUM_RTC/CESIUM_RTC.schema.json from https://github.com/KhronosGroup/glTF/blob/master/extensions/Vendor/CESIUM_RTC/README.md What else are you looking for here?
This would be a bit of a magic value; I still think we need the extension explicitly defined even if it is quite short and simple.
This could work well. We should also consider it in the context of texture compression so we don't end of changing it again for that (additions would be fine, of course). |
Some extensions (like Do we really need glTF schemes for them? The lack of some sort of "fast lane" could slow down development of glTF implementations. |
Sorry for the slow response; I was at a conference all last week. We do need an extension (.md file) for them, but it can be very small and just reference, for example, the GL extension. In some cases, the extension will not require any new schema, in other cases, it may just be new allowed enums, and in other cases, it may be new properties/objects. |
Of course.
My point is that we need conventions for translation GL names into new properties, e.g., how |
Ah, yes, I agree throughout glTF, not just extensions. |
Does |
The spec is not explicit about this. Perhaps the 1.0.1 spec should change this from |
Makes sense. |
@lexaknyazev do you have a specific example for this? JSON schema actually allows us to define fields as integers, e.g., accessor. byteOffset. |
|
@lexaknyazev currently, this implies that a parameter with |
So it looks like there was a bug in collada2gltf some time ago:
I think spec can be explicit on that to prevent runtimes from somehow "converting" matrices. |
Thanks, updated the spec in f548e60. I will also cherry-pick this for master since it does not depend on the 1.0.1 spec. BTW, pull requests into the 1.0.1 branch for any issues are welcome; in the meantime, I am working through items in this issue in the next few days. |
I suspect this is because |
OK. (If only GLSL was so permissive...) |
No, but the spec wasn't explicit. Updated spec in the 1.0.1 branch: ff4ac97 |
See aa152d8. |
See 17d3145. |
After evaluating this, #563 (comment), more, I agree that this is the right direction; however, it will require a new version of the KHR_binary_glTF extension to take full advantage. I'd like to limit the scope of the 1.0.1 spec largely to improvements for validation (a few exceptions are OK, for example, #573). I feel like we might be better off holding off on this until glTF includes texture compression and then making all the changes at once: this, texture compression, and KHR_binary_glTF updates. What do you think? Do you have pressing use cases that should drive this happening sooner? |
Several points, actually:
|
Maybe we can establish some sort of framework for texture formats now to be able to add exact compression extensions later in non-destructive manner? |
And we shouldn't forget about |
@lexaknyazev are you up for proposing all these texture changes in a new issue (can still be labeled Also - one more use case for this: creating glTF materials procedurally at runtime. |
OK
I understand that extension has been ratified already. |
This extension is widely used in Cesium; it is the default. We could keep backwards-compatible code in the loader, but new assets won't work with older versions of Cesium, which would mean the converter would need another check box ( I would rather queue this extension spec change up with other potential changes for a v2. |
I originally said that this should be a glTF extension, #563 (comment). I started writing the extension spec, and I now agree that it is overkill. For the 1.0.1 spec, I suggest that we just allow This means that the client will need to enable the OES_element_index_uint WebGL extension, or divide the accessor up (and all that entails) if the extension is not supported. However, given that the extension is widely supported, I do not think we need to worry about the later in practice. What do you think? |
I think this
is a crucial point. Client has to know about uint indices, so I'd prefer to mention |
Yes, I thought about this too. This could be convenient for clients and avoids double-tasking Thoughts? |
+1 for separate list. The situation can become a bit more complex for GLSL extensions like |
I believe it's possible for |
Yes, exactly. To keep things organized, can you please start the proposal in a new issue? |
See #619. |
|
@lexaknyazev have all tasks in this issue been addressed or moved to other issues? If so, I will label this as |
I've found only one unaddressed issue in this thread: attribute parameters must have semantic, otherwise they will be unlinkable to accessors (comment). |
Lgtm |
Labeling this as resolved. Please submit future 1.0.1 spec issues in new issues. |
Updated in #826 |
Base spec
FLOAT
orFLOAT_VEC*
. I believe that a little permissive comment about that would be enough.MODELVIEWINVERSETRANSPOSE
andMODELINVERSETRANSPOSE
technique parameter semantics are declared asFLOAT_MAT3
. At the same time presence ofnode
field requires parameter to beFLOAT_MAT4
. Spec is silent on this ambiguity. (seeboxSemantics/glTF/boxSemantics.gltf
)technique.uniforms
nor intechnique.attributes
?technique.attributes
and have avalue
property at the same time?FLOAT
(or other scalar) type and"count": 3
be linked withFLOAT_VEC3
accessor? (More details in the comment below.){POSITION|NORMAL|...}_%d
semantics valid? Provided samples use onlyTEXTURE_%d
.mesh.primitive.indices
accessor to not to beFLOAT
and to referencebufferView
withELEMENT_ARRAY_BUFFER
target.a_texcoord_0
attribute.NORMAL
runtime can try to calculate them on-the-fly, but withoutPOSITION
there's nothing to render at all.EXT_binary_glTF
instead ofKHR_binary_glTF
.OES_element_index_uint
?millennium_falcon.gltf
fromfr24-3d-models
has accessors with"componentType": 5125
(UNSIGNED_INT
) which isn't a valid value without extension.Added from comments below
"There is no way to specify texture's width and height. This is needed for using raw binary texture data (not web-compressed formats).", Thoughts on tightening up the spec #563 (comment), Thoughts on tightening up the spec #563 (comment)maximumAnisotropy
example, Thoughts on tightening up the spec #563 (comment)aspectRatio
default, Thoughts on tightening up the spec #563 (comment)KHR_binary_glTF
sceneSize
to be > 0.KHR_materials_common
falloffAngle
isPI/2
in spec butPI
inlight.spot.schema.json
.WEB3D_quantized_attributes
1. ShoulddecodedMax
anddecodedMin
array lengths be aligned with accessor'stype
?2. Can we enforcedecodeMatrix
length to be just one of [4, 9 16] instead of [1-16]?Moved to #610
Original comment:
Base spec
FLOAT
orFLOAT_VEC*
. I believe that a little permissive comment about that would be enough.MODELVIEWINVERSETRANSPOSE
andMODELINVERSETRANSPOSE
technique parameter semantics are declared asFLOAT_MAT3
. At the same time presence ofnode
field requires parameter to beFLOAT_MAT4
. Spec is silent on this ambiguity. (seeboxSemantics/glTF/boxSemantics.gltf
)technique.uniforms
nor intechnique.attributes
?technique.attributes
and have avalue
property at the same time?Can technique parameter withMore details in the comment below.FLOAT
(or other scalar) type and"count": 3
be linked withFLOAT_VEC3
accessor?{POSITION|NORMAL|...}_%d
semantics valid? Provided samples use onlyTEXTURE_%d
.mesh.primitive.indices
accessor to not to beFLOAT
and to referencebufferView
withELEMENT_ARRAY_BUFFER
target.a_texcoord_0
attribute.NORMAL
runtime can try to calculate them on-the-fly, but withoutPOSITION
there's nothing to render at all.EXT_binary_glTF
instead ofKHR_binary_glTF
.OES_element_index_uint
?millennium_falcon.gltf
fromfr24-3d-models
has accessors with"componentType": 5125
(UNSIGNED_INT
) which isn't a valid value without extension.KHR_binary_glTF
There is no requirement on
sceneSize
to be > 0.KHR_materials_common
Default value for
falloffAngle
isPI/2
in spec butPI
inlight.spot.schema.json
.WEB3D_quantized_attributes
decodedMax
anddecodedMin
array lengths be aligned with accessor'stype
?decodeMatrix
length to be just one of [4, 9 16] instead of [1-16]?The text was updated successfully, but these errors were encountered: