mrdoob / three.js

JavaScript 3D Library.
https://threejs.org/
MIT License
101.81k stars 35.31k forks source link

SEA3D vs GLTF #14419

Closed RemusMar closed 6 years ago

RemusMar commented 6 years ago

This is the current (july 2018) GLTF status. Test case: the same skinned mesh exported from 3DS Max 2018. Standard material: Diffuse + Specular + Normal

1) SEA3D exporter + SEA3D importer: http://necromanthus.com/Test/html5/Lara.html SEA file size: 658 KB Result: close to perfect

2) Babylon3D GLTF exporter + GLTF importer: http://necromanthus.com/Test/html5/Lara_gltf.html GLB file size: 1,850 KB Result: messed up materials

I've also tested the FBX2GLTF utility (by Facebook): the same wrong results

Important note: there is nothing wrong with THREE.js and PBR materials: http://necromanthus.com/Test/html5/Lara_PBR.html

In any case, PBR was a bad choice for GLTF and also, all the current converters are collection of bugs.

Mugen87 commented 6 years ago

I don't think this repo is the right place for this post. It's neither a feature request, nor a bug. So my question is: What are you trying to accomplish with this issue? Bashing glTF?

If you encounter problems with an exporter or converter, I suggest you open an issue at the respective github repo.

In any case, PBR was a bad choice for GLTF and also, all the current converters are collection of bugs.

I generally reject such Trump-like statements. They have a provocative nature and are not objective at all.

RemusMar commented 6 years ago

I don't think this repo is the right place for this post.

It's the best place for sure and it shows the current GLTF status.

I generally reject such Trump-like statements.

Really? Here is a statement from Trump: Google already failed with UTF8.

If you encounter problems with an exporter or converter, I suggest you open an issue at the respective github repo.

Many of them blame THREE.js for bad GLTF results. The posted samples prove they are wrong.

looeee commented 6 years ago

@RemusMar are you suggesting any particular actions we should take? If not I vote to close this issue.

RemusMar commented 6 years ago

RemusMar are you suggesting any particular actions we should take?

