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

Should OBJLoader2 become OBJLoader? #13663

Closed
2 tasks done
kaisalmen opened this issue Mar 22, 2018 · 10 comments
Closed
2 tasks done

Should OBJLoader2 become OBJLoader? #13663

kaisalmen opened this issue Mar 22, 2018 · 10 comments
Labels

Comments

@kaisalmen
Copy link
Contributor

kaisalmen commented Mar 22, 2018

OBJLoader2 is now around for a while and I think that especially the Parser has reached at least feature parity with the other implementation. It currently has less known bugs and the code is now more accessible to others than it has been in the past (code reviews&verification in R90 and R91).

OBJLoader is more widely used and replacing it with OBJLoader2 will only be smooth once the following issues and questions are solved and answers are found together with you:

  • Any external dependency LoaderSupport needs to be removed. The current dev version of OBJLoader2 already solved this.
  • The dev version sill contains the generic MeshBuilder. It provides among other things a mesh building function which uses data directly or data from web worker (buffers as Transferables). It could stay inside OBJLoader, but logically it should be outside violating the first assumption. I currently don't have a good answer for this problem. One solution is to put MeshBuilder inside `WorkerLoader´ where it is needed, but then it cannot be used here. I don't like having code duplicates. Do you at all see that this is a useful feature for other loader or beyond? What's you opinion?
  • Reduce LOCs as much as possible. Due to the second point, more overall features (see table below) and MTL utility function (which should be kept as dependency is not enforced) it is currently twice as big as OBJLoader

Async loading/parsing currently available with OBJLoader2/LoaderSupport will be realized by Generic WorkerLoader.

The following table outlines the major features of both loaders:

Feature OBJLoader OBJLoader2
Handle all face types [x] [x]
Support for points and lines [x] [x]
Negative face indices [x] [x]
Support Ngons [x] [x]
Flat/Smooth Multi-Materials [ ] [x]
Indexed Rendering [ ] [x]
Handle Face-Type change inside group [x] [x]
Deactivate console logging [ ] [x]
Load MTLs via MTLLoader if desired [ ] [x]

Progress can be tracked in the dev branch mentioned above and in the external OBJLoader2 repo.

I hope you agree that the time has come to replace OBJLoader and we can maintain one implementation in the future.

Three.js version
  • Dev
  • r91
@kaisalmen
Copy link
Contributor Author

kaisalmen commented Mar 22, 2018

Description has been updated (overhauled it again this morning).

/cc @mrdoob @jonnenauha

@jonnenauha
Copy link
Contributor

Yes, I'm all for this. It just depends if backwards compatibility is more important to doob than removing old code from the repo.

I assume if the class OBJLoader just gets swapped, is there backwards compatibility in terms of func names, func params and what the funcs return? Or will this break code or force people to port OBJLoader code when upgrading to the release this would be done in? If porting is needed, how big of an effort is it and where will it be documented (release notes etc., your repos wiki or readme)?

@kaisalmen
Copy link
Contributor Author

Functional compatibility parse, load, setMaterials is already provided. It is currently a drop in replacement on dev branch that loads the existing original obj examples.

@kaisalmen
Copy link
Contributor Author

kaisalmen commented Apr 2, 2018

Updated 2018-04-17
I have rebased the dev branch and included a second preview of WorkerLoader.

Multi-usage options example now realized with WorkerLoader where applicable:
https://rawgit.com/kaisalmen/three.js/WorkerLoader_OBJLoader/examples/webgl_loader_obj2_options.html

The original OBJ&MTL example:
https://rawgit.com/kaisalmen/three.js/WorkerLoader_OBJLoader/examples/webgl_loader_obj_mtl.html

OBJ verify:
https://rawgit.com/kaisalmen/three.js/WorkerLoader_OBJLoader/examples/models/obj/verify/verify.html

@kaisalmen kaisalmen changed the title OBJLoader2 should become OBJLoader Should OBJLoader2 become OBJLoader? Jun 14, 2018
@kaisalmen
Copy link
Contributor Author

kaisalmen commented Jun 14, 2018

Hey @mrdoob and @jonnenauha I thought a about the whole thing again. What about we introduced the re-worked version of OBJLoader2 utilizing WorkerLoader first and then we decide this question (I therefore rephrased the tile)?
WorkerLoader took a while mostly due to personal time-constraints, but once the "make-any-loader-run-as-good-as-possible-in-worker" is complete, then it hopefully make people wanna use it. 😉

