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 example: Add option to merge geometries after loading #23173

Merged
merged 18 commits into from
Jan 13, 2022
Merged

LDrawLoader example: Add option to merge geometries after loading #23173

merged 18 commits into from
Jan 13, 2022

Conversation

yomboprime
Copy link
Collaborator

@yomboprime yomboprime commented Jan 7, 2022

Related comment: #23168 (comment)

Description

In the LDrawLoader example, add a gui option (and function) to merge model geometries by material after model loading. The option is named 'Merge model' and gives better render performance. This option disables the 'Construction steps' and the 'Show conditional lines' options.

The results are more noticeable in Firefox, where frames are almost not dropped where they dropped before even on small models. The model process time is somewhat 1/5 of loading time.

The function mergeObject() is 180 lines out of 560 of the example and it is fairly complex, so I'd rather move it to the LDrawLoader. But this can be done at any time.

@yomboprime yomboprime requested a review from gkjohnson January 7, 2022 21:20
@yomboprime
Copy link
Collaborator Author

Umm I think I had my git repo a bit dirty. Only the last commit of those 11 is relevant for this PR.

@gkjohnson Is it Ok or should I clean my repo and make another PR?

@yomboprime
Copy link
Collaborator Author

I've enabled again the conditional lines hide option because I've seen it could be done easily.

Copy link
Collaborator

@gkjohnson gkjohnson left a comment

Choose a reason for hiding this comment

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

Awesome! This is a nice utility function and keeps the LDrawLoader more clean.

In the previous merge objects code path the merged geometry resulted in one mesh with different groups and a material array (in addition to the two line segment objects) which simplifies the final object. It might be nice but I don't necessarily have strong opinions on it!

Live demo:

https://raw.githack.com/yomboprime/three.js/ldraw_merge_model/examples/webgl_loader_ldraw.html

examples/webgl_loader_ldraw.html Outdated Show resolved Hide resolved
@gkjohnson
Copy link
Collaborator

@gkjohnson Is it Ok or should I clean my repo and make another PR?

This is more a question for @mrdoob. The PR is easy enough to look at on its own for me and the commits will be squashed on merge.

@yomboprime
Copy link
Collaborator Author

@mrdoob The PR is ready to merge if those old commits are ok to be squashed.

@mrdoob mrdoob added this to the r137 milestone Jan 10, 2022
@yomboprime
Copy link
Collaborator Author

Done, ready to merge.

examples/jsm/loaders/LDrawLoader.js Outdated Show resolved Hide resolved
@yomboprime
Copy link
Collaborator Author

I made the LDrawUtils.js and the doc file for LDrawLoader.

You can see the new doc page here:
https://raw.githack.com/yomboprime/three.js/ldraw_merge_model/docs/index.html#examples/en/loaders/LDrawLoader

@mrdoob mrdoob merged commit c9daf11 into mrdoob:dev Jan 13, 2022
@mrdoob
Copy link
Owner

mrdoob commented Jan 13, 2022

Thanks!

@yomboprime yomboprime deleted the ldraw_merge_model branch January 14, 2022 00:39
@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