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

GLTFLoader: Support map in LineBasicMaterial #17511

Closed
lappy4711 opened this issue Sep 16, 2019 · 18 comments · Fixed by #25717
Closed

GLTFLoader: Support map in LineBasicMaterial #17511

lappy4711 opened this issue Sep 16, 2019 · 18 comments · Fixed by #25717

Comments

@lappy4711
Copy link

lappy4711 commented Sep 16, 2019

Was there a significant change in the GLTFLoader in version 90? We're having problems with gltf/glb files that we export from Paraview, the colors are not visible in our viewer. If changing the version to version 89, the colors are visible. Here is an example file:
005.zip

@mrdoob
Copy link
Owner

mrdoob commented Sep 16, 2019

Do you mind sharing screenshots?

@lappy4711
Copy link
Author

lappy4711 commented Sep 16, 2019

Here you go.

V92:
v92

V89:
v89

Note, not my codepen. All I did was to paste in my file in the loader and change the version numbers.

The original codepen is https://codepen.io/siouxcitizen/pen/QroPwJ

I have seen exported colors from Paraview in the latest GLTFLoaders, but only when I export with the Paraview UI. My example file is from a programmatic python Pararaview export....which seems to create a different result than the UI export. Works sometimes though, e.g in earlier versions (< 90) of the GLTFLoader and in the babylonjs online viewer. And in Windows built in 3D viewers.

@lappy4711
Copy link
Author

lappy4711 commented Sep 16, 2019

A colleague saw the images and said "that looks like a color space issue". It seems that the default color space in Paraview is "diverging". My best guess right now is that the Paraview UI export code converts to rgb before exporting and that we must do that as well in python. Don't know though why v90 and above is more unforgiving regarding color space (if this is correct). Will update when our Paraview script guy has implemented explicit conversion to rgb.

@donmccurdy
Copy link
Collaborator

@lappy4711 have you seen the Textures section under the THREE.GLTFLoader documentation? If you aren't using those renderer settings, you will see color space issues, but I don't think that's the cause here.

@mrdoob @takahirox the change was caused by b965ef1. With that commit we began using LineBasicMaterial for lines, which doesn't support a base color .map, instead of MeshStandardMaterial or MeshBasicMaterial. Would we want to add texture support for LineBasicMaterial, or is that out of its scope? 🤔

@mrdoob
Copy link
Owner

mrdoob commented Sep 16, 2019

GLTF support a texture for lines? Does it also have uvs?

@lappy4711
Copy link
Author

@donmccurdy I have tested
renderer.gammaOutput = true;
renderer.gammaFactor = 2.2;

I don't think
texture.encoding = THREE.sRGBEncoding;
is applicable since I'm not loading any textures (and neither is the codepen example I used for illustration of the issue)

@donmccurdy
Copy link
Collaborator

@lappy4711 this model does contain a texture, attached below. Those settings are good to use, generally, but they aren't the problem in this case.

0050

@mrdoob because glTF uses PBR materials, the section on materials for lines and points is pretty sparse. But yes, this model contains both a texture and UVs on a Lines primitive.

@lappy4711
Copy link
Author

Adding some more info....the colors are visible with three.min.js versions 90 and above...but if change GLTFLoader.js to 90 and above it is all white.

@mrdoob
Copy link
Owner

mrdoob commented Sep 16, 2019

Adding some more info....the colors are visible with three.min.js versions 90 and above...but if change GLTFLoader.js to 90 and above it is all white.

How's that possible? We never supported textures in LineBasicMaterial 🤔

@donmccurdy
Copy link
Collaborator

In r90 GLTFLoader switched from using MeshStandardMaterial to LineBasicMaterial for lines.

@mrdoob
Copy link
Owner

mrdoob commented Sep 16, 2019

Oh... Can Lines have normals too in GLTF?

@donmccurdy
Copy link
Collaborator

donmccurdy commented Sep 16, 2019

Hopefully no one will be shocked if their normals don't work there, but yes. glTF does not prohibit lines and points from having normals, tangents, skinning weights/indices, morph targets, or custom vertex attributes.

@mrdoob
Copy link
Owner

mrdoob commented Sep 16, 2019

I guess we may need a LineStandardMaterial? What about Points?

@donmccurdy
Copy link
Collaborator

Just getting LineBasicMaterial closer to feature parity with MeshBasicMaterial would be enough, I think. A PBR shading model for lines is more than anyone has asked for. Pretty much just supporting .map, along with emissive and AO perhaps?

Although if MeshStandardMaterial already 'just works' with lines, and LineStandardMaterial can reuse that implementation without duplicating much code, that's great. I'd be curious how things work in NodeMaterial now; I still want to add a gltfLoader.setUseNodes( true ) option.

@Mugen87
Copy link
Collaborator

Mugen87 commented Sep 17, 2019

I still want to add a gltfLoader.setUseNodes( true ) option.

Just for reference: #14572

@lappy4711
Copy link
Author

lappy4711 commented Sep 19, 2019

So what is the advice? Wait for a fix or use the r89 loader?
Thanks

@mrdoob mrdoob changed the title GLTFLoader version 90 and above GLTFLoader: Support map in LineBasicMaterial Sep 19, 2019
@donmccurdy
Copy link
Collaborator

donmccurdy commented Sep 19, 2019

@lappy4711 I don't know what the fix is yet, or when it will be ready. If you need to apply textures to lines, in the meantime, you'll need to either use r89 or load the texture separately from the rest of the model.

@lappy4711
Copy link
Author

@donmccurdy we just realised that we should add a "Tube" filter to the streamlines in Paraview. We now have colors, so I can keep using the latest loader.

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

Successfully merging a pull request may close this issue.

4 participants