search

mrdoob / three.js

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

Enhancement Proposal: Wrap OBJLoader in web worker #9756

Closed kaisalmen closed 7 years ago

kaisalmen commented 8 years ago

Description of the enhancement

I like to propose several enhancements to the existing OBJLoader. The aim is to wrap the existing OBJLoader into a new web worker. I have created a fully working prototype based on the latest dev code: See my fork of three.js The goal is to integrate large OBJ files (>100MB, >1 million triangles) into a scene while the user is able to interact with the scene.

Proposed changes to OBJLoader:

Thanks in advance for providing feedback. This could be my first contribution to three.js. ;-)

Kai

Three.js version
makc commented 8 years ago

OBJLoader: 37363.55ms

that's impresive ) it would be nice if all loaders could be made like that

Usnul commented 8 years ago

it would be quite simple to parse geometry and construct it inside a worker. Especially a buffered geometry, as it could be stuffed into a transferrable - which eliminates expensive deep copy. Materials I'm not so sure about.

jonnenauha commented 8 years ago

We have discussed this many times with @mrdoob in other issues. You should read through https://github.com/mrdoob/three.js/issues/8704

As you can see there, @mrdoob agrees that we could create OBJLoader2. I think for a such big change + refactoring like this it would be better than trying to keep OBJLoader backwards compatible. Easier to start fresh, we can still reuse code from the current loader where it makes sense.

This would leave users that are happy with the current loader in peace, working code. We could log a deprecation warning once the OBJLoader2 has feature parity and we can show it is faster without web worker mode. I think it should support main thread mode as well, an auto fallback for browsers that don't support them and config option if you want to force main thread. For simple models I'm sure the bootstrap time of the worker is gonna be a net loss for total load time if the models are simple.

@mrdoob has also wanted some cleanup, refactoring and simplification for OBJ loader so that it would be easier to approach for new devs. My initial hunch is that this new worker based setup would be a lot more complicated for "noobs" to jump into the development :) At least harder to wrap the whole thing into your head, but still probably as easy to find the code part that needs fixes. I'm all for web workers (as I comment in the issue).

I like your ideas and effort. The demos work great, I did not look at the code closer yet (I will). I especially like the streaming of each submesh when its done, great. I think this should also be configurable so you can turn it off, or maybe even default off. Most users probably would not like to render partial meshes to the end user.

I need to read you code, if you already have this, but there needs to be a configurable worker pool that distributes parallel loads into many workers. Also its important to keep workers alive for a while after its load is done, creating a new worker and loading three.js into it expensive from my experience, sometimes hundreds of msecs (even if the file is in browser cache). I have implemented such a system and would be able to help here.

How about you create a separate repo that does not have the full three.js in it. Just your new loader and its needed bits. You could just pull latest three.js as from npm and make demo/test pages that run it with latest release. I could jump in as a contributor and help you out.

Once its fleshed out we could import it as OBJLoader2 into three.js repo. In fact, users who really need this could get the loader from the separate repo right away to test and give feedback. As the loader does not mod any core parts, a separate repo would be better imo. This would give us ability for our own release cycle, issue tracker etc.

it would be quite simple to parse geometry and construct it inside a worker. Especially a buffered geometry, as it could be stuffed into a transferrable - which eliminates expensive deep copy. Materials I'm not so sure about.

Yes the no copy transfer/transferable is a must for this kind of loader. I have implemented this kind of worker before, imo its best to submit each attribute separately and construct a new buffer geometry in main thread (just add the received attrib/buffer as is).

I would probably tranfer material metadata as an object to the main thread and create materials there. Though you could just serialize the worker created material as JSON and deserialize it back in the receiving end. I believe toObject is a standard API for pretty much everything, but would need to see if it is smart enough to omit default properties to reduce the object size. If it does serialize everything, submitting a simpler metadata would be better.

kaisalmen commented 8 years ago

Thank you all for providing feedback. Give me some time to process the information. I wasn't aware about #8704. It seems that I didn't check enough issue history. A thorough answer will follow.

Usnul commented 8 years ago

Workers are not free, if you want them as part of the core library - you need management code for it at some point.

Workers currently don't allow live-code transfer, so you have to either load a separate .js files inside a worker or pass all relevant code with dependencies as a string into the worker and then use "new Function(codeString)". You could also use a URL blob (which is what will be used to create the worker initially, i suspect).

