mrdoob / three.js

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

OBJLoader doesn't support 3ds Max's # object #6682

Closed makc closed 8 years ago

makc commented 9 years ago

It looks like it assumes "o name" format, but this particular obj file from wavefront does not have them. Its structure looks like this:

# 3ds Max Wavefront OBJ Exporter v0.97b - (c)2007 guruware
# File Created: 11.10.2013 19:47:40

mtllib Alesia.mtl

#
# object Alesia_figure
#

v  14.700874 99.412476 -0.028355
v  7.184631 104.040680 -3.556595
...

so you see, objects are only marked in "comments". Do you think it is good idea to extend regexp to support this?

makc commented 9 years ago

simple replacement # object to o in this file worked for me so I will not pursue this. But in case you want to, the file is here.

RemusMar commented 9 years ago

I don't think you need multiple objects for sub-meshes. You need a single object (for best performances and not only). On the other hand, the OBJ format is useful for static meshes only. cheers

makc commented 9 years ago

How do you know what I need :smile: I certainly do not need shoes and ear rings to be in the same mesh with the body.

RemusMar commented 9 years ago

I certainly do not need shoes and ear rings to be in the same mesh with the body.

For animated models you ALWAYS need a single mesh for best results. Those "shoes" and "ear rings" sub-meshes will be controlled by bones linked to the root bone. The bottom line: you need a single mesh!

cheers

RemusMar commented 9 years ago

How do you know what I need

You need this: http://necromanthus.com/Test/html5/Alesia.html :laughing: But as I said before, the best approach is to export a single mesh. With a "smart" export you can access the sub-meshes at runtime: http://necromanthus.com/Test/html5/Tifa.html Click on the stage to add/remove the sub-meshes.

cheers

makc commented 9 years ago

nice find :+1: indeed they have different obj file with everything merged

makc commented 9 years ago

wait, that file is created today. that's your site, isn't it?

RemusMar commented 9 years ago

nice find :+1: indeed they have different obj file with everything merged

In fact they are 3D artists gurus and programming noobs. :laughing:

wait, that file is created today. that's your site, isn't it?

Just grab that OBJ (file size reduced with 66% keeping the same quality). cheers

makc commented 9 years ago

btw, in that link of yours, open console and type this in:

scene.children[2].children.length

you could be surprised )

RemusMar commented 9 years ago

scene.children[2].children.length you could be surprised )

Do I look like someone surprised? I gave you the "childrens". Now you need to play with them (soccer). :sunglasses:

makc commented 9 years ago

point being your carefully merged mesh is now split in dozen of pieces ) whereas you only needed 3.

RemusMar commented 9 years ago

point being your carefully merged mesh is now split in dozen of pieces ) whereas you only needed 3.

I didn't split or merge anything (that's the original model with a bunch of sub-meshes). I gave you the access to those sub-meshes, that's all.

cheers

makc commented 9 years ago

well it is certainly so in your obj file, but three.js has different vision for this. it crates 15 independent meshes loosely grouped together.

RemusMar commented 9 years ago

well it is certainly so in your obj file, but three.js has different vision for this. it crates 15 independent meshes loosely grouped together.

That's why I've stopped using all the current THREE loaders, except one: SEA3D. The best exporter & loader by far! http://necromanthus.com/Test/html5/testA.html

cheers

mrdoob commented 9 years ago

so you see, objects are only marked in "comments". Do you think it is good idea to extend regexp to support this?

Adding regexp fixes slows down the loading considerable (because it parses the whole file). Maybe we could add options (like... fixRhino = true) and only execute those when enabled.

schubboy commented 8 years ago

You could easily just recognize the static substring "# object" to recognize the line. Also, using a regex to scan a single line isn't that cost prohibitive, especially since its already being used all over the place in the OBJ Loader.

The loader is parsing the file line-by-line, so I'm not sure what you are referring to when you say that the regex scans the whole file?

mrdoob commented 8 years ago

The loader is parsing the file line-by-line, so I'm not sure what you are referring to when you say that the regex scans the whole file?

Oh, in the past, to keep the code clean I used to have some regex that would transform the file. I guess it made more sense with the rhino issue. I guess in this case we could add that regex, but it's dangerous to start supporting non-standard things like this...

schubboy commented 8 years ago

We are utilizing 3DSMax to produce the OBJ file, so even though formatting the object definition line with a "# object" may be non-standard, Max occupies a very large space in the market. If the change were large, I'd agree with the slippery slope analogy, but simply recognizing 2 different ways to call out the object definition seems to be the end of it. For our company, by adding that update, now OBJ is fully supported in the 3DSMax pipeline.

mrdoob commented 8 years ago

How about doing a OBJFixer class? So one can run the file through it before parsing it with OBJLoader.

makc commented 8 years ago

