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

Tiles: Memory adjusted screen space error #2851

Merged
merged 4 commits into from
Jan 11, 2024
Merged

Conversation

Avnerus
Copy link
Collaborator

@Avnerus Avnerus commented Jan 2, 2024

Hi,
Opening this PR for feedback, even though this is still a work in progress.
Based on the CesiumJS implementation, I added basic functionality for memory adjusted SSE. The screen space error is adjusted based on the specified maximum GPU memory boundary. I think this feature is crucial for getting tilesets to work on mobile, especially iPhone, because Safari crashes immediately when too much GPU memory is allocated.

I made the feature optional with the memoryAdjustedScreenSpaceError flag (though in CesiumJS it's always on).
Some caveats:

  1. The estimation of GPU memory intake for glTFs is not accurate. It should estimate the total memory sum of the geometry, textures, and cesium tables and not just the total bytes of the payload. Hints here and here for implementing this later. Still, even with the current estimate, the implementation is already very useful in restricting the memory usage.
  2. The SSE adjustment works by creating "headroom" for memory overflow before increasing the SSE. For example, the maximum memory is 32MB with 1MB overflow. The SSE will increase when the memory usage goes over 33MB, and decrease when it goes back to 32MB after more tiles are unloaded. This is to prevent a ping pong situation where the SSE indefinitely increases and decreases. This overflow amount should be adjustable because it depends on tile sizes, so I added the memoryCacheOverflow option. For example, I noticed that Cesium uses a 1MB overflow for Google 3D Tiles, and it works well here too. But when I tried it on a point cloud it didn't work well. I noticed there is also an open issue in Cesium about this behavior.

I also added the screen space error value to the statistics, since it's very useful to see how it's adjusted.
Finally, note that I removed the maximumScreenSpaceError option from tileset-traverser and instead I use the value from the tileset. I didn't find any use for that option, but might be I missed something from I3S or other.

Thanks and let me know your comments!

Copy link
Collaborator

@ibgreen ibgreen left a comment

Choose a reason for hiding this comment

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

Thanks for putting up this PR. I suppose we should also update docs and add at least one test.

I am starting to see a pattern where contributors copy in feature X or feature Y from CesiumJS.

  • Not sure how easy it would be to create, but for my review and planning purposes it would be helpful to have a list of all missing features so we can track against something, right now I am not sure how much of this to expect...

modules/tiles/src/tileset/tileset-3d.ts Show resolved Hide resolved
modules/tiles/src/tileset/tileset-3d.ts Show resolved Hide resolved
modules/tiles/src/tileset/tileset-3d.ts Show resolved Hide resolved
modules/tiles/src/tileset/tileset-3d.ts Outdated Show resolved Hide resolved
modules/tiles/src/tileset/tileset-3d.ts Outdated Show resolved Hide resolved
modules/tiles/src/tileset/tileset-3d.ts Outdated Show resolved Hide resolved
   getter.
 - Remove the `memory exceeded` flag (no use of it was detected).
 - Combine screen space adjustment function to one
   `adjustScreenSpaceError()` function.
@Avnerus
Copy link
Collaborator Author

Avnerus commented Jan 9, 2024

Thanks for the review! Pushed an update and posted some comments.

modules/tiles/src/tileset/tileset-3d.ts Show resolved Hide resolved
modules/tiles/src/tileset/tileset-3d.ts Show resolved Hide resolved
@Avnerus
Copy link
Collaborator Author

Avnerus commented Jan 11, 2024

@ibgreen, I think now the only open question for this PR is the matter here. Could you comment on what you think is the best course of action?

Copy link
Collaborator

@ibgreen ibgreen left a comment

Choose a reason for hiding this comment

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

This is OK to merge. That said:

  • Shouldn't we add something to the docs?
  • Worth mentioning this as a bullet in the v4.1 release notes (docs/whats-new.md)?
  • Also, showing the current screenspaceerror in the stats object should be a minimal change and would be nice so we can track how it changes.

@Avnerus
Copy link
Collaborator Author

Avnerus commented Jan 11, 2024

Thanks!

Also, showing the current screenspaceerror in the stats object should be a minimal change and would be nice so we can track how it changes.

That one is already in the PR (MAXIMUM_SSE stat).

Regarding a docs update. I could run through that when I make the PR for the typescript cleanup.

@ibgreen ibgreen merged commit 2243c12 into master Jan 11, 2024
4 checks passed
@ibgreen ibgreen deleted the tiles-adjusted-sse branch January 11, 2024 17:28
@Avnerus
Copy link
Collaborator Author

Avnerus commented Jan 23, 2024

@ibgreen: two comments about this PR.

  1. About the estimation of GPU memory: I realized that this should be implemented on the glTF/model level. Since I am not using loaders.gl's GLTFLoader, but the one provided with three.js, see here and here how I implemented the counts.
    Is there a maintainer for GLTFLoader than can implement those? One only needs to put the sum into tile.content.gpuMemoryUsageInBytes and loader.gl's tile loader will pick it up. As expected, the actual memory counts are much higher than the current estimates (about 10 times).

  2. Is there going to be a new alpha soon? I was hoping to have this feature available in three-loader-3dtiles.

Thanks!

@ibgreen
Copy link
Collaborator

ibgreen commented Jan 23, 2024

About the estimation of GPU memory: I realized that this should be implemented on the glTF/model level.

  • We don't do that at the moment (However, we do track GPU memory allocation in luma.gl, but that mainly helps us track how much memory grows when we load).
  • Adding memory calculations to the GLTFLoader would indeed be a nice feature. I am the primary maintainer but I don't have time, but I'd be happy to review a PR.
  • Even if the calculations are incomplete, having something may be better than nothing, and we can improve later.

Is there going to be a new alpha soon?

  • We are planning to release 4.1 before the end of the month. Perhaps @dsavinov-actionengine and team could cut an alpha before that?

@dsavinov-actionengine
Copy link
Collaborator

@ibgreen @Avnerus
Got an alpha released today:

v4.1.0-alpha.10

Your PR is in there @Avnerus 👍

@Avnerus
Copy link
Collaborator Author

Avnerus commented Jan 24, 2024

Awesome! Thank you @dsavinov-actionengine .

@ibgreen ,

Adding memory calculations to the GLTFLoader would indeed be a nice feature. I am the primary maintainer but I don't have time, but I'd be happy to review a PR.

I won't be able to work on that for now, but if you'd like I could open an issue, referencing my three.js implementation.

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.

3 participants