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: colour -> color #23168

Merged
merged 1 commit into from
Jan 7, 2022
Merged

Conversation

gkjohnson
Copy link
Collaborator

Related issue: #23157 (comment)

Description

Convert all variable names and comments with the spelling "colour" to "color" in LDrawLoader.

Also it occurs to me that merging all geometry within a single loaded LDraw hierarchy using BufferGeometryUtils.mergeBufferGeometries is not so straight forward. In order to get an optimal model you need to make sure all faces that share a material are sorted next to each other and grouped. And some of the LDraw meshes will have multiple materials. It would need to be a mesh and material-aware geometry merge which is more complicated than I initially anticiptated so we'll have to revisit that another time.

https://raw.githack.com/gkjohnson/three.js/ldraw-colour-color/examples/webgl_loader_ldraw.html

@gkjohnson gkjohnson requested a review from yomboprime January 7, 2022 03:06
@yomboprime
Copy link
Collaborator

It would need to be a mesh and material-aware geometry merge

Perhaps it would be easier to do it in two steps: First a separation by materials (which should be easy to do), and then merging all geometries of the N materials in N calls to BufferGeometryUtils.mergeBufferGeometries. I don't know if it will be too slow but I think I'll try it.

@mrdoob mrdoob added this to the r137 milestone Jan 7, 2022
@mrdoob mrdoob merged commit 6b0352a into mrdoob:dev Jan 7, 2022
@mrdoob
Copy link
Owner

mrdoob commented Jan 7, 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