search

CesiumGS / cesium-unreal

Bringing the 3D geospatial ecosystem to Unreal Engine
https://cesium.com/platform/cesium-for-unreal/
Apache License 2.0
879 stars 285 forks source link

Fix `FName` memory issue when running for extended periods of time #1423

Closed csciguy8 closed 1 month ago

csciguy8 commented 1 month ago

Closes #1422.

A user reported an error with FName allocations when running for long periods of time (5 hours).

Interesting output from Unreal FName overflow, allocated 1024MB of string data. FName strings are never freed and should be created sparingly

In loadPrimitiveGameThreadPart we create a new FName object for every primitive loaded. In some cases, these can be fairly large (Ex. 165 characters) as they are essentially the url to the asset that was fetched.

This PR solves the problem by avoiding creating an FName entirely.

Ran through all the samples level and didn't see any issues. Also, these meshes and components are not visible in the Outliner window, so there's no apparent user facing change either.

csciguy8 commented 1 month ago

so there's no apparent user facing change either.

One use for named components is for debugging, specifically when wanting to know what assets are mapped to what you are actually looking at. (thanks @kring for mentioning)

Here's how... click on a piece of the terrain in the viewport, try to delete it. You'll see a console message like this...

Cmd: DELETE Cmd: ACTOR DELETE LogEditorActor: Cannot delete component CesiumGltfPrimitiveComponent /Game/CesiumSamples/Maps/01/01_CesiumWorld.01_CesiumWorld:PersistentLevel.CesiumWorldTerrain.CesiumGltfComponent_282.https://assets.ion.cesium.com/us-east-1/asset_depot/1/CesiumWorldTerrain/v1.2/16/20975/46531.terrain?v=1.2.0&extensions=octvertexnormals-metadata-watermask mesh 0 primitive 0: Can't delete non-editable component.

Is this valuable enough to keep the FName creation around? Probably not. And if debugging in this way, you can just make a test branch, and put the naming back in anyway. Interesting though.

azrogers commented 1 month ago

I think this suggestion was made a bit jokingly, but I do think it would be good to wrap this in a define. Something like CESIUM_CREATE_TILE_NAMES. Would mean that future developers don't have to hunt through old pull requests to figure out how to enable this debugging feature.

csciguy8 commented 1 month ago

@azrogers Sensible suggestion. I've made this change, and it's ready for another look.

azrogers commented 1 month ago

Looks good to me. Thanks @csciguy8!