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

FBX Converter now exports in a ObjectLoader compatible format #9326

Closed

Conversation

Benjamin-Dobell
Copy link
Contributor

@Benjamin-Dobell Benjamin-Dobell commented Jul 12, 2016

@mrdoob I'd appreciate it immensely if this could be reviewed/merged fairly quickly. The break-neck speed of development (a good thing) meant my last pull request (#7046) adding this functionality (almost a year ago) was unmergable pretty fast!

The converter in this pull request doesn't yet support BufferGeometry - I've actually been converting from Geometry to BufferGeometry on the fly (in the ObjectLoader) in my three.js fork. This is great because all scenes (old ones too) benefit from the newer BufferGeometry functionality - mind you it's not something I'd ever submit a pull request for, in its current state it's quite crude.

If I get a chance I'll submit a further pull request that adds BufferGeometry output as an additional command line switch. Although admittedly this isn't something I'll use myself, I prefer deciding at load time which geometry should be in a buffer or not (some geometry I modify or read at runtime).


FBX converter now exports to an ObjectLoader compatible format.

In addition to making the output ObjectLoader compatible, I've also added or
improved a bunch of functionality:

  • Fixed oddity whereby textures were output to a 'maps/' directory but only
    referenced by filename. The relative path is now stored in JSON.
  • Added texture filename collision detection which aborts conversion when two
    textures would be copied over each other. The abortion behavior can be
    replaced with a printed warning by specifying --ignore-texture-collisions
    (-i).
  • Textures can now be saved to directories other than 'maps/' by specifying
    --texture-output-dir (-o). This path is relative to the output JSON file and
    file references in the JSON are updated accordingly.
  • Materials whose textures contain a non-empty alpha channel now automatically
    have 'transparent' set to true (Requires ImageMagick). This can be disabled
    by specifying --no-transparency-detection (-a).
  • Non web compatible textures are now automatically converted to PNG or JPEG
    files, the former if they contain alpha (Requires ImageMagick). If
    ImageMagick is not available a warning is reported and we gracefully fall
    back to the old behavior.
  • Existing --pretty-print (-p) behavior has now been extended to print JSON
    properties in the order 'metadata', 'type', 'uuid', , 'data',
    'children'.
  • Redirected errors and warnings to stderr rather than printing to stdout.
  • Fixed FBX -> three.js texture bindings.
  • Added support for LineSegments.
  • Added duplicate geometry detection functionality enabled by specifying
    --optimize-geometry (-g). This just does a brute force search for identical
    geometry and removes the duplicates, replacing node geometry references
    accordingly.

Also performed general refactoring which included:

  • Deleted lots of dead and commented-out code.
  • Made method naming consistent (using underscores).
  • Deleted unused/pointless variables.
  • Replaced manual path string manipulation with native python methods (which
    fixed a couple of obscure bugs).

The following functionality has been removed:

  • Scene flattening, --flatten-scene (-f), has been removed as it made a heap
    of assumptions and doesn't really make sense in a converter. The task is
    better performed in a 3D modelling package, or even in three.js itself after
    a Scene has been loaded.

This is also an optimisation for scenes where child objects may
belong to multiple parents e.g. Several MultiMaterial may share the
same child Materials.
ObjectLoader looks for Reference objects in materials and resolves
them correctly. Cyclic material reference detection is included so
infinite loops will not occur.
In addition to making the output ObjectLoader compatible, I've also added or
improved a bunch of functionality:

  - Fixed oddity whereby textures were output to a 'maps/' directory but only
    referenced by filename. The relative path is now stored in JSON.

  - Added texture filename collision detection which aborts conversion when two
    textures would be copied over each other. The abortion behavior can be
    replaced with a printed warning by specifying --ignore-texture-collisions
    (-i).

  - Textures can now be saved to directories other than 'maps/' by specifying
    --texture-output-dir (-o). This path is relative to the output JSON file and
    file references in the JSON are updated accordingly.

  - Materials whose textures contain a non-empty alpha channel now automatically
    have 'transparent' set to true (Requires ImageMagick). This can be disabled
    by specifying --no-transparency-detection (-a).

  - Non web compatible textures are now automatically converted to PNG or JPEG
    files, the former if they contain alpha (Requires ImageMagick). If
    ImageMagick is not available a warning is reported and we gracefully fall
    back to the old behavior.

  - Existing --pretty-print (-p) behavior has now been extended to print JSON
    properties in the order 'metadata', 'type', 'uuid', <other keys>, 'data',
    'children'.

  - Redirected errors and warnings to stderr rather than printing to stdout.

  - Fixed FBX -> three.js texture bindings.

  - Added support for LineSegments.

  - Added duplicate geometry detection functionality enabled by specifying
    --optimize-geometry (-g). This just does a brute force search for identical
    geometry and removes the duplicates, replacing node geometry references
    accordingly.

Also performed general refactoring which included:

  - Deleted lots of dead and commented-out code.

  - Made method naming consistent (using underscores).

  - Deleted unused/pointless variables.

  - Replaced manual path string manipulation with native python methods (which
    fixed a couple of obscure bugs).

The following functionality has been removed:

  - Scene flattening, --flatten-scene (-f), has been removed as it made a heap
    of assumptions and doesn't really make sense in a *converter*. The task is
    better performed in a 3D modelling package, or even in three.js itself after
    a Scene has been loaded.
@mrdoob
Copy link
Owner

mrdoob commented Jul 12, 2016

Sorry...

  1. I still don't agree with the THREE.Reference approach.
  2. MultiMaterial may be gone soon. Material / MultiMaterial not properly implemented #8924
  3. It would still be better to output BufferGeometry. We are in the process of "killing" Geometry. This PR goes against that.
  4. You should target the dev branch.