Spawning too many workers, or keeping them alive without use is probably not a good idea - there is a penalty depending on browser/device. Just something to think about, more long-term.

kaisalmen commented 8 years ago

@jonnenauha, yes keeping backward compatibility is important and I agree that it is a good idea to create an OBJLoader2 and also to do it in an independent repository as you suggested: I have created a repository for OBJLoader2 prototyping. It will be filled with content/examples in the upcoming days during my spare time.

What I already achieved with my modified version of the OBJLoader is that it can be used independent of a wrapping web worker. You are still able to use it without touching any code. The WWOBJLoader is just an extension. I needed to move code into new logical blocks which in the end is a fairly heavy modification to the existing OBJLoader.

With OBJLoader2 we are able to implement more fundamental changes to the existing loader. If I am not mistaken the processing of the OBJ file content is fairly robust now as the code has received various bug fixes. So, the core processing code should stay, but we could think about optimizations (data structures, etc.).

Objectives for OBJLoader2:

Objectives for web worker wrapper/worker controller:

That's it for now. I have added the objectives to the readme of the bespoke repository. Should we continue the discussion here or move to a new issue in the new repository and just add status updates here?

kaisalmen commented 8 years ago

The simple example using male02.obj is now ported and available in the new repository.

kaisalmen commented 7 years ago

It took a while, but I have first results and want to share how far I have come. Feedback is very welcome.

2016-11-06: Status update

Some thoughts on the code:

Approach is as object oriented as possible: Parsing, raw object creation/data transformation and mesh creation have been encapsulated in classes.

Parsing is done by OBJCodeParser. It processes byte by byte independent of text or arraybuffer input. Chars are transformed to char codes. Differnet line parsers (vertex,normal,uv,face,strings) are responsible for delivering the data retrieved from a single line to the RawObjectBuilder.

The RawObjectBuilder stores raw vertex, normal and uv information and builds output arrays on-the-fly depending on the delivered face information. One input geometry may lead to various output geometry as a raw output arrays are stored currently stored by group and material.

Once a new object is detected from the input, new meshes are created by the ExtendableMeshCreator and the RawObjectBuilder is reset. This is then just a for-loop over the raw objects stored by group_material index.

Memory Consumption

Only 60% of the original at peek (150 MB input model) has a peak at approx. 800MB whereas the existing has a peak at approx. 1300MB in Chrome.

Performance

So far, I only ran desktop tests: Firefox is generally faster than Chrome (~125%). 150MB model is loaded in ~6.4 seconds in Firefox and ~8 seconds in Chrome. Existing OBJLoader loader takes ~5.1s in Firefox and ~5.3s in Chrome. Tests were performed on: Core i7-6700, 32GB DDR4-2133, 960GTX 4GB, Windows 10 14393.351, Firefox 49 and Chrome Canary 56. Biggest room for improvement: Assembling a single line and then using a regex to divide it, seems to be faster than evaluating every byte and drawing conclusions. This is not what I expected. I will write a second OBJCodeParser that works differently. From my point of view OO approach is not hindering performance.

Examples:

Existing OBJLoader

New OBJLoader

Larger model not in the prototype repository (27MB zip): Compressed 150MB Model

Usnul commented 7 years ago

Sounds really good! Regarding OO approach, one thing to be careful about is garbage collection, this is where performance can take a sharp hit. 800MB memory usage is curious, it's good that it takes less, but would be interesting to know how it scales (e.g. 800MB = X*a + b, what are X,a,b?).

kaisalmen commented 7 years ago

@Usnul: Hope this answer makes sense. :-) During parsing the max. amount of heap is used when a the biggest object is transformed (2140212 vertices in the benchmark example) from input to raw mesh data.
b: Data delivered by FileLoader (previous XHRLoader) is base memory footprint and only exists while FileLoader is kept in memory (subject to GC afterwards). My observation: Chrome does not flush this data immediately. Changing tabs sometimes enforces this.
Size of single input object from OBJ file (Varying X) + Size of raw mesh data (1.x * input data) (a): Varying X: Input Data is immediately disregarded as new RawObjectBuilder is used (subject to GC). a0 to aN: Output data is bound to the BufferGeometry. This size increases from start to end of parsing (not on heap as you can see below).

This is what happens in Chrome: ptv1perf

kaisalmen commented 7 years ago

With the updated I just pushed to WWOBJLoader Prototyping speed is now similar to the existing implementation: Both Firefox and Chrome parse the 150MB model under 5 seconds on my dev machine (100-200ms difference to existing OBJLoader)!

