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

SceneUtils: Add reduceVertices(). #22742

Merged
merged 4 commits into from
Oct 24, 2022
Merged

SceneUtils: Add reduceVertices(). #22742

merged 4 commits into from
Oct 24, 2022

Conversation

elalish
Copy link
Contributor

@elalish elalish commented Oct 27, 2021

I wrote this some time ago to support model-viewer's automatic camera framing (which is tight without allowing the object to leave the frame for any orbit parameters with constant zoom). I always thought it might be a useful generic method for three.js, so I figured I'd contribute it and see if anyone agrees it might be useful to others.

@marcofugaro
Copy link
Contributor

Oh so it's like the Array.prototype.reduce() function but for vertices.

Maybe it's easier for users if the signature is the same as Array.reduce(), meaning first argument is the function and second argument the initial value?
So the second argument can be defaulted to vertex if omitted?

@elalish
Copy link
Contributor Author

elalish commented Oct 28, 2021

Sort of, but the difference is that it does not have to reduce to a Vertex3 type. For instance, you could reduce to a field of view for a given radius or something of that nature. Since the type is unknown, you have to supply an initial value, because there is no default constructor.

@Mugen87
Copy link
Collaborator

Mugen87 commented Nov 4, 2021

reduceVertices() works with the geometry which is no property of Object3D. That makes me feel that Object3D is not the right place for this method.

For the time being I would suggest to add the method to SceneUtils.

@elalish
Copy link
Contributor Author

elalish commented Mar 14, 2022

Sorry for the delay, I've addressed the comments now. I was reminded of this while fixing this function in MV for skinned meshes: google/model-viewer#3272

I was hoping to fix three's bounding box computation, but since it's inside of BufferGeometry it can't access the bones. This function can be a work-around in the meanwhile, but it seems like there should be a more core way of handling this.

@Mugen87
Copy link
Collaborator

Mugen87 commented Mar 15, 2022

I was hoping to fix three's bounding box computation, but since it's inside of BufferGeometry it can't access the bones.

Related #21507.

We can consider to move the function into the core depending on how #21507 evolves.

@Mugen87 Mugen87 changed the title Object3D.reduceVertices SceneUtils: Add reduceVertices(). Mar 16, 2022
@elalish
Copy link
Contributor Author

elalish commented Apr 4, 2022

Okay, updated from traverse to traverseVisible to allow some control. Also ran build-examples to generate the js version, which conveniently told me about some includes I'd forgotten.

@elalish
Copy link
Contributor Author

elalish commented Apr 5, 2022

Actually, @donmccurdy would you mind taking a look at this? It seems to work quite well generally, but it still seems to fail your example: google/model-viewer#2450 (comment)

@elalish
Copy link
Contributor Author

elalish commented Apr 5, 2022

Interesting, so think I found the final remaining problem. The issue with the old CesiumMan model was that is used unorm8 for its skin weights vector. That's handled properly in the shaders, but in SkinnedMesh -> boneTransform it is not. I tried adding getNormalizedComponentScale( geometry.attributes.skinWeight.array.constructor ); to that function, but three does not like importing a function from an example. @Mugen87 @donmccurdy any thoughts on the right way to solve this? Maybe getNormalizedComponentScale should be a member of BufferAttribute?

@donmccurdy
Copy link
Collaborator

It does seem like we're going to need something like #22874 sooner or later, either directly on THREE.BufferAttribute or in a utility function.

@elalish
Copy link
Contributor Author

elalish commented Apr 6, 2022

Agreed; is there any problem with #22874? I'd like to merge that first so I can clean this up and test it.

@donmccurdy
Copy link
Collaborator

Just bumping this thread now that #22874 has been merged. :)

@elalish
Copy link
Contributor Author

elalish commented Oct 11, 2022

Okay, I think this one's ready now. Supposedly I should run npm run build-examples to update the js directory, but when I do, I get a pile of unrelated changes. What should I do?

@LeviPesin
Copy link
Contributor

Supposedly I should run npm run build-examples to update the js directory

You shouldn't, all building is done by mrdoob in separate commits.

@Mugen87 Mugen87 added this to the r146 milestone Oct 24, 2022
@Mugen87 Mugen87 merged commit 426c4ba into mrdoob:dev Oct 24, 2022
@elalish
Copy link
Contributor Author

elalish commented Oct 24, 2022

Thank you!

@elalish elalish deleted the reduce branch October 24, 2022 18:29
@joshuaellis joshuaellis mentioned this pull request Oct 31, 2022
19 tasks
@OndrejSpanel
Copy link
Contributor

Sort of, but the difference is that it does not have to reduce to a Vertex3 type. For instance, you could reduce to a field of view for a given radius or something of that nature. Since the type is unknown, you have to supply an initial value, because there is no default constructor.

Note: In functional languages such function is usually called fold, not reduce.

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.

6 participants