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.

Sign up for GitHub

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

Jump to bottom

LDrawLoader: 2 bugs and 7 improvements #24257

Merged
merged 9 commits into from
Jun 28, 2022
Merged

LDrawLoader: 2 bugs and 7 improvements #24257

merged 9 commits into from
Jun 28, 2022

Conversation

yomboprime
Copy link
Collaborator

@yomboprime yomboprime commented Jun 20, 2022 •
edited
Loading

@gkjohnson

Related issue: None

Solved two bugs and made 7 improvements to LDrawLoader.
Bugs:

  • In parse(), missing call to applyMaterialsToMesh()
  • Missing assignment of userData.colorCode in conditional edge material.

Improvements:

  • Changed the order of search of files in the defaults directories, which improves load time.
  • Added 4 fields to userData: Author, fileName, type and colorCode.
  • If a color is not found, instead of giving error, give a warning and return a pink material.
  • Luminance parsing improvement (when the value is in the token, i.e. LUMINANCE15)

I'm sorry I made it separated by commits but I lost that repository. It is all in one commit.

@gkjohnson
Copy link
Collaborator

For testing:

https://raw.githack.com/yomboprime/three.js/ldraw_bugs2/examples/webgl_loader_ldraw.html
Nice work!

@mrdoob mrdoob added this to the r142 milestone Jun 21, 2022
@yomboprime
Copy link
Collaborator Author

yomboprime commented Jun 21, 2022 •
edited
Loading

Thanks!

I will make another PR with the documentation changes.

@yomboprime
Copy link
Collaborator Author

Oh BTW I forgot to remove the cloning of the edge materials too.

@yomboprime
Copy link
Collaborator Author

@mrdoob this pr is ready to merge.

@mrdoob mrdoob merged commit 5c6990e into mrdoob:dev Jun 28, 2022
@mrdoob
Copy link
Owner

mrdoob commented Jun 28, 2022

Thanks!

abernier pushed a commit to abernier/three.js that referenced this pull request Sep 16, 2022
@yomboprime @abernier
LDrawLoader: 2 bugs and 7 improvements (mrdoob#24257)
947092f 
* LDrawLoader: 2 bugs and 7 improvements

* Fix typo

* Don't clone missing pink material. LUMINANCE token single quotes

* Small variable optimization

* Make missing material generic

* Make missing material generic, fix

* Add missing line which sets userData.fileName

* Fix returned edge material in getMainEdgeMaterial()

* Just added a small check to getMainEdgeMaterial()
snagy pushed a commit to snagy/three.js-1 that referenced this pull request Sep 21, 2022
@yomboprime @snagy
LDrawLoader: 2 bugs and 7 improvements (mrdoob#24257)
0307825 
* LDrawLoader: 2 bugs and 7 improvements

* Fix typo

* Don't clone missing pink material. LUMINANCE token single quotes

* Small variable optimization

* Make missing material generic

* Make missing material generic, fix

* Add missing line which sets userData.fileName

* Fix returned edge material in getMainEdgeMaterial()

* Just added a small check to getMainEdgeMaterial()
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@LeviPesin LeviPesin LeviPesin left review comments

@gkjohnson gkjohnson gkjohnson approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
r142
Development

Successfully merging this pull request may close these issues.
4 participants
@yomboprime @gkjohnson @mrdoob @LeviPesin