Update 2016-11-11: Ok, I think, it will now become very hard to improve parsing speed further. New OBJLoader is on average now slightly faster then the existing implementation and this was already really fast! I took the fastest parse time out of three runs each: New OBJLoader: 3835.16ms (Firefox 64-bit 49.0.2) New OBJLoader: 4251.402ms (Chrome 64-bit 56.0.2916)

Existing OBJLoader: 4102.86ms (Firefox 64-bit 49.0.2) Existing OBJLoader: 4429.495ms (Chrome 64-bit 56.0.2916)

Btw, these things improved performance most:

jonnenauha commented 7 years ago

Cool stuff, good progress, I'll take a peak at the codebase. At this point I'm mostly interested in the new parser code. The web worker threading is ofc a cool addition.

Edit: The multi-material feature is quite a big one. I'll see if I can help out with that. If I make a PR can I push new example assets? I could also make some kind of unit test system with grunt that would run all the files and verify the thing is still working :) Quite useful with bigger changes.

jonnenauha commented 7 years ago

What is the latest code I should read, ObjLoader3.js? The WWCommons.js still pulls in ObjLoader2?

The structure of the repo is kind of confusing, some suggestions.

  1. Latest three.js release should be fetched with npm or bower.
  2. New source code this library provides should be in /src. ObjLoader2/3 are just two different versions of the same thing? I think the final thing should be called ObjLoader2. When you are done experimenting there should be one file imo.
  3. Examples in /examples could be splitted into /test and /assets. The old obj/mtl loaders could be in /test/three.js.old or something like that. Not with /src.

Then implement tests you can run with shell eg. grunt test that executes .js tests that report the load times etc. for both old and new loader.

grunt build should produce a /build dir that contains one file that has all the things this library needs. Both human readable and minified versions.

Finally repo/library also needs a better/catchier name. "Hey dude are you using the new WWOBJLoader?" does not exactly roll off the tongue easily :) ObjLoader2 is kind of boring too though, but conveys the message clearly to three.js end users. The web worker part does not need to be in the name, its just one features, as you have improved the main thread performance as well (at least what I'm reading here).

Edit: If you agree on the points I could send a PR to you with at least some of the changes. The repo is still small so it would be easy to do at this point.

kaisalmen commented 7 years ago

Ok, I took the three.js repo layout as blueprint. OBJLoader3 is the one correct one! OBJLoader2 was the original proposal (starting point for this issue). Just pushed an update: Multi-material code is there, but there are some index vertex index issue with the MultiMaterial.

jonnenauha commented 7 years ago

Yeh. I think what at the end of the day goes to three.js is one or a few new files, if its decided to be merged there. Those should be the non-minified build artifacts from this lib repo. So the structure does not necessarily need to be similar. Why loaders are in examples in three.js is because they are not bundled in the build, but separately provided examples (even if they are the defacto loaders most users use).

jonnenauha commented 7 years ago

With quick testing I'm not seeing the perf gains. For models/obj/cerberus/Cerberus.obj three.js master OBJLoader ~100-150msec, this lib ~150-250 msec. I think the perf can be improved but I need to take a closer look. Does your figure actually parse/process the file twice? I saw some funcs that counted things like objects/materials prior to the parsing?

General observations: You have prepared all the classes to be very configurable. Most of the operational modes can be turned on/off or you can provide you own custom functions. To the point where you are wrapping the default built in function call for fetching a single byte out of the array buffer :) This might be a bit overkill, as its done thousands and thousands of times inside the parser.parse. We need to look at the hot path and make it fast.

You are also newing up a lot of "complex" objects in the start of each parse. This is probably fine, as they encapsulate state. In the parsers you are again mapping/calling lots of functions to get back to the main parser.

var parsers = {
    void: new LineParserBase( 'void' ),
    mtllib:  new LineParserStringSpace( 'mtllib', 'pushMtllib' ),
    vertices: new LineParserVertex( 'v', 'pushVertex' ),
    normals:  new LineParserVertex( 'vn', 'pushNormal' ),
    uvs:  new LineParserUv(),
    objects:  new LineParserStringSpace( 'o', 'pushObject' ),
    groups: new LineParserStringSpace( 'g', 'pushGroup' ),
    usemtls:  new LineParserStringSpace( 'usemtl', 'pushMtl' ),
    faces:  new LineParserFace(),
    lines:  new LineParserLine(),
    smoothingGroups:  new LineParserStringSpace( 's', 'pushSmoothingGroup' ),
    current: null
};

