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

Free typed arrays for instanced models #10731

Merged
merged 6 commits into from
Aug 30, 2022
Merged

Conversation

j9liu
Copy link
Contributor

@j9liu j9liu commented Aug 26, 2022

Closes #10421. This addresses the temporary buffers created by InstancingPipelineStage. The attributes are loaded as typed arrays for reasons like computing transform matrices, or computing the min/max of the translation attribute if it's not provided. But afterwards, there is no further manipulation of the data, and thus no reason to keep recreating the buffers from those typed arrays.

This PR rewrites parts of I3dmLoader and GltfLoader to load in transform attributes as typed arrays only if the rotation attribute is defined (i.e. matrices have to be computed). Additionally, feature IDs were for some reason being loaded as typed arrays, so they have been restricted to loading as buffers instead.

TODO: need to profile the changes for i3dms in combination with other functions (those that require rebuilding draw commands)

@cesium-concierge
Copy link

Thanks for the pull request @j9liu!

  • ✔️ Signed CLA found.
  • CHANGES.md was not updated.
    • If this change updates the public API in any way, please add a bullet point to CHANGES.md.

Reviewers, don't forget to make sure that:

  • Cesium Viewer works.
  • Works in 2D/CV.

@j9liu j9liu marked this pull request as draft August 26, 2022 20:59
@j9liu j9liu requested a review from ptrgags August 29, 2022 13:28
@j9liu
Copy link
Contributor Author

j9liu commented Aug 29, 2022

Did some performance testing. Here's the difference between heap screenshots, using this Sandcastle:

before changes (main) after changes
arraybuffer before changes arraybuffer after changes
57 array buffers 46 array buffers

For runtime performance, I used this Sandcastle, which toggles the lightColor of the tileset. This forces the models to rebuildDrawCommands, so hopefully the difference as a result of changing InstancingPipelineStage would be noticeable. I recorded for 11 seconds, toggling the button 10 times within that timeframe, and got these results:

before changes (main) after changes
image image

Overall, I believe this PR improved both memory usage and runtime performance. It's now ready for review @ptrgags

@j9liu j9liu marked this pull request as ready for review August 29, 2022 16:18
Copy link
Contributor

@ptrgags ptrgags left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@j9liu looks good, I can reproduce your profiling results. Just a couple comments

Source/Scene/Model/I3dmLoader.js Outdated Show resolved Hide resolved
Source/Scene/GltfLoader.js Outdated Show resolved Hide resolved
@j9liu
Copy link
Contributor Author

j9liu commented Aug 30, 2022

@ptrgags updated!

@ptrgags
Copy link
Contributor

ptrgags commented Aug 30, 2022

Updates look good, thanks @j9liu!

@ptrgags ptrgags merged commit 4237eef into main Aug 30, 2022
@ptrgags ptrgags deleted the free-instanced-typed-arrays branch August 30, 2022 14:09
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.

Clean up instanced typedArray usage in ModelExperimental
3 participants