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

LDrawLoader: Further improve smooth normal generation performance #22231

Merged
merged 15 commits into from
Aug 2, 2021

Conversation

gkjohnson
Copy link
Collaborator

Merge #22228 first

Description

  • Adjust the bookkeeping of faces to retain whether the original face was a quad or triangle and change normal smoothing and geometry generation accordingly.
  • Rename scope.triangles array to scope.faces

With this change the total load time for the AT-ST model comes down from ~21 seconds to ~15 seconds compared to #22228.

There's a normal smoothing issue I found (not new) that I'll fix in another PR and I'll add preloadMaterials and setPartsLibraryPath functions so the loader is easier to use with the full LDraw parts library.

cc @yomboprime

@yomboprime
Copy link
Collaborator

yomboprime commented Aug 1, 2021

...and I'll add preloadMaterials and setPartsLibraryPath functions so the loader is easier to use with the full LDraw parts library.

Is it not sufficient with the current setPath()? It does exactly that (establishes the path to the library)

@gkjohnson
Copy link
Collaborator Author

gkjohnson commented Aug 1, 2021

Is it not sufficient with the current setPath()? It does exactly that (establishes the path to the library)

It does but it seems like a common case that the model you want to load would be saved in a different location from from the the canonical parts library. For example you currently can't load a model if your parts are stored at /path/to/parts/library/ and models at /path/to/models/ even if you pass the absolute path in for the model URL (which is maybe something else that should be fixed across loaders, too?) so I'm proposing the following:

new LDrawLoader()
  .setPath( '/path/to/models/' )
  .setPartsLibraryPath( '/path/to/parts/library/' )
  .load( 'my-model.mpd', result => {} )

@yomboprime
Copy link
Collaborator

yomboprime commented Aug 1, 2021

Okay, I see it fine.

@mrdoob mrdoob added this to the r132 milestone Aug 2, 2021
@mrdoob mrdoob merged commit 721bf74 into mrdoob:dev Aug 2, 2021
@mrdoob
Copy link
Owner

mrdoob commented Aug 2, 2021

Thanks!

@mrdoob
Copy link
Owner

mrdoob commented Aug 2, 2021

Merge #22228 first

Ops! Missed that. Lets see if I can fix this...

@mrdoob
Copy link
Owner

mrdoob commented Aug 2, 2021

Seems like I can't... I reverted this PR and merged #22228.
I tried cherry-picking this PR again but I'm getting conflicts. Sorry... 😅

@gkjohnson
Copy link
Collaborator Author

Heh sorry I should have marked as draft 😅

Here's a replacement PR: #22247

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