....

function LineParserBase( name, robRefFunction  ) {
    this.name = name;
    this.robRefFunction = robRefFunction;
}

function LineParserVertex( name, robRefFunction ) {
    LineParserBase.call( this, name, robRefFunction );
    this.buffer = new Array( 3 );
    this.bufferIndex = 0;
}

... later on results

robRef[ this.robRefFunction ]( this.buffer );

I think at the end of the day, as discussed elsewhere, this boils down to

The mem footprint of arraybuffer should be better as we have more control, but the speed does not seem to be there at least for this low/moderate sized mesh. Though I would guess arraybuffer parsing is faster, its just some other stuff (maybe points above) that is slower.

Simplifying the whole chain a bit and not wrapping functions over single line execs might belp. I cant really see clearly where the time is spent with chromes profiling tools at this point. Need more investigation and reading the codebase.

Edit: From my point of view, at this moment this codebase is more complex than what it is trying to replace. Not saying its a bad thing though, but it was one of mrdoobs points to make the library more approachable, which the regexp soup certainly is not either :) I would maybe start by splitting each of the classes into separate files, easing developers to first of all read the whole thing, instead of jumping inside a one big file. Could use the same module require that three.js itself is now using and the same tools to produce the final build.

Itee commented 7 years ago

Open parenthesis (

Why loaders are in examples in three.js is because they are not bundled in the build, but separately provided examples (even if they are the defacto loaders most users use).

@jonnenauha What think mrdoob and other "mains" contributor of three.js about keep really useful stuff in example directory ?

Due to es6 module and rollup with threeshaking only required stuff will be bundle, so it's (in my view) a non sens to keep this stuff out of the lib... If you have some ref issue about that, i will read them with attention, thank.

) Close parenthesis

@kaisalmen Really good job ! I will forked your repo as soon as possible to watch in deeper the added stuff. I think due to "imminent" es6 refactoring of three.js may you should consider to make an ancestor class where all loader will could inherit from, in view to inherit webworker stuff easily on every other loader.

kaisalmen commented 7 years ago

Good evening, sorry, I was not able to answer earlier.
@jonnenauha, thanks for your comments. You pointed at one thing I did not focus on during development so far: Initialization is taking quite some time and therefore the parsing time for small models goes up. I did the same test you performed and I can confirm the results. I focused on big models files where the time for init phase becomes almost negligible and where overall parsing and mesh creation speed is as described before.
The various extension of LineParserBase seem over-engineered and should be reduced in complexity. I wanted to build small logical blocks that are easy to understand and maintain, but I guess the result is not what I wanted to achieve in the first place. I agree, re-work is required here!
But, this is only one aspect of new the loader and unfortunately you did not provide feedback on the other parts (2 and 3):

  1. Input Parsing (OBJCodeParser) creates a
  2. "normalized" object representation (RawObjectBuilder) that is
  3. used to build the meshes (ExtendableMeshCreator) The "normalized" representation of the parsed data in RawObjectBuilder and indexing of geometric data according object/group/material/smoothing depending on the configuration allows creation of meshes and material become fairly easy in ExtendableMeshCreator. Just by changing the way the index is created (e.g. treatment of smoothing groups) the ExtendableMeshCreator receives different input. Because the contract between RawObjectBuilder and ExtendableMeshCreator is so simple extending it for the web worker is also straight forward.
    Points 2 and 3 are a lot easier to understand in the new implementation and fairly confusing in the existing one. I personally think these parts are easier to understand for other/new developers. Part 1 did not improve the situation, so far. Sorry for that! ;-)
    Worklist for me:
    • Improve the parsing (point 1) with focus on speed for small and large OBJ files.
    • Consider splitting code to different files, because:
    • web worker is then only dependent on Cache, LoadingManager and FileLoader (as far as I can see now)
    • Give ideas for general web worker approach for other loaders
    • Further improve documentation
    • Work on repository structure and provide easier access to tests / provide evidence of performance

P.s.: Screenshot: I did a page reload when I a captured the timeline with Chrome that's why things seem to appear twice.

kaisalmen commented 7 years ago

@jonnenauha I have overlooked some of your edits:

Edit: If you agree on the points I could send a PR to you with at least some of the changes. The repo is still small so it would be easy to do at this point.

