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

Losing diffuse settings when using textures #726

Closed
richtr opened this issue Sep 22, 2016 · 12 comments
Closed

Losing diffuse settings when using textures #726

richtr opened this issue Sep 22, 2016 · 12 comments

Comments

@richtr
Copy link
Contributor

richtr commented Sep 22, 2016

The example in the spec for adding textures is as follows:

"materials": {
    "material-1": {
        "technique": "technique1",
        "values": {
            "diffuse": "texture_file2",
        },
        "name": "material_1"
    }
},

// ...

"textures": {
    "texture_file2": {
        "format": 6408,
        "internalFormat": 6408,
        "sampler": "sampler_0",
        "source": "file2",
        "target": 3553,
        "type": 5121
    }
}

A number of formats allow adding of both a texture map and separate diffuse vec4 values.

Is the only way to set textures in glTF to override one of the material attributes (e.g. diffuse)? How should we compensate for the loss of diffuse values from the input here?

Original model example:

Epona-obj.zip

screen shot 2016-09-22 at 5 06 10 pm

Same model converted to glTF (adding a texture overwrites the diffuse settings in the output):

Epona-gltf.zip

screen shot 2016-09-22 at 5 06 30 pm

@RemiArnaud
Copy link
Contributor

You need to create a proper diffuse texture. From the information it looks like you want to multiply the original texture with the scalar.
Or you can create your own shader, that would take in account both parameters.

Out of curiosity, what tool did you use to generate this gltf?

@javagl
Copy link
Contributor

javagl commented Sep 22, 2016

The file says

"generator": "Open Asset Import Library (assimp v3.3.202485390)"

The file seems to be invalid in various ways. The validator prints a whole lot of warnings and error messages. Most of them refer to accessors.min/max, and can be ignored here, but some of them are likely relevant:

            "path": "/textures/texture/sampler",
            "message": "Property must be defined."
...
            "path": "/materials/DefaultMaterial",
            "message": "When technique is undefined, values must be undefined too."

There are no techniques contained in the glTF. One could assume that it should use KHR_materials_common, but the file doesn't say so. So I rather wonder which viewer is used for this, and how it can swallow this file....

Attached is a glTF that uses a fully embedded representation (i.e. the mesh and the texture are embedded, together with some "default" shaders). But it's true: If you want to take a diffuse texture and a diffuse color into account, this will likely only be possible with custom shaders.

Epona_Exported.zip

@RemiArnaud
Copy link
Contributor

Time for conformance test of various exporters ?

On Thu, Sep 22, 2016 at 10:30 AM, Marco Hutter [email protected]
wrote:

The file says

"generator": "Open Asset Import Library (assimp v3.3.202485390)"

The file seems to be invalid in various ways. The validator
http://github.khronos.org/glTF-Validator/ prints a whole lot of
warnings and error messages. Most of them refer to accessors.min/max, and
can be ignored here, but some of them are likely relevant:

        "path": "/textures/texture/sampler",
        "message": "Property must be defined."

...
"path": "/materials/DefaultMaterial",
"message": "When technique is undefined, values must be undefined too."

There are no techniques contained in the glTF. One could assume that it
should use KHR_materials_common, but the file doesn't say so. So I rather
wonder which viewer is used for this, and how it can swallow this
file....

Attached is a glTF that uses a fully embedded representation (i.e. the
mesh and the texture are embedded, together with some "default"
shaders). But it's true: If you want to take a diffuse texture and a
diffuse color into account, this will likely only be possible with custom
shaders.

Epona_Exported.zip
https://github.com/KhronosGroup/glTF/files/487968/Epona_Exported.zip


You are receiving this because you commented.
Reply to this email directly, view it on GitHub
#726 (comment),
or mute the thread
https://github.com/notifications/unsubscribe-auth/AAIsuSvWc7w-L-vcrdBw2eV0AhbzbxDEks5qsrsYgaJpZM4KEAbr
.

@richtr
Copy link
Contributor Author

richtr commented Sep 22, 2016

So I rather wonder which viewer is used for this, and how it can swallow this file....

I have been fixing up the THREE.js glTF loaders to the point that they can successfully render this current conversion output and using the latest dev branch head in THREE.js will successfully render the converted glTF file attached to the original post above.

