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: Refactor, cache pre-parsed file contents #23157

Merged
merged 11 commits into from
Jan 6, 2022

Conversation

gkjohnson
Copy link
Collaborator

@gkjohnson gkjohnson commented Jan 6, 2022

Related issue: --

Description

This PR looks fairly big but it's mostly rearranging code. The significant change here is that LDraw files are now parsed without context so they parsed contents are no longer specific to a scope being processed in the moment. Instead the context-specific parameters (face inversion, materials, scale) are applied after the fact when the file contents are being applied to a part. This lets us cache the parsed contents of the file and clone the result rather than reparse the text file contents every time. The big adjustment is that the LDrawFileCache has been changed to the LDrawParsedCache and the logic from objectParse has been moved into the cache.

This included a bit of a speed boost (around 12% or 700ms on the AT-ST model when loading without smooth normals) but the bigger benefit is that this is a big step towards producing generalize geometry lists for full parts and cache smooth normals so they only have to be generated once. And from there it should be easy to produce only a single piece geometry per part to share among all meshes that use it. That should enable a much larger speed boost when loading a model especially with smooth normals.

I tested this with the Scorpion Pyramid model to check part library models as well as all the packed models in the repo.

https://raw.githack.com/gkjohnson/three.js/cache-parsed-data/examples/webgl_loader_ldraw.html

Sorry for the big change list @yomboprime 😅. I think it's only one more big one after this to make a cache for the reusable, smoothed geometry.

@gkjohnson gkjohnson requested a review from yomboprime January 6, 2022 02:34
@mrdoob mrdoob added this to the r137 milestone Jan 6, 2022
@yomboprime
Copy link
Collaborator

yomboprime commented Jan 6, 2022

Looks great! It is an awesome refactor.

It seems Firefox performance is degraded in the big models (AT-ST, X-Wing) but this happened already when separateObjects = true. Chrom* eats it with no problems.

A small thing I've noted while reading is that the words "colour" and "color" are used indistinctively throughout the code. Perhaps we should stay with one (If I'm correct, the latter is used in the U.S.)

@gkjohnson
Copy link
Collaborator Author

I seems Firefox performance is degraded in the big models (AT-ST, X-Wing) but this happened already when separateObjects = true

Oh interesting -- good to know. I'm inclined to say that the models should be merged after the fact if necessary for this case or we can add the "separate objects" flag in again later.

A small thing I've noted while reading is that the words "colour" and "color" are used indistinctively throughout the code. Perhaps we should stay with one (If I'm correct, the latter is used in the U.S.)

I've tried to use "colour" in all of my new additions to remain consistent with the rest of the variables in the file. Though there are a couple places where I see "color" used that were moved around in this PR. The "LDraw" files use the spelling "colour" internally but either way I don't have a preference. I happy with a change to a consistent spelling in another PR if that's preferred. Do you have a preference? "Color" would be consistent with three.js' naming convention.

@yomboprime
Copy link
Collaborator

yomboprime commented Jan 6, 2022

the models should be merged after the fact if necessary for this case or we can add the "separate objects" flag in again later.

Yes, later we could introduce a "merge objects" option (which disables construction steps visualization)

Do you have a preference? "Color" would be consistent with three.js' naming convention.

Then we should choose "color". Except in the literal !COLOUR when parsing, obviously 😊

@yomboprime
Copy link
Collaborator

yomboprime commented Jan 6, 2022

Yes, later we could introduce a "merge objects" option (which disables construction steps visualization)

I should add that perhaps this can be done just in the example without touching the loader, with a simple call to BufferGeometryUtils. I've not thought of it in depth, though.

@gkjohnson
Copy link
Collaborator Author

@mrdoob to be clear I think this can be merged as is and I can make some of the other updates in upcoming PRs to avoid adding more changes to this one. Some of the upcoming things to do, then:

  • Change "colour" variable names to "color"
  • Add a "mergeObjects" flag to manually merge objects in the example after load
  • Add "PartsGeometryCache" to cache pre processed meshes and void redundant normal smoothing, geometry creation
  • Fix a small issue related to normals

@mrdoob mrdoob merged commit bd23028 into mrdoob:dev Jan 6, 2022
@mrdoob
Copy link
Owner

mrdoob commented Jan 6, 2022

Thanks!

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