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

Triangle: Add setFromAttributeAndIndices(). #22404

Merged
merged 2 commits into from
Sep 2, 2021
Merged

Conversation

Mugen87
Copy link
Collaborator

@Mugen87 Mugen87 commented Aug 24, 2021

Related issue: Fixed #22337.

Description

Added a buffer attribute version of setFromPointsAndIndices().

@WestLangley
Copy link
Collaborator

A similar method was added 9 years ago without demand. I am not sure it is used.

@makc ? What is the use case?

@makc
Copy link
Contributor

makc commented Aug 26, 2021

@WestLangley like I said basically to get a Triangle out of clicked face, but it makes more sense to replace the method rather than add a new one

@makc
Copy link
Contributor

makc commented Aug 26, 2021

btw, in my case, I came to Triangle docs page when looping over vertices looking for a one-liner to calculate face area. I did not think there was a method like setFromPointsAndIndices, but there was - unfortunately, it was outdated, too

@Mugen87
Copy link
Collaborator Author

Mugen87 commented Aug 27, 2021

A similar method was added 9 years ago without demand. I am not sure it is used.

Many math methods were introduced with the initial commits without a concrete usage in the library. The methods represented a basic and reasonable API of math classes. However, just because the repo does not use a method right now it does not necessarily mean we should remove it. At least it should not be a rule of thumb. Especially since most of the math methods are compact and do not noticeably impact the complexity and size of the core.

Hence, I vote to add setFromAttributeAndIndices() and also keep setFromPointsAndIndices().

@WestLangley
Copy link
Collaborator

However, just because the repo does not use a method right now it does not necessarily mean we should remove it.

@Mugen87 I think you were the only one who ever suggested we remove a method for that reason. ;-)

@mrdoob
Copy link
Owner

mrdoob commented Sep 2, 2021

Hmm, I guess it's not much code anyway...

@mrdoob mrdoob added this to the r133 milestone Sep 2, 2021
@mrdoob mrdoob merged commit d957011 into mrdoob:dev Sep 2, 2021
@mrdoob
Copy link
Owner

mrdoob commented Sep 2, 2021

Thanks!

@joshuaellis joshuaellis mentioned this pull request Sep 18, 2021
23 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.

Triangle.setFromPointsAndIndices buffer attribute version?
4 participants