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

[WIP] WorkerLoader V1.0.0-preview, OBJLoader2 V3.0.0-preview and wrappers #15272

Closed
wants to merge 7 commits into from

Conversation

kaisalmen
Copy link
Contributor

@kaisalmen kaisalmen commented Nov 17, 2018

This was long in the making (#13663 and #13664) and it is not fully done yet, but I want to propose to introduce the WorkerLoader as a preview, so people can start testing it more easily and of course give feedback to the code.

I suggest to keep OBJLoader2 V2.5.0 and LoaderSupport for now. The namespaces of the new and existing things are separated, so they can co-exist. OBJLoader2 3.0.0-preview is called WLOBJLoader2 this shall reflect that it is used in conjunction with the new WorkerLoader and does no longer rely on LoaderSupport which means loaders are not required to extend it. Making a loader to work with WorkerLoader requires it to have a parse function (non-worker usage) and in addition a buildWorkerCode function for worker mode. Missing things can be added in a wrapper, see WLPCDLoader. WorkerLoader opens the door for auto-configured usage of loaders independent of worker usage.

The two base examples for OBJLoader2 remain. The more complex ones and the new once for DRACOLoader (#15249) and PCDLoader rely on WorkerLoader.

This description is far from complete. I will add all the details in update to this post or a later post. Then it will also make sense to ping people.
....
UPDATE: I will not be able to write this down this before the end of the week.

I am currently moving to a new house that's why response time is likely higher and code commit activity is temporarily reduced, but I want to move this forward, finally.

@@ -688,18 +688,14 @@ THREE.OBJLoader = ( function () {
if ( isLine && material && ! ( material instanceof THREE.LineBasicMaterial ) ) {

var materialLine = new THREE.LineBasicMaterial();
THREE.Material.prototype.copy.call( materialLine, material );
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Did you modify this file by mistake?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

When I started my idea was to replace OBJLoader. I reverted in a later commit. Shall I squash parts of the history, so nobody gets confused?

Copy link
Collaborator

@Mugen87 Mugen87 Nov 19, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please revert these changes. Besides, I think it's no good idea to replace OBJLoader. From my point of view, OBJLoader2 is not suitable to do this for several reasons (e.g. the general code style and architecture of the loader).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

First nine commits are now squashed to one (see below). OBJLoader is now untouched, I overlooked that change.

#13663 asks the question whether OBJLoader2 should replace OBJLoader, but there was no comment from you guys.

OBJLoader2 is not suitable to do this for several reasons (e.g. the general code style and architecture of the loader).

Ok, could you please be more specific? On the contrary, the architecture of the original OBJLoader prevents an easy solution of #12597. OBJLoader2's architecture allowed me to solve this problem in a couple of hours...

Copy link
Collaborator

@Mugen87 Mugen87 Nov 20, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry for the lack of feedback but the OBJ format is not that important compared to other 3D formats (like glTF) these days.

In any event, OBJLoader2 does things like input validation (the usage of THREE.LoaderSupport.Validator). In general, three.js does not perform something like that for performance reasons. I also think that the general handling of LoaderSupport feels like over-engineering to me. You clearly overestimate the benefit of workers in context of loading 3D assets. From my point of view the benefit of a generic worker solution for loaders is highly questionable. The only valid use case of workers for me is in context of Draco decompression right now.

@@ -132,6 +130,12 @@ var files = {
"webgl_loader_vrm",
"webgl_loader_vrml",
"webgl_loader_vtk",
"webgl_loader_worker_gltf",
"webgl_loader_worker_pcd",
Copy link
Collaborator

@Mugen87 Mugen87 Nov 19, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I do not vote to add a PCD example. A single glTF example is sufficient from my point of view. We should keep in mind that we have to maintain this code. Please keep the changes lean.

Copy link
Contributor Author

@kaisalmen kaisalmen Nov 19, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, I agree, 95% copied code in an example is bad. A multi-loader example is better if there should be multiple worker compatible loaders that can be run with `WorkerLoader.

LoaderSupport is temporarily back: It contains MeshReceiver and MeshTransmitter: Working title for now. They support sending and receiving meshes from Worker to Main with Transferables in mind.
Fix for PCDLoader. It allows empty undefined urls with parse (required for WorkerLoader)
WorkerLoader now allows to set a MeshTransmitter which allows to put other loaders inside a worker.
Added WIP version of webgl_loader_worker_multi
WorkerLoader.LoadingTask now allows to set a worker code build function independent of the configured loader.
Unified error handling with common _throwError function. A callback used instead if throwing an error. WorkerLoader.Director is now able to continue after a loading error occurred.
webgl_loader_obj2_run_director.html examples allows to enforce sync mode to demonstrate the difference between async loading with workers and sync loading with parse on main.
Allow to force sync loading in LoadingTask
Add convenience methods to WorkerLoader
Rename parserInstructions to parserConfiguration
WorkerLoader: now exposes LoadingTask as and property and via function getLoadingTask.
Clean-up code and simplify code paths.
Adjusted WorkerDirector accordingly.
WorkerLoader needs loader to be set. Then default pipeline can always be used.
WorkerLoaderDirector is now fully working with WorkerLoader.
WorkerLoader/LoadingTask now has fully separated load and parse steps.
LoadingTasks: Callbacks available for completion of each step including overall completion
LoadingTaskConfig is used to configure WorkerLoader in more complex scenarios. Simple loadAsync and parseAsync methods are kept. LoadingTask is used internally, but complexity is completely hidden.
Clean-up API
WorkerLoader relies internally on LoadingTask that encapsulates all runtime information related to loading and parsing assets with a specific loader.
LoadingTask is configured with LoadingTaskConfig. Director uses LoadingTaskConfigs to dynamically feed WorkerLoaders with new LoadingTasks.
WorkerLoader keeps simple parseAsync and loadAsync methods that hides the more complex config, but if required automation is possible with execute and a LoadingTaskConfig
Ported webgl_loader_obj2_options.html to WorkerLoader.
Introduction of WorkerLoader (WIP): webgl_loader_obj2_options.html makes use of async parse and load via WorkerLoader.js
First version of transformed OBJLoader2 without dependencies to LoaderSupport
…ctor example:

 - All functions related to a pool of WorkerLoaders for one specific extension
 - Able handle different Pools with separate queues and configuration
 - Allows to update the worker count while running (see example)
…f parse is performed async only then load is allowed async as well. Only a sub-set of three.js is required to be available in Worker:

These are THREE.DefaultLoadingManager, THREE.Cache and THREE.FileLoader.
WorkerSupport and its default handling class _WorkerRunnerRefImpl now perform init, load and parse, whereas load is optional.
Examples and OBJLoader2 have only been adjusted to support new features or to be compatible with new signatures.
WorkerLoader enhancements:
- Allow to define parser function name.
- MeshTransmitter inclusion and usage parameters have been separated
- Class serialization has been enhanced to include non-prototype functions and properties

webgl_loader_worker_multi.html relies on unmodified PCDLoader.
…ders configurable with WorkerLoader.

Live objects are no longer modified only loader objects in worker are modified.
Defined a WWDRACOLoader that internally uses WorkerLoader to run DRACOLoader in Worker. Interface to GLTFLoader is as is.
Updated WorkerLoader to latest version and introduced a preview version of WLOBJLoader2
Preview of WorkerLoader with examples for implementations or wrappers for PCDLoader, GLTFDRACO and OBJLoader2
Kept two V2.5.0 OBJLoader2 examples.
All loader_worker examples rely on WorkerLoader including the OBJLoader2 examples
Fixed webgl_loader_worker_gltf only using extension of DRACLoader and not worker
@kaisalmen kaisalmen force-pushed the WorkerLoader_OBJLoader branch from c43a7dc to 9a75a48 Compare November 19, 2018 22:06
@mrdoob mrdoob added this to the r100 milestone Nov 21, 2018
@mrdoob mrdoob modified the milestones: r100, r101 Dec 28, 2018
@mrdoob mrdoob modified the milestones: r101, r102 Jan 31, 2019
@mrdoob mrdoob modified the milestones: r102, r103 Feb 28, 2019
@kaisalmen
Copy link
Contributor Author

kaisalmen commented Mar 20, 2019

Hello, I finally woke up from my winter sleep. Sorry for staying silent for much longer than I personally expected. I really started to work on this whole subject again in my WWOBJLoader dev branch repo.

Updated goals are:

  • Make OBJLoader2 completely self-standing (no more LoaderSupport or Validator code deps, asset handling internally), OBJLoader2.Parser has no dependencies to THREE so it is able to run in Worker with least possible foot-print
  • WorkerLoader is actually an Batch/Automation-Loader with possibility to wrap Loaders in Workers or drive existing loaders from external configuration (with code or json/yaml in the future). This should be reflected by name and composition of the different pieces (have smaller less code dependent blocks).
  • Chain different loaders and make assets available from one to the next in a generic way. This could remove MTLLoader wrapper code from OBJLoader2.
  • Make assets available from Main to Worker bi-directional = evolve MeshTransfer to AssetTransfer
  • Modernize code / Transform to jsm

As soon as I have something working again, I will rebase this branch and add new commits.

Are you ok with this PR staying open or should I close it and start a new one later.

Thank you.

@Mugen87
Copy link
Collaborator

Mugen87 commented Mar 21, 2019

Are you ok with this PR staying open or should I close it and start a new one later.

I think it's better to close it and start with a fresh one 👍

@kaisalmen kaisalmen closed this Mar 21, 2019
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.

3 participants