Yes, this sounds great and I would appreciate it.

kaisalmen commented 7 years ago

Ok, the missing features have been implemented, but I face an issue with MultiMaterial I don't understand. @jonnenauha I know this is not a help forum, but eventually you have an idea. I implemented it very similar to you. The BufferGeometry vertex, normal and uv arrays are filled with multiple BufferGeometry.set instead of wrapping an existing array:
Only the first materialIndex is used when I set BufferGeometry.addGroup( start, count, materialIndex ). In my test a MultiMaterial with three materials is created, but all three vertex groups that are defined with addGroup use the same material (0). If I set the materialIndex in addGroup to fixed 1 or 2 for all vertex groups then all faces have material from index 1 or 2. So, MultiMaterial is defined correctly, but somehow BufferGeometry ignores the materialIndex. This is the code: OBJLoader3.js ll. 1088-1178 Any ideas?

Some insights on how the obj data is processed. "Female02.obj" defines the first object like this (for now the code treats all 'g' as one object when useMultiMaterials is set):

g groupA
usemtl matA
s 1
f many lines...
g groupB
usemtl matB
s 2
f many lines...
s off
f one line...
s 2
f many lines...
s off
f one line...
s 2
f many lines...

will result be internally stored as (with vertices, normals and uvs ordered accordingly):

groupA|mat_A|s1 (matA with smooth shading)
groupB|mat_B|s2 (matB with smooth shading)
groupB|mat_B|s0 (cloned matB with flat shading due to 0/off)

If useMultiMaterials is true then one Buffergeometry with MultiMaterial and corresponding groups is created. If useMultiMaterials is false then three BufferGeometry with one Material each is created.

kaisalmen commented 7 years ago

First preview of WWOBJLoader+OBJLoader3 which allows to load multiple big OBJ files (zipped) is available here: WWOBJLoader Testbed Enjoy, but beware, if you load all available files, your GPU needs approx. 800MB and the browser will require more than 1.2GB of memory. wwobjloader Some issues around materials are not yet resolved and the parser code is still untouched. Good news is that when OBJLoader3 is reused for loading by WWOBJLoader the parsing is faster (no new heavy init-costs). WWOBJLoader and its front-end need some work as well. Getting there... :-)

kaisalmen commented 7 years ago

I have completed the parser rework. The parser code is a lot simpler and easier to understand, I think. Performance is there for small and large models. A Firefox 50.0.1 (64bit) fresh browser instance parses the 150MB model in 3.2 seconds:

Global output object count: 2127
parseArrayBuffer: 3216.56ms

Chrome is a little slower (57.0.2936.0 canary (64-bit):

Global output object count: 2127
parseArrayBuffer: 4088.800ms

Some random thoughts:

Next:

kaisalmen commented 7 years ago

@jonnenauha: I have adjusted the repository structure Edit 2016-12-01:

Edit 2016-12-02:

Edit 2016-12-04:

Edit 2016-12-11:

kaisalmen commented 7 years ago

The fun begins:
Web Worker OBJ Parallels Demo

ww_parallels

Edit 2016-12-16:

kaisalmen commented 7 years ago

Hi all, I didn't have much time to work on this the last week, but I fixed some bugs on the demo and the web worker director class: Web Worker OBJ Parallels Demo

Now, you can see the completion status of every web worker used: ww_parallels_3

I think, the code is feature complete and I would like to receive some feedback from you.

Any feedback on the code and the demos is very welcome!

Edit 2016-12-27: As promised some words providing an overview

In contrast to the existing OBJLoader the new OBJLoader2 consists of three pieces:

What is the reason for separation?

The loader should be easily usable within a web worker. But each web worker has its own scope which means any imported code needs to be re-loaded and some things cannot be accessed (e.g. DOM). The aim is to be able to enwrap the parser with two different cloaks:

  1. Standard direct usage
  2. Embedded within a web worker

As OBJParser is independent of any other code piece of three.js or any other library, the surrounding code either needs to directly do the required integration (OBJLoader2 and OBJMeshCreator) or WWOBJLoader and the communication and data proxy (WWOBJLoaderProxy) ensure it. WWOBJLoaderProxy basically provides the same functionality as OBJLoader2 and OBJMeshCreator, but the work is done by the web worker.

WWOBJLoaderProxy is extened from WWLoaderProxyBase. The base defines the plan for usage of the proxy. One idea is to build other proxies for other web worker based loaders and the other idea is automation and orchestration.

Directing the symphony

WWDirector is introduced to ease usage of multiple WWOBJLoaderProxy. It is able to create a configurable amount of loader proxies that extend WWLoaderProxyBase via reflection just by providing parameters. An instruction queue is fed and all workers created will work to deplete it once they have been started. The usage of WWDirector is not required.

Parser POIs

The parser and mesh creation functions have reached full feature parity with the existing OBJ loader. These are some interesting POIs:

Improvements

Examples:

Web Worker OBJ Parallels Demo WWOBJLoader OBJLoader2 Original OBJLoader

Larger models not in the prototype repository: Compressed PTV1 Model (150MB) Compressed Sink Model (178MB) Compressed Oven Model (150MB)

jonnenauha commented 7 years ago

Demo is very cool, great progress :)

