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

details property should be removed in favor of an extension #361

Closed
fabrobinet opened this issue Feb 28, 2015 · 8 comments
Closed

details property should be removed in favor of an extension #361

fabrobinet opened this issue Feb 28, 2015 · 8 comments

Comments

@fabrobinet
Copy link
Contributor

The details property provides ways for an engine who creates its own shaders to just use the input values of the parameters based on semantics & conventions.
A example of details is:

"details": {
    "commonProfile": {
        "extras": {
            "doubleSided": false
        },
        "lightingModel": "Blinn",
        "parameters": [
                       "ambient",
                       "diffuse",
                       "emission",
                       "light0Color",
                       "light1Color",
                       "light1ConstantAttenuation",
                       "light1LinearAttenuation",
                       "light1QuadraticAttenuation",
                       "light1Transform",
                       "light2Color",
                       "light2ConstantAttenuation",
                       "light2LinearAttenuation",
                       "light2QuadraticAttenuation",
                       "light2Transform",
                       "modelViewMatrix",
                       "normalMatrix",
                       "projectionMatrix",
                       "reflective",
                       "shininess",
                       "specular",
                       "transparency"
                       ],
        "texcoordBindings": {
            "ambient": "TEXCOORD_0",
            "diffuse": "TEXCOORD_0",
            "reflective": "TEXCOORD_0"
        }
    },
    "type": "COLLADA-1.4.1/commonProfile"
},
  1. A minor issue in the current export, the property commonProfile should be COLLADA-1.4.1/commonProfile to be consistent with type. But that's not a big deal as more important changes should be considered and with extensions type goes away.
  2. Then the details property shouldn't be tied to the COLLADA-1.4.1/commonProfile in the sense that glTF core spec shouldn't even be aware of it. But if that's a core property it should be specified along with the properties within it such as lightingModel, texcoordBindings...
    glTF aims to be generic just like OpenGL so while these informations are mandatory for developers using an engine who do not want/can't use shaders provided by glTF, high level concepts such as lightingModel should be specified in extensions where these concepts makes sense with regard to the tool or original asset (COLLADA, FBX...) who generated the asset.
  3. Finally the parameters property should be reworked to be provided as a dictionary, so that a mapping is provided to rely the concept defined for a given parameter named as transparency in COLLADA-1.4.1/commonProfile and named opacity in glTF 0.9 see Trouble converting Agisoft PhotoScan model #339

Consequently the specification of COLLADA-1.4.1/commonProfile supported parameters will have to take place in a separate extension and not in core glTF and sounds that's more sane to me.
Other converters(FBX...) may then specificy in separate extensions in the same way which properties where used to generate the provided shaders - so they can possibly be re-created.

The proposal is to then to replace within the pass property, details by extensions as shown in the following example:

  • I removed the light properties, because the discussion about them will take place here: Light Semantics #93
  • The matrices parameters are currently exported but that's not needed.
"allExtensions" : ["COLLADA-1.4.1/commonProfile"],
"extensions": {
    "COLLADA-1.4.1/commonProfile": {
        "extras": {
            "doubleSided": false
        },
        "lightingModel": "Blinn",
        "parameters": {
                       "ambient" : "ambient",
                       "diffuse" : "diffuse",
                       "emission" : "emission",
                       "reflective" : "reflective",
                       "shininess" : "shininess",
                       "specular" : "specular",
                       "transparency" : "opacity"
                       },
        "texcoordBindings": {
            "ambient": "TEXCOORD_0",
            "diffuse": "TEXCOORD_0",
            "reflective": "TEXCOORD_0"
        }
    }
}
@fabrobinet
Copy link
Contributor Author

@pjcozzi @tparisi @RemiArnaud what do you think ?

@pjcozzi
Copy link
Member

pjcozzi commented Feb 28, 2015

I agree completely with the intension here. As of now, I don't use this in Cesium so I don't have any firsthand experience with it, but this looks reasonable to me.

Minor typo:

"allExtensions" : "[COLLADA-1.4.1/commonProfile]"

should be:

"allExtensions" : ["COLLADA-1.4.1/commonProfile"]

@fabrobinet
Copy link
Contributor Author

Thanks for the feedback, typo fixed.

@pjcozzi
Copy link
Member

pjcozzi commented Sep 1, 2015

Adding discussion from #240. @fabrobinet said:

About doubleSided it would deserve a separate issue...
material is one right place, it is how 3dsmax and other packages deal with that. AFAIK Maya uses it at the geometry level, but there is no absolute best way about exposing double sided information - that's probably why it was never specified in COLLADA. But this led to a big fragmentation of assets WRT to the way of handling double_sided, still the majority converged to extra within material... so I vote to take decision here and have double_sided at the material (technique) level. And do not forget it is just a hint in details , if you don't want to handle double_sided that way - you can with more indices & normals or put an extra in geometry - the purpose of this meta-information is just to be able to re-generate shaders....

More broadly, I'll probably change a bit the details property, to be not tied to a COLLADA profile so that we can put more information in there without having to use extra. For instance in lighting models WRT COLLADA spec & the lambert technique we can't have specular with common profile, I'd like to overcome these limitations...

@pjcozzi
Copy link
Member

pjcozzi commented Sep 1, 2015

Adding discussion from #97. @fabrobinet said:

the ability to associate a material property to texture.
Concretely, if one needs to regenarate a texture fetch for a specular map, he needs to know that say :TEXCOORD_2 is associated with the specular parameter that refer to a texture.
This will allow to write code such as `specularColor = texture2D(u_specularTexture, v_texcoord2).
Thus, This association "material property"(like specular) -> mesh attribute (TEXCOORD_2 in the example), is missing and this is tracked here:
#47

Initial converter implementation: 0445397

@pjcozzi
Copy link
Member

pjcozzi commented Sep 1, 2015

I think @fabrobinet proposal here is the right direction, but I am not the right person to take the lead on this since Cesium is not generating shaders from this description.

I can start tightening up the schema and figuring out how to integrate it with the spec, and then come back to this if someone, perhaps @tparisi or @kainino0x, doesn't get to it first.

Also keep in mind that we want to support PBR materials post 1.0.

@pjcozzi
Copy link
Member

pjcozzi commented Sep 17, 2015

I suggest we close this, and keep the material discussion in #84. @tparisi OK?

@pjcozzi
Copy link
Member

pjcozzi commented Sep 17, 2015

Closing since the design discussion is taking place in #84.

@pjcozzi pjcozzi closed this as completed Sep 17, 2015
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

2 participants