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

DataTextureLoader: Refactor parsing. #17406

Merged
merged 1 commit into from
Sep 3, 2019
Merged

DataTextureLoader: Refactor parsing. #17406

merged 1 commit into from
Sep 3, 2019

Conversation

Mugen87
Copy link
Collaborator

@Mugen87 Mugen87 commented Sep 2, 2019

Same as #17403

@Mugen87 Mugen87 added this to the r109 milestone Sep 2, 2019
@Mugen87 Mugen87 merged commit b7f4a3a into mrdoob:dev Sep 3, 2019
@Mugen87
Copy link
Collaborator Author

Mugen87 commented Sep 3, 2019

Updated builds: f3205d7

@Mugen87
Copy link
Collaborator Author

Mugen87 commented Sep 3, 2019

Finally, all loaders except of OBJLoader2 are now directly or indirectly derived from Loader. The general loader architecture is now more clear and consistent 🎉

@kaisalmen I've tried to change OBJLoader2 according to the rest of the codebase. However, OBJLoader2 is derived from OBJLoader2Parser. Maybe you can find a way to derive OBJLoader2 from Loader instead and use an instance of the parser class as a member variable. I'm aware that OBJLoader2 has a very special design but it still would be preferable to adhere some three.js standards 👍

@mrdoob
Copy link
Owner

mrdoob commented Sep 3, 2019

Finally, all loaders except of OBJLoader2 are now directly or indirectly derived from Loader. The general loader architecture is now more clear and consistent 🎉

Sweet! Many thanks!

@kaisalmen
Copy link
Contributor

@Mugen87 I just recently introduced the derivation of OBJLoader2Parser, so a way back should be easy. It used to be like that in the V3-beta. I think the introduction of Loader is a good idea and OBJLoader2 will be aligned before 109 is due.

@Mugen87
Copy link
Collaborator Author

Mugen87 commented Sep 3, 2019

@kaisalmen Sounds great!

@kaisalmen
Copy link
Contributor

kaisalmen commented Sep 4, 2019

Ported already. Needs some polishing + other issues should to be resolved along:

@kaisalmen kaisalmen mentioned this pull request Sep 6, 2019
6 tasks
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