-
-
Notifications
You must be signed in to change notification settings - Fork 35.6k
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
Conversation
examples/js/loaders/OBJLoader.js
Outdated
@@ -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 ); |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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).
There was a problem hiding this comment.
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...
There was a problem hiding this comment.
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", |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
c43a7dc
to
9a75a48
Compare
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:
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. |
I think it's better to close it and start with a fresh one 👍 |
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 andLoaderSupport
for now. The namespaces of the new and existing things are separated, so they can co-exist.OBJLoader2
3.0.0-preview is calledWLOBJLoader2
this shall reflect that it is used in conjunction with the newWorkerLoader
and does no longer rely onLoaderSupport
which means loaders are not required to extend it. Making a loader to work withWorkerLoader
requires it to have aparse
function (non-worker usage) and in addition abuildWorkerCode
function for worker mode. Missing things can be added in a wrapper, seeWLPCDLoader
.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 forDRACOLoader
(#15249) andPCDLoader
rely onWorkerLoader
.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.