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: Cache geometry versions of parts, reduce geometry redundancy, improve parse time #23232

Merged
merged 39 commits into from
Jan 18, 2022

Conversation

gkjohnson
Copy link
Collaborator

@gkjohnson gkjohnson commented Jan 14, 2022

Related issue: #23157 (comment)

Description

This one took a bit to get my head around because of the scoping, deferred cloning of geometry, and asynchronous processing but this is the last big LDrawLoader PR I'm planning to do (for awhile, at least). It reworks LDrawLoader to move all the subobject processing and geometry generation to a separate cache class called "LDrawPartsGeometryCache". The .loadModel function will return a promise that resolves with a built group and (if relevant) unfinished material array so pass through color codes or out-of-scope material cods can be applied later. The missing materials are saved in the materials array as string color codes which are replaced when they're actually being added to a model. Those pre-built parts are cached so any redundant processing can be saved.

Here are some of the stats for the AT-AT model. You can see we have a significant improvement in the number of unique geometries generated and a parse time improvement of around 35-45% depending on whether normal smoothing is enabled or not:

AT-AT Stats

BEFORE AFTER
Perf w/ smoothing ~14500ms ~9300ms
Perf w/o smoothing ~5700ms ~3100ms
Cache Hits N/A 902
Unique Geometries 3224 472
Unique Materials 23 23
Triangle Count 1030054 1030054
Mesh Count 1077 1077
Steps 879 879

Live Demo

https://raw.githack.com/gkjohnson/three.js/cached-parts/examples/webgl_loader_ldraw.html

Here's how I tested:

  • Tested the existing bundled LDraw models to ensure they load
  • Tested with the Pharaoh themed models I've shown in other PRs to ensure parts fetching works
  • Loaded an individual part with no colors to ensure that works
  • Chose a few steps from some of the bundled LDraw models and looked at a few of the build steps to make sure they looked consistent.

I'll keep these in draft until #23231 is merged but feel free to give feedback or ask questions in the mean time. I know it's a big one to dig in to.

@gkjohnson gkjohnson requested a review from yomboprime January 14, 2022 07:28
@yomboprime
Copy link
Collaborator

I'll keep these in draft until #23231 is merged but feel free to give feedback or ask questions in the mean time. I know it's a big one to dig in to.

I'm sorry I didn't see this PR and made the other conflicting PR.
I'll review this the best as I can tomorrow.

For the moment I see great performance improvements!

@mrdoob mrdoob added this to the r137 milestone Jan 14, 2022
@gkjohnson
Copy link
Collaborator Author

I'm sorry I didn't see this PR and made the other conflicting PR.

Heh, I hadn't made it! The conflicts are easy to resolve I just wanted to get ahead of any more. I just added the changes from your previous PR back in.

I see that merge the model, has somehow broken, though. It's going to be hard to spin this as a feature 😄 I'll fix that, as well

image

@gkjohnson
Copy link
Collaborator Author

I see that merge the model, has somehow broken, though.

Fixed! Because the geometry was now shared the world matrices were being applied multiple times to the same geometry instance.

@gkjohnson gkjohnson marked this pull request as ready for review January 14, 2022 18:33
examples/jsm/loaders/LDrawLoader.js Outdated Show resolved Hide resolved
examples/jsm/loaders/LDrawLoader.js Outdated Show resolved Hide resolved
examples/jsm/utils/LDrawUtils.js Show resolved Hide resolved
@yomboprime
Copy link
Collaborator

It seems the X-Wing model does not load, it gives error. If I add err parameter to onError() in the example (which should be added), it gives:
Error loading model: Error: LDrawLoader: Material properties for code 0x2CFD7DD not available.

It seems the solution is to support direct colors (0x2CFD7DD) in the function getMaterial( c, colorCode ) internal loader function.

@gkjohnson
Copy link
Collaborator Author

It seems the solution is to support direct colors (0x2CFD7DD) in the function getMaterial( c, colorCode ) internal loader function.

Thanks missed that somehow! Just pushed a fix along with an extra log in the ldraw example to output the error. And with the "direct" materials only being evaluated on the last material pass for geometry now we actually generate fewer materials (And therefore fewer draw calls) than we did before because previously a new material was being produced for every primitive. It could still be improved to afford caching of materials across multiple meshes but another time.

@yomboprime
Copy link
Collaborator

I think that's all. Congrats for the refactor, this one was big :-)

I couldn't resist to make an animation with physics :-)
https://yomboprime.github.io/LDrawAnimation/examples/ldraw_animation.html

@yomboprime
Copy link
Collaborator

Oh, one more thing :-(

About half of the models now don't work when the option 'merge model' is set. This is because apparently there are meshes being generated with groups with zero count and materialIndex = -1.

group0

@gkjohnson
Copy link
Collaborator Author

About half of the models now don't work when the option 'merge model' is set. This is because apparently there are meshes being generated with groups with zero count and materialIndex = -1.

Small change I forgot to commit -- just pushed it. It's because current material was initialized to "undefined" rather than "null" which it was checking for so an empty group was always added first. Should be fixed now. And looks like the Infinity counts were there previously. Thanks again for checking everything!

@yomboprime
Copy link
Collaborator

yomboprime commented Jan 17, 2022

And looks like the Infinity counts were there previously.

Yes, I think the last geometry group is always open-ended.

@mrdoob mrdoob requested a review from yomboprime January 18, 2022 17:20
Copy link
Collaborator

@yomboprime yomboprime left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the PR is ready to merge, we have not observed any more errors.

@mrdoob mrdoob merged commit ca021a2 into mrdoob:dev Jan 18, 2022
@mrdoob
Copy link
Owner

mrdoob commented Jan 18, 2022

Thanks!

@gkjohnson gkjohnson deleted the cached-parts branch January 18, 2022 22:45
@joshuaellis joshuaellis mentioned this pull request Jan 19, 2022
20 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