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: Check if a face edge is a sub segment of a long edge when smoothing normals #23077

Merged
merged 5 commits into from
Jan 4, 2022

Conversation

gkjohnson
Copy link
Collaborator

@gkjohnson gkjohnson commented Dec 23, 2021

Related issue: #22957 (comment)

Description

This PR implements the approach mentioned in #22957 (comment) to create hard edges in the corner cases when a part is made up of multiple materials and edges stretch over multiple face edges that are broken up by color. It works by indexing every edge line as a normalized ray and then checking to see if face edges lie on that ray. This is more expensive but it is only run when if a part is made of 2 or more materials.

Before After
image image

In the Scorpion Pyramid model there are 55 parts total that are made of 2 or more materials and are therefore smoothed with this more advanced logic. Here's an image of those parts:

image

From doing some performance checks it doesn't look like this significantly impacts the performance of smoothing normals on the pieces where it's relevant.

A small side note that that red seat piece in the bottom left that looks like it is all one color is incorrectly formed and actually has one triangle that is a different material causing it to fall into the above condition. The most segments that have to be checked against once a face edge has been found to lie on an edge ray is 9 in the Scorpion Pyramid model.

cc @yomboprime

@yomboprime
Copy link
Collaborator

Amazing work. I've read the code, but honestly I can't make a deep review or tests at the moment.

@gkjohnson gkjohnson marked this pull request as ready for review December 23, 2021 16:43
@mrdoob mrdoob added this to the r137 milestone Dec 23, 2021
@mrdoob
Copy link
Owner

mrdoob commented Jan 4, 2022

Lets merge this for now.

@mrdoob mrdoob merged commit 91e4d29 into mrdoob:dev Jan 4, 2022
@mrdoob
Copy link
Owner

mrdoob commented Jan 4, 2022

Thanks!

@gkjohnson gkjohnson deleted the ldraw-subsegment-edges branch January 4, 2022 20:11
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