As you saw, the file was converted via the Open Asset Import Library (https://github.com/assimp/assimp). I haven't been running output through the glTF validator but it feels like that would be a good thing to do while continuing to developing glTF export within that convertor.

           "path": "/textures/texture/sampler",
           "message": "Property must be defined."
...
           "path": "/materials/DefaultMaterial",
           "message": "When technique is undefined, values must be undefined too."

Both sampler and technique (+ a few other things like shaders, skins, programs and animations) are not currently implemented in the assimp glTF import/export code. Since technique is not currently supported it sort of makes sense to pass values in the material (YMMV on this).

@lexaknyazev
Copy link
Member

When materials extension (such as KHR_materials_common or FRAUNHOFER_materials_pbr, both are drafts for now) is not used, material.technique must be defined, because material.values has no meaning without linked technique.

@pjcozzi
Copy link
Member

pjcozzi commented Sep 24, 2016

@richtr

I have been fixing up the THREE.js glTF loaders...

Thanks so much for your contributions!

Both sampler and technique (+ a few other things like shaders, skins, programs and animations) are not currently implemented in the assimp glTF import/export code. Since technique is not currently supported it sort of makes sense to pass values in the material (YMMV on this).

Do you know if there is a roadmap or tasklist of glTF features that still need to be implemented in assimp. We could help spread the word and I may even be able to find a student interested in working on it.

Also, do you have any glTF projects we could showcase with the master list: https://github.com/KhronosGroup/glTF#gltf-tools

@richtr
Copy link
Contributor Author

richtr commented Sep 27, 2016

I have been fixing up the THREE.js glTF loaders...

Thanks so much for your contributions!

No problem! FWIW, I have now rewritten the existing THREE.js glTF loader if anyone would like to review it: mrdoob/three.js#9786. Maybe I will follow up with a PR to this repo if/when that is merged to point /loaders/threejs in this repo to the new glTF loader in the THREE.js repo directly.

Do you know if there is a roadmap or tasklist of glTF features that still need to be implemented in assimp.

If a tasklist of glTF features missing from assimp did exist it would look something as follows :)

  • techniques
  • extensions (specifically KHR_material_common and KHR_binary_glTF)
  • animations
  • shaders
  • cameras
  • skins

I'm not aware of any roadmap for the features but I do know that the glTF import/export code in assimp is getting some much needed attention right now.

We could help spread the word and I may even be able to find a student interested in working on it.

Both of these things would be great! I think there are a lot of interesting things to work on wrt assimp code. If we can bring that library up to glTF v1.0 standards then we should be able to import from other formats in to glTF consistently and reliably.

Also, do you have any glTF projects we could showcase with the master list: https://github.com/KhronosGroup/glTF#gltf-tools

Not yet but I may have soon!

If anyone wants to spin any of this off in to other Github issues I would be happy to do that.

@pjcozzi
Copy link
Member

pjcozzi commented Sep 27, 2016

Maybe I will follow up with a PR to this repo if/when that is merged to point /loaders/threejs in this repo to the new glTF loader in the THREE.js repo directly.

Yes, please!

We could help spread the word and I may even be able to find a student interested in working on it.

Both of these things would be great! I think there are a lot of interesting things to work on wrt assimp code. If we can bring that library up to glTF v1.0 standards then we should be able to import from other formats in to glTF consistently and reliably.

I have a student who will work on gltf-pipeline, and I will be looking for more in October (the project would start in January).

Thanks for the task list; I agree it could be a great project. If/when I have an interested student, I will follow-up with you to learn the current assimp glTF state.

@ascandal
Copy link

ascandal commented Oct 10, 2016

@pjcozzi I have been working on the assimp glTF export. Recently I have been able to update some v1.0 validator issues, exported animations, and skins. All recently merged into assimp master.

Can you explain to me more the purpose of the gltf-pipeline repo you mention?

Maybe this discussion can be moved to a more appropriate place.

@pjcozzi
Copy link
Member

pjcozzi commented Oct 12, 2016

gltf-pipeline is about processing glTF models, e.g., glTF goes in and glTF goes out. It overlaps with assimp in some ways since it does optimizations like removing unused meshes, combining meshes, reordering for vertex cache, etc, but it is also glTF-specific in many ways since it, for example, converts glTF to Binary glTF, can add the WEB_quantized_attributes extension, etc.

We haven't fully packaged it up yet, but it is still in good enough shape to use; we are already using it as part of the online COLLADA to glTF converter.

If you want to discuss gltf-pipeline more, just submit an issue in its repo.

@donmccurdy
Copy link
Contributor

I think this can be closed? glTF2.0 has distinct baseColorFactor and baseColorTexture properties.

@pjcozzi
Copy link
Member

pjcozzi commented Dec 24, 2017

Agree with @donmccurdy

@pjcozzi pjcozzi closed this as completed Dec 24, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

7 participants