Skip to content

Conversation

@takahirox
Copy link
Collaborator

@takahirox takahirox commented Feb 28, 2019

I think scene.matrixAutoUpdate should be false in performance_static.html example. Otherwise, root scene local and world matrices are updated and then the world matrices of all the objects under the scene are unnecessarily updated every frame.

https://github.com/mrdoob/three.js/blob/r101/src/core/Object3D.js#L593-L634

Another option is scene.autoUpdate = false.

@takahirox takahirox changed the title Set scene.matrixAutoUpdate = true in performance_static example Set scene.matrixAutoUpdate = false in performance_static example Feb 28, 2019
@takahirox takahirox changed the title Set scene.matrixAutoUpdate = false in performance_static example Skip unnecessary matrices update in performance_static example Feb 28, 2019
@takahirox takahirox force-pushed the FixStaticPerformanceExample branch from 85d4bf5 to 253f84e Compare February 28, 2019 14:47
@mrdoob mrdoob added this to the r102 milestone Feb 28, 2019
@mrdoob mrdoob merged commit d736480 into mrdoob:dev Feb 28, 2019
@mrdoob
Copy link
Owner

mrdoob commented Feb 28, 2019

Thanks!

@takahirox takahirox deleted the FixStaticPerformanceExample branch February 28, 2019 17:50
@WestLangley
Copy link
Collaborator

Setting scene.matrixAutoUpdate = false is actually preventing the computation of the world matrices in this case.

I expect there are very few users that understand this subtly. I wonder if this will cause confusion... Users copy these examples.

@mrdoob
Copy link
Owner

mrdoob commented Feb 28, 2019

True that. We should probably revert this.

@takahirox
Copy link
Collaborator Author

takahirox commented Feb 28, 2019

Setting .matrixAutoUpdate = false for static objects in Three.js is good practice for better performance. It's also written in the documentation.

https://threejs.org/docs/#manual/en/introduction/Matrix-transformations

I think this example with .matrixAutoUpdate = false can be a good example for users to know the practice. So how about adding comment about what .matrixAutoUpdate = false is for in the code instead of reverting?

@mrdoob
Copy link
Owner

mrdoob commented Feb 28, 2019

I don't know... This specific example is more about performance testing.

A better practice would be to merge all these meshes, or to use instancing...

@WestLangley
Copy link
Collaborator

WestLangley commented Mar 1, 2019

I am quite surprised to see four three.js examples with

scene.matrixAutoUpdate = false;

I wonder if the authors of those examples understood what this did. Do you think other users should be copying this pattern?

@takahirox
Copy link
Collaborator Author

takahirox commented Mar 1, 2019

This specific example is more about performance testing.

Yes, this example is for static objects performance test. So I thought user expects to see the performance of an example optimized for static objects (maybe without instancing nor merging).

@takahirox
Copy link
Collaborator Author

takahirox commented Mar 1, 2019

Do you think other users should be copying this pattern?

Personally, I think yes. Because as we discussed #14138 (comment) it's difficult on our current API to automatically skip unnecessary matrices update so I think better to encourage setting .matrixAutoUpdate = false for static objects including scene to get better performance.

Do you think for users it's hard to understand and/or problematic to use?

@Mugen87
Copy link
Collaborator

Mugen87 commented Mar 1, 2019

From my point of view, the "average" user should not bother with this flag. Otherwise they will be surprised why certain transformation changes do not have any affect. I think the ultimate goal is to have some sort caching in the near future so this flag becomes more or less obsolete. Even if this means to change the API in context of Object3D.matrix.

Because of this, I agree with @WestLangley. We should consider to clean up the other four examples.

@takahirox
Copy link
Collaborator Author

takahirox commented Mar 1, 2019

Even if we end up removing scene.autoMatrixUpdate = false from the other examples, I still think we should have it in the performance_static example. I think mesh.matrixAutoUpdate = false; here

https://github.com/takahirox/three.js/blob/253f84e0010d04e1a4632e7b2e51a61b73d069b9/examples/webgl_performance_static.html#L68

is for skipping unnecessary matrices update and I was surprised to see unnecessary world matrices update function spends a lot of time when I got CPU profile. Probably this isn't intentional.

I think if we add comment like this, it isn't confusing to users. What do you think? (Feel free to improve and simplify the comment.)

// Set .matrixAutoUpdate = false here for skipping unnecessary matrices calculation for static object.
// Note that by setting this flag false .position/quaternion/scale change doesn't affect to transform
// without explicitly calling .updateMatrix(). You shouldn't set false for dynamic objects.
// See https://threejs.org/docs/#manual/en/introduction/Matrix-transformations
scene.matrixAutoUpdate = false;

BTW

to have some sort caching in the near future

I thought it may not be doable in short term because the API change can break a lot of core/example/user code so I suggested setting flag false. Yeah of course happy if we can do in the near future tho.

Update: Or the purpose may be clearer by adding scene.autoUpdate = false and removing all .matrixAutoUpdate = false in this example.

@mrdoob
Copy link
Owner

mrdoob commented Mar 15, 2019

Even if we end up removing scene.autoMatrixUpdate = false from the other examples, I still think we should have it in the performance_static example.

That sounds good to me.

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.

4 participants