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

Docs: Clarify InstancedMesh.setMatrixAt() needs update. #20620

Merged
merged 2 commits into from
Nov 6, 2020

Conversation

gonnavis
Copy link
Contributor

@gonnavis gonnavis commented Nov 4, 2020

Clarify "You have to set .instanceMatrix.needsUpdate flag to true after called .setMatrixAt()."
I have been troubled by this for a long time, even though I finally found it mentioned in the previous lines.

@Mugen87
Copy link
Collaborator

Mugen87 commented Nov 4, 2020

I wonder if we could hide the necessity to set the needsUpdate flag at all by introducing a private dirty flag that indicates that the buffer attribute requires an update. It would be set to true when using setMatrixAt(). The dirty flag could then be checked by the renderer per frame. If set to true, needsUpdate is set to true.

I think it's better use a separate dirty flag for this and not directly set needsUpdate in setMatrixAt(). Otherwise the internal buffer attribute version would be quickly reach high numbers.

@gonnavis
Copy link
Contributor Author

gonnavis commented Nov 5, 2020

I also think that call self method but then require to set another property's needsupdate is a bit strange.

@mrdoob
Copy link
Owner

mrdoob commented Nov 6, 2020

My goal with InstancedMesh was to provide a way to render many objects at the same time with minimal CPU (and memory) usage. I thought about adding a flag in setMatrixAt() but I worried that it would increase the CPU usage when updating the matrices of thousands of objects per frame.

@mrdoob mrdoob added this to the r123 milestone Nov 6, 2020
@mrdoob
Copy link
Owner

mrdoob commented Nov 6, 2020

Can you change the text to this?

Make sure you set [page:.instanceMatrix][page:BufferAttribute.needsUpdate .needsUpdate] to true after updating all the matrices.

@gonnavis
Copy link
Contributor Author

gonnavis commented Nov 6, 2020

@mrdoob Changed.

@mrdoob mrdoob merged commit 1f6e8d2 into mrdoob:dev Nov 6, 2020
@mrdoob
Copy link
Owner

mrdoob commented Nov 6, 2020

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.

None yet

3 participants