@mrdoob should we start considering at some point to copy this new ObjLoader2 into three.js. I like that it is in its own repo, but it would be easier to discover from this repo. Could we still dev this in the separate repo, take issues there and at times copy the files as a pull request to three.js?

I don't really know how we should go forward with this. Should it be left as its own repo, have its own issues etc. I mean the loader is not part of three.js core, but it would be shipped as external files that would be handy for devs. Also being ObjLoader2 it won't break existing users, people can port to it when they like with quite minimal changes, a bit more changes if they wish to utilize the web-worker.

git submodules are a mess and a hassle for devs. I would not take that route myself.

kaisalmen commented 7 years ago

I would not go for git submodules either. What about I create an npm package? @jonnenauha do you see need for renaming things? I have updated the previous post with the promised overview.

jonnenauha commented 7 years ago

So that we don't break code I would probably rename the JS object from THREE.ObjLoader to THREE.ObjLoader2. Keeping it the same name as the current loader could also be considered if the entire v1 API can be found from the v2 loader. At the end of the day the user including ObjLoader2.js is a clear decision to use it, but if it needs porting the app code to new funcs etc. it might be best to rename to not throw off people?

I think the best route to intergrate this library into three.js is to produce a single file, instead of multiple that need the correct load order. This can be done easily with gulp/grunt/whatever task runner in your repo. eg. grunt build would produce ./build/ObjLoader2.js without minification and ./build/ObjLoader2.min.js. The non minified version could then just be copied to three.js as a single file. And users that prefer using your repo (eg. want to update to latest release before updated to main three.js repo) they can pick it up from there or just use npm to fetch the library to their application.

I would not commit the build files into the repo, like three.js does. I think they are committed for convenience in three.js, but more convinient would be to just make a github release on each release tag. But this is a personal choice you can make, I just don't like the idea of build artifacts in the repo that can be produced with scripts.

So the build exec would concat these https://github.com/kaisalmen/WWOBJLoader/blob/master/test/webgl_loader_objloader2_direct.html#L93-L95 so users dont have to include lot of files in the right order.

For the webworker stuff you could make a second build output ObjLoader2.WW.js that concats https://github.com/kaisalmen/WWOBJLoader/blob/master/test/webgl_loader_ww_parallels.html#L91-L93 and whatever else is needed (probably needs ObjLoader.js stuff as well).

I can make the build scripts with gulp (seems to be preferred nowadays from grunt). I believe you can also instruct npm to run the build when the package is fetched (not sure, if not maybe best to put the build files in the repo after all?).

To summarise my thoughts

Once the build step is there, we could also enable travis-ci to run the build and run tests the build against files in the repo. This can be added later.

jonnenauha commented 7 years ago

Just realized you import the parser in the script that gets loaded into the webworker https://github.com/kaisalmen/WWOBJLoader/blob/master/src/loaders/WWOBJLoader.js#L11

This should work ok but maybe we could also make the build to copy the parser file instead of import. This makes one network request go away and people wont need to hassle with paths in production. I believe ./my.js will be resolved from the page/path where you are, not where the script was imported from (eg. CDN)? But if that works with your example pages it probably is then resolved from the ww script ref :P

kaisalmen commented 7 years ago

Basically, I agree on the plan. Just, the web worker stuff needs to be split into to files as the web worker code must be isolated if I am not mistaken. You could stringify the complete web worker code, include it in a another file and do some magic, but I don't want to got that way for multiple hundred lines of code.