How about .extraCompatibilityOrSomething = true in OBJLoader

RemusMar commented 8 years ago

How about .extraCompatibilityOrSomething = true in OBJLoader

+1

As I said several times before, the backward compatibility is a must in any serious project.

mrdoob commented 8 years ago

As I said several times before, the backward compatibility is a must in any serious project.

As I've also said several times before, maybe this is not the serious project you think it is 😉

mrdoob commented 8 years ago

Trolling aside... Something like loader.strict = false sounds good to me.

However, I would still "convert" the file at load time. I wouldn't want files that follow the standard to load slower because we're trying to support non standard outputs.

mrdoob commented 8 years ago

So, maybe...

if ( this.strict === false ) {

    text = text.replace( /\ \\\r\n/g, '' ); // rhino adds ' \\r\n' sometimes
    text = text.replace( /# object /g, 'o ' ); // 3ds Max exporter

}
makc commented 8 years ago

but is there really a standard? or it's just a bunch of exporters producing different output due to being written by different people?

makc commented 8 years ago

Looking at things like #7943 it sure looks like 2nd case

makc commented 8 years ago

For another example, on my project the artist sent me these files:

# This file uses centimeters as units for non-parametric coordinates.

mtllib BASE.mtl
g default
v 0.087486 0.018835 0.046367
v 0.087486 3.213657 0.046367
v 2.412503 3.213657 0.046367

and

# This file uses inches as units for non-parametric coordinates.

mtllib BASE.mtl
g default
v 1.162508 -1.631096 0.146166
v 1.162508 1.563725 0.146166

I do not think he typed these comments in there by hand, it looks like his exporter stores coordinate units this way.

makc commented 8 years ago

Here is another example of object name in comments:

# File exported by Pixologic 3D Print Exporter for ZBrush
# http://www.pixologic.com
mtllib Base_Frame_Merged_2.mtl

#Object: obj0
#Material used: mat0
#Vertex Count: 3456
#UV Count: 13808
#Face Count: 3452
#Vertex Count: 3456
v 0.087486 0.018835 0.046367
v 0.087486 3.213657 0.046367
v 2.412503 3.213657 0.046367
v 2.412503 0.018835 0.046367
v 0.087486 0.018830 0.086302
v 0.087486 3.213657 0.086302
makc commented 8 years ago

this last file also does this

vt 0.901073 0.745891
vt 0.579899 0.235958
vt 0.579898 0.614333
vt 0.304294 0.614333
vt 0.304294 0.235958
g obj0
usemtl mat0
#Face Count: 3452
f 13/1 9/2 10/3 14/4
f 3177/5 3157/6 3303/7 9/8
f 9/9 3303/10 3301/11 10/12
f 10/13 3301/14 3299/15 11/16
f 11/17 3299/18 3297/19 12/20
mrdoob commented 8 years ago

Exactly. That's why I'm not very keen on trying to support all the variations that people made up in the parser. I would instead have some code that converts so it's cleaner and easier to follow.

schubboy commented 8 years ago

I'm with you on keeping things fast and backwards comparable.

What about a extendedMode = true with a property field for a custom regex expression to apply to the object detector statement? Perhaps a ObjOptions static class with a couple known variations could be done as well. Like THREE. ObjOptions.3DSMaxMode =/# object (.+)/; etc etc. you get the idea.

On Tuesday, January 19, 2016, Mr.doob notifications@github.com wrote:

Exactly. That's why I'm not very keen on trying to support all the variations that people made up in the parser. I would instead have some code that converts so it's easier to follow.

— Reply to this email directly or view it on GitHub https://github.com/mrdoob/three.js/issues/6682#issuecomment-172994413.

-steve

mrdoob commented 8 years ago

loader.config.check3dsmaxobjects = true;?

schubboy commented 8 years ago

Perfect. Solves everybody's issues and maintains performance for "old" behavior.

On Wednesday, January 20, 2016, Mr.doob notifications@github.com wrote:

loader.config.check3dsmaxobjects = true;?

— Reply to this email directly or view it on GitHub https://github.com/mrdoob/three.js/issues/6682#issuecomment-173219737.

-steve

jonnenauha commented 8 years ago

I just suggested something similar in #8383 for how to handle o/g parsing. I think the simplest way to accomplish all this stuff is to take in a options objects to the parse function and document what those switches do. Providing sane defaults (the current behavior) to not break anything.

// defaults to this is options is passed
objLoader.parse(text, {
    ignoreGroups : true // ignores "g" lines as previous releases did!
    readCommentObjects : false // if true read "# object xxx" as "o" lines
});

Edit: Additionally I think the option should be something generic like readCommentObjects which could support various exporters behavior (maybe max is not the only one) instead of starting to make per authoring software booleans for each quirk they have.

jonnenauha commented 8 years ago

This might go a bit off topic but relates to the options object. I would like to see a way to handle comments lines in a controlled manner. Many software put useful metadata in comments. If your app wants to read for example the exporters name and version and show it to the user etc. I also see some geo/gis exporters putting metadata like # File units = meters that some apps might want to utilize.

To extend on the options idea, this would also give you a option to create new object from spesific comments (if readCommentObjects is not implemented for convinience).

objLoader.parse(text, {
    // ... other options from above comment
    onCommentLine : function(objects, object, geometry, material, line) {
        // ability to parse metadata into current 'object/geometryu' that is passed in
        if (line.indexOf("# Exported from ") === 0)
            object.metadata = { exporter : line.substring("# Exported from ".length) };
        // example for creating new objects from comment
        else if (line.indexOf("# object") === 0 )
            // manipulate objects, object to create a fresh object (see ObjLoader internals)
    }.bind(this)
});

Then in code conditionally invoke the function for comment lines

if (options && typeof options.onCommentLine === "function")
    options.onCommentLine(objects, object, geometry, material, line);

This would bring quite a bit of flexibility for app developers, I could even implement all this if you want/don't have time for it :P

mrdoob commented 8 years ago

@jonnenauha all this sounds great to me!

jonnenauha commented 8 years ago

@makc What version of three.js are you using? The current version handles both o and g lines exactly the same. They start a new object that will get its own geometry and material.

If you use the latest release at least for you particular Alesia.obj you linked you don't need to replace # object <name> with o <name>. The file has g <name> lines for each submesh correctly. This is how the current ObjLoader loads it.

image

I will still implement the onCommentLine callback option as there are other needs for it. But for 3ds Max exported files its not needed. I suppose there is some kind of options as blender does if meshes should be separated and how. They seem to use g which is fine.

In fact if you do replace the comment lines with o <name> youll get meshes with zero position geometries, if you look at the file it does not declare any faces after each # object <name> but does after the g <name> lines.

Replacing them with the latest releases loader will result in [.CommandBufferContext]RENDER WARNING: Render count or primcount is 0. warning spam. I have fixed this in my PR so that in the future objects that declare no geometry will be silently ignored.

Summary: The latest loader will load 3ds Max exported obj correctly, provided that it marks submeshes with g as your example file does. The commented # object <name> should not be parsed at all (or converted by hand) as they will produce objects with empty geometries.

makc commented 8 years ago

@jonnenauha you can't expect me to remember, this issue was filed on Jun 12, 2015

jonnenauha commented 8 years ago

Right, I did not check the timeline of the issue that carefully :)

