FBX Converter now exports in a ObjectLoader compatible format

[Benjamin-Dobell]

Had to make a few changes to three.js itself to get things working (namely adding a Reference object).

@mrdoob The very recent renaming of MeshFaceMaterial to MultiMaterial made my rebase a tad difficult, but I'll forgive you because you fixed the UV coordinate bug I was having with MeshFaceMaterial 😉

I've also made MultiMaterial a real Material. This was required because initMaterial needed to be called to setup things like material.precision (@mrdoob unless you already addressed this?) If this is unnecessary or undesirable I'll happily drop this commit. (DROPPED)

[Benjamin-Dobell]

@mrdoob Is there anything I can do to help you test this? e.g. provide some FBX files?

P.S. Thanks for all the awesome work you've done with three.js, it's immensely appreciated!

[WestLangley]

If you must add optional parameters to the constructor, maybe the API should be

var scene = new THREE.Scene( { fog: fog } );

[Benjamin-Dobell]

Admittedly, I wasn't super thrilled about that change either. However, I did it this way to keep in line with how the rest of ObjectLoader works. If you look in ObjectLoader you'll see arguments to other objects are passed in a similar fashion.

[WestLangley]

I think required arguments would have a different API from optional arguments. That is just my opinion, however.

[Benjamin-Dobell]

@WestLangley Definitely inclined to agree with you. I only went with this approach because it's what THREE.HemisphereLight does:

https://github.com/mrdoob/three.js/blob/dev/src/loaders/ObjectLoader.js#L549
https://github.com/mrdoob/three.js/blob/dev/src/lights/HemisphereLight.js#L15

However, I guess in the case of THREE.HemisphereLight the object is a bit more specialised i.e. not likely to become littered with arbitrarily ordered optional parameters.

@mrdoob Your feedback on the matter would be appreciated.

[mrdoob]

Hmm... Why do you need to pass fog in the constructor?

[Benjamin-Dobell]

I don't specifically want to pass fog to the constructor, I just wanted to try continue supporting everything the existing FBX converter (i.e. SceneLoader) supports. However, I noticed that ObjectLoader seems to be passing all JSON options from data as constructor arguments to the various objects being instantiated, even for optional parameters (in the case of HeimisphereLight):

https://github.com/mrdoob/three.js/blob/dev/src/loaders/ObjectLoader.js#L506-L573

So I just followed suit.

[Benjamin-Dobell]

There are several code style issues with this pull request that I'll need to iron out before merging - I used spaces instead of tabs, how did I even do that, I love tabs!

I'll force push the changes soonish, however the functionality can still be assessed.

[Benjamin-Dobell]

I've just force pushed:

• JS formatting fixes (ran through Mr. Doob approves to be certain).
• Cleaned up the python even more (removed some unused variables and consistently named others).
• Removed references to ambient color for materials (also checked other properties are valid).
• Fixed the FBX -> three.js material bindings for bump maps, env maps and ambient occlusion.
• Added warning message when unsupported texture types are encountered.
• Replaced any references to MeshFaceMaterial with MultiMaterial.
• Updated the README with the additional usage options.
• Added a new commit on the end to add support for emissiveMap in ObjectLoader.

and of course tested all the changes 😄

[mrdoob]

I trust you on the changes to the converter 😉
But I'll have to take a look at the changes on src. On a first check I'm not super keen on the THREE.Reference naming/approach. What is it for exactly?

I've also made MultiMaterial a real Material. This was required because initMaterial needed to be called to setup things like material.precision (@mrdoob unless you already addressed this?) If this is unnecessary or undesirable I'll happily drop this commit.

Does MultiMaterial need precision?

[Benjamin-Dobell]

But I'll have to take a look at the changes on src. On a first check I'm not super keen on the THREE.Reference naming/approach. What is it for exactly?

Yeah, I'm not overly fond of it either, however I can explain the logic behind it...

MultiMaterial (and potentially other used defined material types) need a way to refer to submaterials, as opposed to just having them embedded (to avoid duplicate nested versions of the same material). We've got uuids on all objects and that's the pattern we follow for referring to objects elsewhere. Typically it's just a matter of specifying the uuid as a string and we're good to go. However, that won't work here due to the way ObjectLoader delegates to MaterialLoader.