I am not familiar with Gulp, yet, but I will dig into it and make it work. :-) As you may have guessed by looking at the code my main language is Java. I am diving into the JavaScript world during my spare time for now. Build-automation is very well known to me, I am just used to different recipes and ingredients. ;-)

Anyway, I will start with renaming. Time may be a little short the next couple of days, but I will keep you posted.

Cobertos commented 7 years ago

On the topic of having to separate the worker into a separate file...

There's a technique I saw on MDN, Embedded Web Workers. Looks like you can put the code in a script tag (with a non-executing type, probably best to use an x- type to be actually valid like shaders do), make a Blob with the correct MIME type, and then use URL.createObjectURL to create a data URL from the blob, which can then be fed to new Worker().

Perhaps you might even be able to make use of Function.toString() to put the code in a function, get the source representation of it, and then make a blob from that. According to this Stack Overflow question it is non-standard though but might be useful with enough error checking. Then there would be no need for separate files.

kaisalmen commented 7 years ago

@Coburn37 thank you, I will investigate and see if one of the strategies could be used. One file containing all code has its charm. @jonnenauha is the namespace THREE.WebWorker ok for the web worker code? I have introduced it and is not yet used in three.js.

jonnenauha commented 7 years ago

Yeah the blob approach works but not in all browsers three.js supports. I have used it before and believe Firefox and IE (<11?) had problems with it. Not sure about current state. There could be an optional file to include as ./build/WWOBJLoader2.embed.js that would just .toString() the whole thing into a global variable that can later be used to load it as a blob :) I would probably leave this out to start with...

@kaisalmen Just concat the webworker stuff instead of importScript, then provide plain and minified versions of all. I just feel the importScript parts will give users more problems, you have to option to make it dead simple.

Sane defaults are important too. If user includes minified version he expects it. So the min version of the library needs to load the minified version of the webworker js file. Same for non minified. You can do these transformation in your js task runner (gulp).

The worker namespace I don't know. I somehow have a feeling three.js wont be doing lot of webworker stuff (people here cant agree on something like this that needs strong opinionated direction in the dev, everything needs to be so generic that it works well for everyone). So probably wont be made a generic utility in three.js any time soon. Maybe but those as to THREE.ObjLoader2.WW.?

Cobertos commented 7 years ago

Just looked up the compatibility:

@jonnenauha Looks like the only browser unsupported that is supported by WebWorkers is IE 10. So I guess that's that.

jonnenauha commented 7 years ago

Right, I mean the ctor for taking in a blob did not work in all browsers back when I tried it. Looks like it was only IE10. I would still leave it for later, first get things going with the builds.

kaisalmen commented 7 years ago

Edit 2017-01-05: Two bundles are available:

Edit 2017-01-07:

Edit 2017-01-08:

Edit 2017-01-09:

Primary mission objectives complete ... now let's find the bugs. ;-)

kaisalmen commented 7 years ago

@jonnenauha, @Coburn37, I think this is ready for usage in three.js! What do you think?

Cobertos commented 7 years ago

I took a deep dive into your repo and here's what I came out with. Correct me where I'm wrong because I'll probably need to integrate this into my project at some point and because I feel as though I might not be getting a good handle on some of the portions of the API. Also, the biggest issue I found was the API (again, I might've just read it wrong) and some of the redundancy/complexity of the names.

There are a few workflows to this:

Questions:

Concerns:

Other than that I think this is great work. If we end up switching off of OBJ to FBX I might end up having to port this to FBX too :P

kaisalmen commented 7 years ago

@Coburn37 Thank you for your comments. You get a thorough answer tomorrow. I need to go to bed. I just realized that there is no simple example for WWOBJLoader2Proxy. I used the director in both cases. More tomorrow...

kaisalmen commented 7 years ago

The comments where really helpful and I have already started to prototype some ideas and thought about improvements where needed: Branch polish

Using THREE.OBJLoader2 directly

This does not apply only here: I was thinking to either make non-public functions like validate and finalize private to the object and passing itself to it or to mark non-public methods with a prefix or suffix. Do you have any suggestions here what is a good and common practice?

Using THREE.OBJLoader2.WW.WWLoader2Proxy for the web worker version of OBJLoader2