@mrdoob This issue can be closed, the r75 ObjLoader loads the file discussed here correctly. My PR (if merged) will add ignoring empty geometries that will remove warning spam from r76 that users have modified these files by hand (convertin comment lines to o lines) might see in r75.

NguyentuanMec commented 8 years ago

Hi everyone, I had tried to use the OBJ Loader + MTL Loader in threejs.r75 to open this model which I downloaded from Sketchfab.com link model: https://sketchfab.com/models/9a893bbdc050422d95c54f913a5707a2 jolie

This model includes many images image and 2 files (jolie.obj + jolie.mtl) image

But when I used the http://threejs.org/examples/#webgl_loader_obj_mtl to test this model, it showed like this pic_error_1

I also tested this model by Blender software, It was OK, So, what did happen to this model ? Why can sketchfab.com open it clearly ? It seem that OBJ Loader of Threejs just can open the 3Ds Max's file which have only 1 image for material texture.

jonnenauha commented 8 years ago

How does one download that model? I must be going blind, cant find a download link, even after creating account and signing in.

NguyentuanMec commented 8 years ago

Haha, I thought it was a mistake of user who posted that model, it was downloadable on that day, one month ago.

jonnenauha commented 8 years ago

Are you saying the model was already broken by the author when you downloaded it, so probably not a bug in three.js?

To me it looks like UVs are incorrect more than some parsing error on the file, but could do some checking if we get links to the files.

NguyentuanMec commented 8 years ago

Hi, I have uploaded it to my drive, https://drive.google.com/open?id=0B5_pk4KW7XLLamtiaXlaNWg0czg

Everybody can get it and do some checking. I really hope we can fix it and make three.js be stronger !

jonnenauha commented 8 years ago

@NguyentuanMec With my PR https://github.com/mrdoob/three.js/pull/8691 code the model loads correctly. Though her eyes are completely white. Looks like the eyes material is using map_D that does not seem to be parsed at all by mtl loader, so probably that is the reason (?!).

image

NguyentuanMec commented 8 years ago

@jonnenauha :+1: Although it have some unsolved problems but I'm feeling great with your support . Your objloader.js #8691 is so good ! Actually, the Blender software only viewed this model similar to your post (#8691) Many thanks again.