Three possible causes for the wrong GLTF results:

  1. the Babylon3D exporter is buggy (they say it's not)
  2. the FBX2GLTF converter is buggy (they say it's not)
  3. the GLTF importer is buggy.

But you and Mugen87 want to close the topic because there is no issue and everytbody is happy ...

looeee commented 6 years ago

the GLTF importer is buggy

You mean the GLTFLoader? Can you identify what the bug is? It would be especially helpful if you can find a very simple model that demonstrates the bug.

(they say it's not)

If you've made bug reports on the Babylon3D exporter and FBX2GLTF can you link to them here?

RemusMar commented 6 years ago

You mean the GLTFLoader?

Yes. Importer = Loader + Parser

It would be especially helpful if you can find a very simple model that demonstrates the bug.

You have everything you need to study the issue. Any PHONG material (Diffuse + Specular + Normal) exported or converted to GLTF gives wrong results. p.s. PHONG represents 50-60% of the current samples, compared to Physical less than 1%. That's why I said that PBR was a bad choice for GLTF.

looeee commented 6 years ago

You have everything you need to study the issue

In other words you want someone else to do the work for you 🙄

Regarding Phong materials and glTF, I agree that this makes glTF a bad choice for converting older models - especially models originally exported as FBX which only supports Phong or Lambert shading.

RemusMar commented 6 years ago

n other words you want someone else to do the work for you

I don't have enough spare time for "GitHub activities". You got the report and the working samples. That's all for now. cheers

looeee commented 6 years ago

In that case, I still don't consider this to be a complete or actionable bug report and I continue to vote to close this issue.

mrdoob commented 6 years ago

Hey @RemusMar,

Thanks for reporting this. Some notes...

File size I'm not sure where you're getting these numbers, this is what I see:

Lara.sea: 1,032,525 bytes 
Lara.glb: 1,850,164 bytes

Lara.sea.gz: 1,031,840 bytes
Lara.glb.gz: 919,712 bytes 

Materials Your model uses Diffuse + Specular. Unfortunately, seems like the specular texture is not being exported. GLTF supports 2 PBR modes: Metalness + Roughness and Specular + Glossiness. You want to export your model using the second mode. GLTFLoader supports both but maybe the Babylon.js doesn't have an option to export in that mode? In that case you may want to do a feature request on their project.

Let us know what you find out.

RemusMar commented 6 years ago

Hi Ricardo,

  1. File size. If you download the files (with Firefox) you'll get: Lara.sea: 658,901 bytes Lara.glb: 1,850,964 bytes Even more: the GLB file does not even contain the equivalent (Metalness) of the Specular texture !

2) Materials As I said before, the original MAX and FBX files contain a standard PHONG material: Diffuse + Specular + Normal None of the current GLTF exporters and converters is able to generate a correct Physical material. PHONG is way more popular than Physical.

p.s. I'm not interested in the GLTF format (SEA3D is better from any point of view). I just want to help other poeple.

RemusMar commented 6 years ago

Another userful sample: Here is a NATIVE Physical material in 3DS Max 2018 exported to GLTF with Babylon3D exporter: http://necromanthus.com/Test/html5/Lara_gltf_physical.html Now the metalness is present, but the result is still wrong:

However, this GLB file looks better (compared to THREE) in the Babylon sandbox ( https://sandbox.babylonjs.com ). Again, this is how the Physical material should look in THREE: http://necromanthus.com/Test/html5/Lara_PBR.html

So let's forget now about PHONG and buggy exporters and converters. We should investigate why the GLB file looks better in the Babylon sandbox. Buggy GLTF Loader in THREE ?

pailhead commented 6 years ago

If one could quickly prototype some hacks over the existing phong / standard implementations i bet it would be pretty useful 😉

mrdoob commented 6 years ago

@RemusMar Seems like the glb includes a AmbientLight.

screen shot 2018-07-08 at 7 44 30 pm

If you set visible to false to the imported AmbientLight the character starts to look less red.

The last thing to do is setting renderer.gammaOutput = true. (Needed when using GLTF #12766)

screen shot 2018-07-08 at 7 48 00 pm
RemusMar commented 6 years ago

Seems like the glb includes a AmbientLight.

Good catch Ricardo! :thumbsup:

If you set visible to false to the imported AmbientLight the character starts to look less red.

Something better (no wasted resources):

        gltf.scene.remove( gltf.scene.children[2] );

The last thing to do is setting renderer.gammaOutput = true. (Needed when using GLTF #12766)

That indicates buggy GLTF Loader (and it has to be fixed). If I use that for other loaders, I get wrong colors for the loaded models. Just think about this scenario: use various loaders for the same scene and one of them is GLTF. It will mess up the entire scene!

Anyway, after removing that ambient light and using this workaround, this is the result: http://necromanthus.com/Test/html5/Lara_gltf_physical.html Much better compared to the initial GLTF one, but the material quality is far away from this one: http://necromanthus.com/Test/html5/Lara_PBR.html Impressive lighting response and great metalness for bra and bikini.

At this stage I won't use GLTF in any serious project.

mrdoob commented 6 years ago

Can you try adding a envMap cubemap to these examples too? PBR looks the best when a envMap is supplied.

RemusMar commented 6 years ago

Can you try adding a envMap cubemap to these examples too? PBR looks the best when a envMap is supplied.

I've added offline. Of course it looks better, but PHONG with Environment map still looks WAY better (in THREE). Also, the envMap does not fix the GLTF Loader issue(s).

mrdoob commented 6 years ago

I've added offline.

Could you update the online samples?

donmccurdy commented 6 years ago

The glTF format supports PBR and unlit shaders at this time. Whether the BabylonJS and FBX2GLTF tools do the Phong-to-PBR conversion in a way that preserves Phong specular maps, I don't know — that would be a question for the repos of those tools. If you are trying to preserve the exact appearance of models using classic Phong shaders, you may have an easier time with other formats.

The last thing to do is setting renderer.gammaOutput = true.

That indicates buggy GLTF Loader (and it has to be fixed).

This is a deliberate decision and not a bug. Base color and emissive textures in glTF (and, typically, diffuse textures in any format...) are in sRGB colorspace. GLTFLoader marks them as such (material.map.encoding = THREE.sRGBEncoding), so that they're converted to linear colorspace for correct PBR lighting calculations. Finally colors should be converted back to sRGB (e.g. renderer.gammaOutput=true).

If you skip all of this, with any format, lighting calculations are incorrect. SEA3DLoader, FBXLoader, and ColladaLoader never touch the .encoding property of any texture, and leave it to the end user to change texture colorspace and renderer colorspace. I'm pretty confident that the large majority of three.js users are passing sRGB colors into three.js without converting, despite the fact that renderer lighting calculations assume linear colorspace, and getting results that are "good enough" but inconsistent with other engines and authoring environments. For correct results you should be using renderer.gammaOutput=true, and marking sRGB textures as sRGB.

None of these issue are specific to glTF (see https://github.com/mrdoob/three.js/issues/11337), but with GLTFLoader we're trying to achieve consistency with other engines and 3D authoring environments, and have chosen to treat all sRGB textures as sRGB for a first step. If you're mixing models from other formats in the scene, then yes it's awkward, and you'd need to either mark the diffuse textures of those formats as sRGB or mark the colorspace on the glTF models to linear (the latter is incorrect for all model formats involved, but may look good enough if you don't need precise colors).


It does not seem like there is anything actionable here, unless there are specific issues we can report to the tools mentioned. @RemusMar if you are happy with your SEA3D workflow, that's great — I'm not interested in debating formats or persuading you to change from something that is already working well for you.

RemusMar commented 6 years ago

Could you update the online samples?

Ricardo, PHONG looks great with Diffuse + Specular only. PBR does not look great with BaseColor + MetallicRoughness only. That's the main problem here. The Normal/Bump and Environment textures are irrelevant at this point. On top of that: more texture layers = bigger file size and performances drop

If you skip all of this, with any format, lighting calculations are incorrect. SEA3DLoader, FBXLoader, and ColladaLoader never touch the .encoding property of any texture

That was a wise decision.

I'm not interested in debating formats or persuading you to change from something that is already working well for you.

Don, I'm not debating the "PBR only" bad choice for GLTF. This topic shows GLTFLoader design flaws. We don't reinvent the wheel here, so "renderer.gammaOutput = true" is not an option now, when GLTF represents less than 1% of the market. cheers

RemusMar commented 6 years ago

Can you try adding a envMap cubemap to these examples too? PBR looks the best when a envMap is supplied. Could you update the online samples?

Because you asked me to:

SEA3D + Phong: http://necromanthus.com/Test/html5/Lara_envMap.html vs GLTF + PBR: http://necromanthus.com/Test/html5/Lara_gltf_envMap.html

The quality drop is obvious. Also, in 3DS Max 2018 the Physical material looks WAY better than the GLTF result.

RemusMar commented 6 years ago

None of these issue are specific to glTF (see #11337), but with GLTFLoader we're trying to achieve consistency with other engines and 3D authoring environments,

That's completely wrong Don! Here we're talking about GLTFLoader and THREE.js The users are interested in the best results with THREE. Other engines and 3D authoring environments are irrelevant here.

And you still don't understand the main problem here. For the last time:

1) JSONLoader (or SEA3D loader) + PBR = GOOD results (close to Phong): http://necromanthus.com/Test/html5/Lara_PBR.html

2) GLTFLoader + PBR = BAD results http://necromanthus.com/Test/html5/Lara_gltf_physical.html

p.s. In the first sample you don't even need "renderer.gammaOutput = true" to get good results !!!

donmccurdy commented 6 years ago

The users are interested in the best results with THREE. Other engines and 3D authoring environments are irrelevant here.

Being able to author a PBR model in Substance Painter or download one from Sketchfab, and have the model appear as the artist designed it, is good for three.js users. I don't think there's any definition of "best result" where that sort of consistency can be dismissed.

This topic shows GLTFLoader design flaws. We don't reinvent the wheel here, so "renderer.gammaOutput = true" is not an option now...

This isn't reinventing any wheels, and it isn't a design flaw. PBR calculations are done in linear space, with every engine I'm aware of. If you pass sRGB data into the renderer and pretend it's linear, the lighting and blending math will come out wrong. The difference is not huge, and so this not a major concern for many three.js users, but nevertheless it is not as good as it could be. For that reason, your "good" result example is not actually correct. But as you've said before, backward-compatibility is important, so I'm not here to advocate for changing any three.js defaults. But because glTF is a new format, and because we're trying to get PBR right, we're going to mark sRGB textures as sRGB, even if other loaders are not doing so.

See this article about Unreal

...textures that are used for color information should have the sRGB flag checked, and textures that are used for masks and numerical calculations in shaders and effects (like Normal maps) should have it unchecked. And if you follow this simple guideline you mostly get the best effect.

This is precisely what we are doing.

looeee commented 6 years ago

...I'm pretty confident that the large majority of three.js users are passing sRGB colors into three.js without converting, despite the fact that renderer lighting calculations assume linear colorspace

You are talking about PBR materials with glTF, but I assume this is just as much a problem with a Phong material?

...because glTF is a new format, and because we're trying to get PBR right, we're going to mark sRGB textures as sRGB, even if other loaders are not doing so.

@donmccurdy should other loaders be doing so? It seems like this inconsistency between loaders is a point of confusion for users, and it would make sense for all of them to treat sRGB textures the same way if possible.

donmccurdy commented 6 years ago

You are talking about PBR materials with glTF, but I assume this is just as much a problem with a Phong material?

Yes, the problem is the same for Phong materials or PBR materials loaded in any other format.

...should other loaders be doing so? It seems like this inconsistency between loaders is a point of confusion for users, and it would make sense for all of them to treat sRGB textures the same way if possible.

If we had a time machine, yes, the other loaders should also be marking sRGB textures containing color data as sRGB. But making the change now would cause confusion and break backward-compatibility, and the gammaOutput=true setting needed to fix output is not intuitive — I don't think changing other loaders can be justified given those issues.

Let's keep an eye on https://github.com/mrdoob/three.js/issues/11337. I hope the resolution there will make color workflows more intuitive. With that and NodeMaterial, there may be opportunities to fix some existing issues without breaking anyone's existing applications.

RemusMar commented 6 years ago

Being able to author a PBR model in Substance Painter or download one from Sketchfab,

They are irrelevant. I get much better results in 3DS Max and that tells me that the GLTFLoader and/or your PBR model are not properly implemented.

The difference is not huge, and so this not a major concern for many three.js users, but nevertheless it is not as good as it could be.

Your girlfriend looks bad but you're happy because your boss told you that's normal. OMG ...

For that reason, your "good" result example is not actually correct.

In fact you should fix your "correct" example to look good.

looeee commented 6 years ago

If we had a time machine, yes, the other loaders should also be marking sRGB textures containing color data as sRGB.

Yeah, it's unfortunate but I agree that it's not worth breaking backwards compatibility over this.

However, as I've been working with larger FBX scenes consisting of multiple models, animated cameras and so on I've found myself wishing that the output of the loader was something more like GLTFLoader's output - that is, it should return an fbx object with properties:

fbx.animations; // Array<THREE.AnimationClip>
fbx.models; // Array <THREE.Group, THREE.Mesh, THREE.SkinnedMesh>
fbx.cameras; // Array<THREE.Camera>
fbx.asset; // Object

There may be other loaders that would benefit from a similar change. We should add this to the backburner (and certainly wait on #11337), but if any loaders do have breaking changes made for whatever reason, then we can use that as opportunity to apply this change as well.

Perhaps we should open a new issue to keep track of this?

Mugen87 commented 6 years ago

Perhaps we should open a new issue to keep track of this?

Please do. That's better than resume the conversation in this closed thread.

RemusMar commented 6 years ago

Just removed the FORCED sRGB encoding in the GLTFLoader.

//          if ( material.map ) material.map.encoding = THREE.sRGBEncoding;
//          if ( material.emissiveMap ) material.emissiveMap.encoding = THREE.sRGBEncoding;
//          if ( material.specularMap ) material.specularMap.encoding = THREE.sRGBEncoding;

The result is better from any point of view:

SEA3D + Phong: http://necromanthus.com/Test/html5/Lara_envMap.html vs GLTF + PBR: http://necromanthus.com/Test/html5/Lara_gltf_envMap.html

donmccurdy commented 6 years ago

I've addressed each of those points in https://github.com/mrdoob/three.js/issues/14419#issuecomment-403513554 — we will not be removing the sRGB encoding assignment to sRGB textures in GLTFLoader. If you would like to override that, it is easy to change the texture encoding after loading the model.

If there are no other actions to take here, this issue should be closed.

RemusMar commented 6 years ago

we will not be removing the sRGB encoding assignment to sRGB textures in GLTFLoader.

That bad choice is yours, but the GLTFLoader is part of THREE, so it's up to Ricardo ( @mrdoob ) if they will be removed or not. In any case, you (and @looeee ) continue to be on the wrong path. Again: your workflow is not good and is not backwards compatible. You should start learning from the more experienced people: https://forum.allegorithmic.com/index.php?topic=8745.0

In Unity you don't have to do anything. Maps that are placed in the metallic/smoothness, ambient occlusion are treated as linear by the shader and maps in the albedo are treated as sRGB. What the shader does for the albedo and specular is "linearize" the maps. It removes the gamma-encoded values from the map in the shader code by applying an inverse gamma to it of 0.4545. In the Unity workflow, this is done automatically and you don't need to flag the images as sRGB.

looeee commented 6 years ago

In the Unity workflow, this is done automatically and you don't need to flag the images as sRGB.

Unity is treating textures as sRGB automatically rather than leaving it up to the user. This is exactly what the GLTFLoader does, and what we are arguing the other loaders should have been doing from the start.

pailhead commented 6 years ago

That bad choice is yours, but the GLTFLoader is part of THREE, so it's up to Ricardo ( @mrdoob ) if they will be removed or not.

Errr.... this is extremely fuzzy. GLTFLoader appears to be a community provided example, that lives in /examples. If you load three.js alone (three.min.js) there wont be any mention of GLTF.

If you use three.js off the shelf you get a scenegraph and various webgl abstractions. In this context, GLTF is just another one of many many examples of how you can translate some generic 3d data / scene file into three.js's structs.

So, at a glance, three.js seems like this generic library, that draws stuff to screen. It doesn't care if you fetch that data from some remote server, and it doesn't care how you parse it (collada, fbx, sea3d, gltf... and 40 others). At the end of the day, you are rendering a THREE.Mesh with THREE.Geometry and THREE.Material. Absolutely all of the loaders share this feature. All of them result in this data structure.

However, in practice, this is not the case, and you seem to be in the right.

GLTF is a "first class citizen" of three.js. @mrdoob wrote that several times. SEA3d is some random format written by some unknown people while GLTF has the backing of THE khronos group. On top of that, it's probably meant to be the backbone of the whole VR/AR revolution, hence so much backing by other big players.

This is just an unfortunate circumstance that three.js found itself in as the most widely used webgl library. People want to do 3d, which three.js solves really well with it's scene graph and other abstractions (things like Mesh, Geometry, Material, Texture etc.). Unfortunately, people also want to make experiences and expect three.js to be able to do that. This is of course complicated, hence, favoring one particular format and giving it preferential treatment makes sense. It may somewhat hurt the very essence of the library (draw stuff on screen) but it's a trade off.

If you care to read all the guidelines, there's a document called owners. @donmccurdy owns that particular loader, so his word carries as much weight as @mrdoob's, so good luck there :)

The directive right now is:

three.js must support gltf

So anything that the khronos group comes up with has to be reflected in three.js. If you look at the discussion historically, @mrdoob doesn't really follow what's going on and @donmccurdy is the authority on all things khronos/gltf related.

So for example, khronos has defined a specification for gltf "extensions". Out of infinitely many extensions, one involves texture transformations. It has been ratified by the khronos group and because of that, three.js absolutely must support it. This warrants a PR with 11 thousands line of code and increasing the size of the library by 1/3.

I think you're wasting a lot of effort head butting a wall here, and that this issue should be closed.

RemusMar commented 6 years ago

I think you're wasting a lot of effort head butting a wall here, and that this issue should be closed.

good point As I said before, Google already failed with UTF8. In the current development status, GLTF (and PBR) follow the same path.

pailhead commented 6 years ago

Unity is treating textures as sRGB automatically rather than leaving it up to the user. This is exactly what the GLTFLoader does, and what we are arguing the other loaders should have been doing from the start.

Unity has an editor which is a foreign concept for three.js. When you "load" a texture into unity, you bring it into the editor which can do all kinds of transformations before actually storing it for game's use. I think that three.js has a different paradigm, since it doesn't care how you create the assets nor how you store them. An analogy would be if three.js came with folder /utils and then /textureTools or something like that, where you could prepare the assets for three.js.

This difference should possibly be considered.

The whole gamma correct workflow is extremely complicated, and if i recall correctly you may lose some precision if you store the textures in one way versus another. With three.js this is still kinda low-levelish so i'd prefer to leave multiple options to the user.

looeee commented 6 years ago

If you care to read all the guidelines, there's a document called owners.

Wow, I never say that before. And apparently I "own" the FBXLoader 😍 😆

pailhead commented 6 years ago

I think it's a software engineering term. You "own" a code base?

RemusMar commented 6 years ago

And apparently I "own" the FBXLoader

So in your opinion you're free to mess it up, isn't it?

pailhead commented 6 years ago

In my opinion everyone who owns something is free to mess it up. I'm not sure if there should be place for disagreement with this, in the modern world :). If you have a Ferrari and decide to put a brick on the gas pedal and have it drive off a cliff, i may think it's a waste but it's YOUR Ferrari, can't get simpler than that. Unless it's 1917 and you're in tsarist Russia.

pailhead commented 6 years ago

That right there is disruptive content... :(

You're free to fork the project, the examples and do whatever work you deem fit. If it makes you feel any better, what the fbx loader owner did is not at all different - the loader did exist for a couple of years and was written by other people before he took ownership. Fork's no different than this.

pailhead commented 6 years ago

I think the documents wouldn't hurt by having some guideline on how to author textures. The texture can still be in the wrong space regardless of the gltf format, like under calibrated or over calibrated. Something saying "when making a texture in photoshop do that, when making it in GIMP do this".

looeee commented 6 years ago

Cut the "disruptive content" crap and grow up !!!

@RemusMar I have no idea what you are talking about. I've never marked yours or anyone else's comments as disruptive.

However, I would like to say that, much as I have a thick skin and don't care about your opinions re usernames, ad hominem attacks of the kind that you frequently make are not acceptable and should be grounds for being excluded from this community.

looeee commented 6 years ago

@pailhead agreed, that would be a very useful addition.

Mugen87 commented 6 years ago

I've never marked yours or anyone else's comments as disruptive.

I'm the one who was doing this. As collaborators we have to moderate issues and ensure correct language and behavior.

RemusMar commented 6 years ago

I'm the one who was doing this. As collaborators we have to moderate issues and ensure correct language and behavior.

Grow up first!

looeee commented 6 years ago

@Mugen87 I agree with this, and will start to be more vigilant in marking disruptive or otherwise useless comments. In this case, since I was the one being targeted, I didn't think it was appropriate to do so.

However, @RemusMar has consistently exhibited what can only be described as disruptive behaviour over a long period of time and I don't think that marking comments is going to bring about any kind of change.

We're consistently having to deal with ad hominem attacks from this user in the form of name calling or other vaguely aggressive statements, and it's quite frankly tedious and detrimental to the development of the three.js community.