Using THREE.OBJLoader2.WW.WWLoaderDirector to use a pool of multiple web workers (inferred from example code in WWOBJLoader2Verify.js

Questions: What's the difference between buildMesh in WWMeshCreator and OBJLoader2MeshCreator? They look kind of the same but I can't think of any reason they'd need to be different except that WWMeshCreator would need to relay the data back as primitives or something because of web worker communication?

When you say "Face N-Gons are not supported identically to the old parser" in your documentation, do you mean that n-gons are not supported, just like the old parser, or they are supported but just not like the old parser did it.

ZipTools and other dependencies are only used for the tests and examples, correct? I don't want to have to depend on these to use them.

Edit 2017-01-11:

Concerns:

The following is addressed on branch polish

I have started to rename private methods and also started to moved functions up and down within classes to better follow the logical workflow. This is not yet completed. Documentation for public functions is also on my worklist.

jonnenauha commented 7 years ago

I see mostly underscore prefixing for private stuff, may it be property or function. I'm not sure if three.js has such coding conventions, they tend to at least wrapping functions for private variables they don't want to leak out. I would go with _finalize, this should tell people not to call it.

And of course leaving it out of the docs is a strong signal too :) I recommend you use jsdoc3 http://usejsdoc.org/ with https://www.npmjs.com/package/gulp-jsdoc3 or https://www.npmjs.com/package/gulp-jsdoc-to-markdown or similar. Produce the docs in the build step to /build/doc. Make your uglify/minify phase remove all comments from the source code (should do that by default).

Cobertos commented 7 years ago

I agree with @jonnenauha that prefixing with _ and leaving it out of the documentation would signal this well enough.

As for the renaming, I think that what you did in the polish branch is good. I saw you took out the root examples folder and consolidated everything into test which I think serves the purpose well. Your explanations helped with the other portions of my confusion as did the name changes. There's still a few little nitpicks I have from the previous post but give it some documentation and IMO it should be good to go.

kaisalmen commented 7 years ago

@jonnenauha and @Coburn37 I agree with your proposal to prefix private methods with _ (see edit of previous post). Doc will come once I have wrote all required function comments.

Completion and integration is near. Feels goods. Thanks for your support so far! 👍

kaisalmen commented 7 years ago

Public function documentation and identification of private functions via prefix is complete. I renamed some methods to make things clearer and fixed minor issues. gulp-jsdoc3 processes the README.md, but no function comments are extracted from the js files. I need to investigate and understand what's wrong...

Edit 2017-01-14: Okay, I found why jsdoc does not generate code doc: singletons need extra tags to be processed properly and I used them everywhere. ;-)

Edit 2017-01-16:

kaisalmen commented 7 years ago

Documentation is complete! I clarified minor things in the tests/demos and performed minor adjustments to the code (e.g. param objects). @jonnenauha and @Coburn37 do you have further comments? Otherwise, this is now ready for integration, I think.

Edit: Each example needs to be flattened (all code inside a single HTML page) to be in-line with other three.js examples, correct? I still need to do this work.

kaisalmen commented 7 years ago

Example changes:

Cobertos commented 7 years ago

Looks consistent in class names, file names, etc. I'm really liking this. My final suggestion would be to move the app directory out of src and into test if you can unless app is relevant to the actual OBJLoader2 (which from when I was reading, I thought it was just a tool for your own examples).

I'll have to integrate this with my app shortly. Nice work!

kaisalmen commented 7 years ago

Thank you! I moved all files from src/app to test/common. Loader code does not depend on these classes. It is only support code that is used by the tests and they will very likely become superfluous once wwobjloader2complex and wwparallels have been modified like the two simple ones.

jonnenauha commented 7 years ago

Nice work. If you feel its ready, make a proper release. Do the gulp build and make a zip file that has the ./build contents and upload it to the github release. This is faster way of people getting the library than clone > jump to tag > build. I will also give this a go at work a bit later when I get back to the relevant tasks/projects.

I would make it 1.0 and be done with it. If @mrdoob wants to slurp the library here, he can always use the latest tag from your repo. Imo its kind of irrelevant if it ends up here, it would be nice, but it can also live in the separate repo. We just need to advertise it to three.js users.

Remove the "prototyping" from the repo description too :)

I'm not sure what the best channels to let people know that this exists. Maybe @mrdoob can tweet the repo link :)

kaisalmen commented 7 years ago

@mrdoob, @jonnenauha and @Coburn37 I have completed the work on the examples: gulp will now produce single page versions of all examples.

WWOBJLoader has now reached version V1.0.0

I have uploaded a zip of the build folder to the release on github!

Please RT if you like!

Cheers :-)