Closed kring closed 5 months ago
I'm using TextureSettingsTest to test support for multiple textures with different sampler settings referencing a single image. I'm using this tileset.json to display it:
{
"asset": { "version": "1.0" },
"root": {
"boundingVolume": {
"sphere": [
0,0,0,10
]
},
"children": [],
"content": { "uri": "TextureSettingsTest.glb" },
"geometricError": 0.0,
"refine": "REPLACE"
}
}
This is as close as we get to working, because double-sided materials are not supported in Cesium for Unreal:
It's doing a GPU copy of the texture data. There are edge cases, though. What if the first texture didn't have mipmaps, but a later one does? Ugh.
@j9liu , if you have the time, can you help with a sanity check / test on anything that changed related to metadata?
I've already reviewed the dependent cesium-native changes, tested the unreal samples, and am looking at rest of the code the now.
@kring any thoughts on how to test this properly?
I see all the unit tests passed, which is good, but I'm not seeing any unit tests that target texture loading / texture memory specifically.
I've also run through all the Unreal samples, in Windows, but don't have a non-windows device to check against.
As for code review, looks like a lot of "overhaul" type code, like you stated. Can definitely look for obvious issues, but there's a chance that anything subtle and non-obvious can slip through here.
@kring One more thing, do you have any numbers of showing how much texture memory was saved?
Ex. In cesium-unreal-samples, map 02_CesiumMelbourne, I saw memory usage go from X to X, a X% reduction.
I see all the unit tests passed, which is good, but I'm not seeing any unit tests that target texture loading / texture memory specifically.
It'll be extremely difficult to write tests for. We'd have somehow confirm that the GPU texture has been populated correctly. I'm open to ideas, but it doesn't seem worth the effort when it's very obvious very quickly just by looking if textures aren't being created successfully.
One thing to watch for is memory leaks. I've put a lot of effort into convincing myself there are none, but that's probably the biggest risk. Well, that and the risk that I've broken something related to metadata textures, because breaking those wouldn't be so obvious at a glance.
One more thing, do you have any numbers of showing how much texture memory was saved?
It eliminates a complete copy of the texture in CPU memory. After this PR, there should be none left (as far as I know!), so the CPU texture memory usage goes from X to 0, for an infinite improvement! š
Counting CPU and GPU memory together, this will cut the texture memory usage in half. This should be true universally across all tilesets and raster overlays.
Well I looked into it, and it's actually not too difficult to read back a texture in Unreal. So I added some tests. I also cleaned up some TODOs. Sadly this PR no longer removes more code than it adds, but it's probably worth it. š
I unfortunately had to mark the new tests NonNullRHI
, meaning they effectively won't run on CI, because the null RHI doesn't allow to create a texture and then read back its pixels, so all the new tests fail. It's also not possible to run with a non-null RHI on the non-D3D11-capable GH Actions runners.
I ran into a crash earlier that I can't reproduce š¤ It was because my 06_CesiumMetadata had been changed for #1360, and when I opened it, it crashed some class that was "SharedThread____" ? But after I git restore
'd the level, I didn't see it again, and I have no idea what I could have done for my PR to induce the crash!
Unfortunately, I took a screenshot and pasted it here to post later, but I accidentally navigated to a different tab and lost the image. And now I can't reproduce it. š I'm documenting this anyway in case I run into it again.
@kring back to you. There's a few comments left here, and also some comments in cesium-native 806.
Appreciate the new texture unit tests, thanks. I think one of your big concerns was memory leaks, and indeed that is a general concern for the plugin right now, so I wrote a new issue to create memory leak tests.
I finished code review and didn't find anything major. It's a lot of code that's changed though, so it's very easy for subtle bugs to slip through. Hopefully unit testing will help. I'll also manual test a bit more through next week.
Thanks for the reviews @csciguy8 and @j9liu. I believe I've addressed all your comments.
Thanks @kring !
This depends on CesiumGS/cesium-native#804 and CesiumGS/cesium-native#806 so merge them first.
After we create an Unreal texture from a glTF
ImageCesium
, we "mostly" don't need thepixelData
anymore. Clearing it saves a lot of memory - an entire copy of the raw, uncompressed pixel data. This is an improvement comparable to #1329 again, and it applies even to Windows (unlike that PR).So that's what this PR set out to do, but it kind of got away from me and ended up being a pretty substantial overall of how textures get created in Unreal. There are a bunch of benefits:
texture
instances reference a singleimage
, we now only create one copy of the texture data on the GPU. Previously we'd upload the image multiple times. This isn't super common, but it's a big win when it happens. Probably the most notable place it happens is when creating upsampled tiles for raster overlays. Previously the upsampled children would each get a full copy of the parent's textures. Now, they all share the parent's textures.Some other notes:
TEXTUREGROUP_8BitData
instead ofTEXTUREGROUP_World
. This is a little bit arbitrary, but I think it's an improvement.pixelData
was used to determine the size of texture data for caching purposes. Now that's pretty much always going to be zero, so the newImageCesium::sizeBytes
field (see CesiumGS/cesium-native#804) is used to track the size instead.