@Benjamin-Dobell
Copy link
Contributor Author

Benjamin-Dobell commented Jul 13, 2016

1 . I still don't agree with the THREE.Reference approach.

I'm in no way attached to that implementation detail. In fact in the fork I've been running in production for the last year I'm not taking this approach. Instead I've added a new property to MultiMaterial called referencedMaterials, which is just an array of UUID.

It's just making the best of what is currently available. My referencedMaterials approach doesn't work quite as well, as MaterialLoader can't load my materials, you get an out of bounds error during rendering. However, using this approach the renderer just silently moves on:

Manual scene assembly with MaterialLoader:

screen shot 2016-07-12 at 11 47 55 pm

ObjectLoader:

screen shot 2016-07-13 at 12 37 53 am

The point is that I absolutely don't want my name attached to an implementation that thinks it's okay to repeat the same glass material 50+ times in the JSON file, and then again have 50 instances in memory when that JSON file is loaded. Much in the same way nodes share Material, MultiMaterial absolutely need to be able to share material.

However, that said...

2 . MultiMaterial may be gone soon.

I'd be stoked if that were merged in.

That completely alleviates the need for MultiMaterial at all, in which case we can kiss goodbye to this nasty inter-material referencing altogether.

3 . It would still be better to output BufferGeometry. We are in the process of "killing" Geometry. This PR goes against that.

Interesting. Is there somewhere I can chime in on this?

The internal representation of geometry is an implementation detail, so if anything I think all Geometry should always just have a buffer property and Buffer<X> objects should be kicked to curb. This is how every other rendering and game engine I've ever used works.

The point is that consumers of the API still need a way to access vertices, faces, normals etc. in a logical manner, but what's done behind the scenes is irrelevant. This gives freedom for the internal buffer representations to change without breaking the public API. This is pretty much a must for cross-platform game engines for example, where the ideal buffer formats vary between DirectX, OpenGL, Vulkan etc.

Of course, being in a browser we have real memory concerns - trust me I know, my modellers hate me 😉 and our clients pounce as soon as a model doesn't work in their mobile browser. So that makes doubling up on representations in memory non-ideal. So we'd want to use the buffer representation exclusively unless an API consumer specifically opts into the other representation; meaning they can go ahead and modify the geometry to their hearts content and eventually invalidate it, causing a push to the buffer representation. I, for example, have stretchable walls. I could futz around with the buffer representation, but realistically that's a "computer representation", not a "human representation".

4 . You should target the dev branch.

D'oh. Rookie mistake!

@Benjamin-Dobell
Copy link
Contributor Author

Benjamin-Dobell commented Jul 14, 2016

@mrdoob Would you mind sharing exactly what I should change to get this merged?

  1. I understand you're not a fan of THREE.Reference and honestly neither am I. Would it be best to wait until nodes support multiple materials? If that's the case, would you be interested in pull requests to this effect?
  2. Also, is BufferGeometry an absolute must?
  3. Obviously I'll need to target the dev branch.
  4. There are a few ObjectLoader additions, each of which is split into its own commit. Would you like me to submit some smaller pull requests for those in the meantime?

EDIT: To be clear, I'm referring to support for quaternion and LineSegments, not the material referencing rubbish 😉

@mrdoob
Copy link
Owner

mrdoob commented Jul 14, 2016

  1. There are a few ObjectLoader additions, each of which is split into its own commit. Would you like me to submit some smaller pull requests for those in the meantime?

EDIT: To be clear, I'm referring to support for quaternion and LineSegments, not the material referencing rubbish 😉

Yes please! 😊

@Benjamin-Dobell
Copy link
Contributor Author

Benjamin-Dobell commented Aug 9, 2016

@mrdoob I've implemented BufferGeometry support (not pushed), however my files sizes have increased drastically (around 350% their previous file size).

30MB files are now coming in at 105MB. They're not gzipping particularly well either. However, download size aside; the huge files cause all major mobile browsers to crash, due to high memory usage. This is an issue my team is already well aware of and we're already being careful to avoid.

The reason for the enormous increase in file size is that we've gone from indexed geometry to completely non-indexed geometry. I noticed that BufferGeometry does have an index but I can't find much documentation.

Am I correct in thinking this is just the one index that needs to be shared by all attributes? i.e. the index is only useful when all attributes are the same for a vertex, rather than having the traditional separate indices for normals, UVs etc?

Obviously I need to do something pretty drastic to get the output back down to reasonable sizes. Do you have any suggestions?

@mrdoob
Copy link
Owner

mrdoob commented Aug 9, 2016

Am I correct in thinking this is just the one index that needs to be shared by all attributes? i.e. the index is only useful when all attributes are the same for a vertex, rather than having the traditional separate indices for normals, UVs etc?

Yes, unfortunately. That's how OpenGL/WebGL works 😕

Obviously I need to do something pretty drastic to get the output back down to reasonable sizes. Do you have any suggestions?

We were looking at base64ing the arrays. But it overcomplicates things a tiny bit.

It may be easier to do a binary file (with scenegraph json embeded) directly.

@mrdoob
Copy link
Owner

mrdoob commented Jun 16, 2018

convert_to_threejs.py has been removed in #14308

@mrdoob mrdoob closed this Jun 16, 2018
@mrdoob
Copy link
Owner

mrdoob commented Jun 16, 2018

For alternatives, #13013 proposes a node.js based converter.

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

Successfully merging this pull request may close these issues.

None yet

2 participants