ObjectLoader knows about entire scenes, and hence is capable of resolving references to images, textures etc. MaterialLoader on the other hand is highly specialised and simply knows how to load basic material properties (as it knows nothing about what textures might be available). Consequently, there is no way for MaterialLoader to resolve references to other materials (as it doesn't know about them), so the responsibility falls to ObjectLoader. However, MaterialLoader does try to load submaterials (as they may be embedded). It does this by looping over its materials array and instantiating objects based on type:

https://github.com/mrdoob/three.js/blob/master/src/loaders/MaterialLoader.js#L37

Consequently, if I'm putting some sort of reference (not necessarily a THREE.Reference, but some hash with a uuid) in materials, it must contain a THREE.Something type. This is why I added THREE.Reference.

Of course, this is but one very specific way of resolving this issue.

Perhaps a better solution would be instead of storing the references to other materials in the materials array at all; simply store the uuids directly in a separate array called materialReferences. Then MaterialLoader won't touch them and ObjectLoader can simply go through materialReferences, resolve the uuids and append the referenced materials as submaterials after all the materials are parsed from JSON (in a similar fashion to what I'm doing with THREE.Reference at the moment). This approach wouldn't require the introduction of a new object (THREE.Reference) but would require a new property materialReferences in the JSON representation (only, not runtime) of materials.

Does MultiMaterial need precision?

I don't believe so, however the WebGL renderer was crashing because it expected some variables like precision to be defined on all materials it encountered. It's very possible you've made changes to address this already, so I'll try dropping this commit and see if things still operate as expected.

...

By the way I'm not super fond of fog being passed to the constructor either. Happy to take another approach if you have one in mind.

[Benjamin-Dobell]

Does MultiMaterial need precision?

I don't believe so, however the WebGL renderer was crashing because it expected some variables like precision to be defined on all materials it encountered. It's very possible you've made changes to address this already, so I'll try dropping this commit and see if things still operate as expected.

@mrdoob I just blew away the commit making MultiMaterial "inherit" from Material, as my testing proved it to be unnecessary with the latest on the dev branch.

[Benjamin-Dobell]

@mrdoob I've just forced pushed a more self-contained implementation (as mentioned above). i.e. THREE.Reference is gone - so this pull request no longer includes any additional classes.

[WestLangley]

Please do not include build files in Pull Requests.

See the Wiki article How to Contribute to three.js.

[Benjamin-Dobell]

whoops didn't mean to!

[Benjamin-Dobell]

@WestLangley Fixed now.

[WestLangley]

quaternion should have preference.

Perhaps the only option, depending on @mrdoob's decision.

[Benjamin-Dobell]

quaternion should have preference.

Possibly, but as a general rule I wouldn't expect to see both in the one object anyway.

[WestLangley]

The converter is up to you, but do you want to use quaternions, instead?

[Benjamin-Dobell]

Fair call. I didn't really change as much stuff as it looks like I did 😉

It was already using rotation, I just moved it to the new location:
https://github.com/mrdoob/three.js/pull/7046/files#diff-ccc8c41ed04893276377da090bf6f968L1980

[WestLangley]

three.js is quaternion-based. rotation is provided for convenience.

Are you certain of the mapping between the way three.js represents Euler rotations and the way FBX does? I expect it is not a trivial assignment.

[Benjamin-Dobell]

@WestLangley I haven't really touched any of the math at all, if it worked before it works now... assuming it worked before.

However, rotation is only used once, and it's on the Scene with values (0, 0, 0). Every where else is using either matrices or quaternions e.g.
https://github.com/Benjamin-Dobell/three.js/blob/fbx-objectloader/utils/converters/fbx/convert_to_threejs.py#L1770

I can verify that the quaternions and matrices work perfectly as I have a heap of FBX files with rotations on the root node.

[threejsworker]

The examples of this pullrequest are now built and visible in threejsworker. To view them, go to the following link:

http://threejsworker.com/viewpullrequest.html#7046

[mrdoob]

Could you remove the ObjectLoader changes from the PR?

[Benjamin-Dobell]

@mrdoob Some of them, sure. Others are required - well, not necessarily required as is, but something similar is necessary.