Status:
https://rawgit.com/kaisalmen/three.js/WorkerLoader_OBJLoader/examples/webgl_loader_obj2_options.html
https://rawgit.com/kaisalmen/three.js/WorkerLoader_OBJLoader/examples/models/obj/verify/verify.html
https://rawgit.com/kaisalmen/three.js/WorkerLoader_OBJLoader/examples/webgl_loader_obj2_meshspray.html
https://rawgit.com/kaisalmen/three.js/WorkerLoader_OBJLoader/examples/webgl_loader_obj2_run_director.html
OBJLoader is unaltered from dev in latest commit. OBJLoader2 now allows to parse and load obj and mtl with same method if MTLLoader is available.

@greggman
Copy link
Contributor

greggman commented Nov 6, 2018

Thought i'd ask what's probably a controversial question but is there any reason to keep the async/sync flag? Why not just always be async? Browser vendors have been trying to get rid of sync IO in browsers for ages. They've printed a warning that sync loading using XMLHttpRequest is deprecated for over 10 years now. I think (could be wrong) the Fetch API meant to replace XHR has no sync option at all.

One of THREE.js's goals is to be simple so maybe it shouldn't provide extra options that lead users down deprecated paths? This is especially true in the ES6/ES7 world where it's trivial to async/await if you want your code to look sync even though it's async.

Even worse, if you're loading any textures then AFAIK it's impossible to be sync so it ends up giving a false sense no? Even using a sync XHR to download the image, putting the image into a texture requires decoding it and both img elements and ImageBitmap are both async for decoding.

Just thought I'd ask.

@greggman
Copy link
Contributor

greggman commented Nov 6, 2018

Actually I think I don't actually understand the async flag. I didn't set it. I tried loading a model. The console prints out useAsync: false but adding in console.log lines in the code it's still async. I think it doesn't actually mean 'async' vs 'sync' it means "use worker to parse file" vs "parse in the main thread".

Maybe consider updating the docs and or renaming the flag to something that's more descriptive of what it actually does?

@greggman
Copy link
Contributor

greggman commented Nov 7, 2018

Maybe I should be opening a new issue but lots of question here

  1. Is using workers really important here? it seems not important to me. Anyone doing actually serious 3D work that wants to load large models and wanted them loaded off the main thread seems like they'd use a better format than .OBJ One where the big data was already in binary and one where there's a scene graph.

    Of course I think it's awesome the work was put in to make a parser that works in a worker I'm just wondering if it's really that important and if it is important it almost seems like all loaders except maybe GLTF need the same work?

    The only place off the top of my head where it's important is a model viewer site where users can drop any .OBJ file? But that seems like a rare use case and doesn't it should be included in all apps using the loader?

  2. Checking the example a .MTL file is loaded before loading the .OBJ but checking my .OBJ file there's a mtllib windmill.mtl file at the top. Should an object loader read that file automatically (maybe via an option?)

  3. I'm trying to write an article on using loading an OBJ file. I'm kind of lost on whether I should be telling people to use OBJLoader or OBJLoader2. If the APIs stay the same then I guess it doesn't matter too much but would appreciate some guidance.

@kaisalmen
Copy link
Contributor Author

@greggman didn't expect that this thread receives much attention again, but it is ok to raise your questions here, I think.
OBJLoader and OBJLoader2 are signature compatible with regard to load and parse. OBJLoader2 provides more features (e.g. parsing in worker and is able to handle intermixed face and vertex definitions, see verify, etc).

To further answer your questions:

  1. Yeah, OBJ is definitely not the format of choice these days. Burt, when I started this worker based loader I exactly wanted non-blocking or minimal blocking loading with OBJ, so I started this work that later ended up here.
    The generic WorkerLoader which could ease to transform or wrap existing loaders in Workers is not done, but I am close to issue a PR with a preview for R99, hopefully. Unfortunately, my spare-time for three.js related work is super limited, currently. It will get better again in December. There are links to examples for WorkerLoader wrappers for PCD and Draco in Generic WorkerLoader #13664

  2. This is ignored by both loaders if I am not mistaken.

  3. As they are signature compatible it basically your choice. The answer on the potential future of both loaders is open. I wanted to get opinions here, but this was not successful so far.

Regarding the async. Yes, it means "use worker to parse file" which is done in parallel, but also not synchronous to main thread. The main receives answers from parser running in the worker. I will think about, whether naming and description can be improved.

kaisalmen added a commit to kaisalmen/three.js that referenced this issue Nov 15, 2018
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
kaisalmen added a commit to kaisalmen/three.js that referenced this issue Nov 17, 2018
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 added a commit to kaisalmen/three.js that referenced this issue Nov 19, 2018
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
@Mugen87
Copy link
Collaborator

Mugen87 commented Jan 12, 2021

Closing, see #21014.

@Mugen87 Mugen87 closed this as completed Jan 12, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

5 participants