• fog support - Easily removed.
  I don't like the constructor thing anyway. Can probably open an issue for fog support though.
• quaternion support - Required.
  The old converter was using quaternions, I didn't change this, I've simply remapped where the quaternions need to go... Well, I could update the converter, but that's getting a bit nitty gritty. The other changes in this pull request are mostly just cosmetic.
• material.referencedMaterials aka nested material support, aka MultiMaterial support - Required.
  Unless you've updated the ObjectLoader recently (admittedly I haven't checked), nested materials are not supported by the existing ObjectLoader format.
• emissiveMap support - Not required... well, by me anyway.
  Should probably be included seems as the other texture maps are supported in a similar fashion.
• Unnecessary cloning. Easily removed.
  I just wasn't bothered until we sorted out the other details, in particular nested materials.

[mrdoob]

Would it be possible to not support MeshFaceMaterial by now? Would be nice to just update the converter so it generates BufferGeometry. With that updated we can then do a new PR with the new features.

[Benjamin-Dobell]

I don't know what other people's FBX look like, but for me not supporting MultiMaterial/MeshFaceMaterial is a complete show-stopper. Every model I have (in excess of 100 models and counting) needs this functionality.

I haven't actually added MeshFaceMaterial support. It was already part of the previous version of the converter, using the SceneLoader format - which is what I was using prior to this.

I've actually been using this exact converter and ObjectLoader in production for over a month now. Been working perfectly.

[mrdoob]

MultiMaterial does have a toJSON, and MaterialLoader does handle json.material. So I guess we do have support in the format already?

[Benjamin-Dobell]

Okay, just had a look at ObjectLoader and MaterialLoader. They've definitely been cleaned up since I created this pull request - in particular ObjectLoader now properly delegates all material loading.

I see MaterialLoader now understands uuid, and in the case of textures is even more generic name (which will be uuids in the case of ObjectLoader).

The issue is materials need a way to reference other materials

This is necessary say if you have one Material, some semi-transparent glass, applied to 100 different mesh (as children of MultiMaterial) you don't want to repeat that Material definition 100 times.

So parsing materials basically needs to be done in two passes. First pass is loading material definitions - skipping over references to other materials. The second pass resolves the material references (uuid lookup) now that the Materials have all been instantiated.

An alternative would be to define referenced materials ahead of the materials referencing them - however that's getting pretty finicky.

[mrdoob]

This is necessary say if you have one Material, some semi-transparent glass, applied to 100 different mesh (as children of MultiMaterial) you don't want to repeat that Material definition 100 times.

I understand, but that's a bit of an optimisation. I just don't want to rush the format design for the sake of bringing the FBX converter up to speed.

[Benjamin-Dobell]

Understandable, that's why I hadn't really been pushing for this to be merged. Things are (or at least were) under quite a bit of flux.

However, it did seem to me that referencing, rather than duplicating, was the "correct" solution given the way the rest of the file format works. Particularly, since one of the key features of this new format is the inclusion of uuid, which presumably was specifically introduced so objects could be referenced more easily/consistently. Pairing that with the fact that we no longer actually need to clone Material per mesh, it'd be a bit disappointing to end up duplicating the materials in the file format.

If you don't mind, I'd prefer to try nail down some of the specifics of format, rather than release a sub-par converter. Of course, this pull request is just a proposal, I'm sure there are other approaches to achieve this.

[Benjamin-Dobell]

By the way, my original implementation didn't require introducing a new key in Material's JSON representation. There's still a little bit of information in my earlier comments, but the code was blasted away by a force push.

Basically I just introduced new THREE type (THREE.Reference, but the the name is fairly arbitrary). If we were to return to my original approach, then the MaterialLoader would not need to know anything about material referencing at all. The existing code would instantiate the correct type (as long as it's in the THREE "namespace") and assign the uuid property. Then ObjectLoader would follow up on the second parse, replacing these reference objects with resolved materials.

This approach means that MaterialLoader remains self contained and never needs to know anything about the ObjectLoader file format.

I basically swapped to the existing code because it seemed less disruptive at the time (no new object types). But with the recent MaterialLoader changes, this approach doesn't seem so appealing.