search

CesiumGS / cesium-native

Apache License 2.0
414 stars 210 forks source link

Track image data size independently of the `pixelData` array size #804

Closed kring closed 6 months ago

kring commented 7 months ago

Added a sizeBytes property to ImageCesium, so that its size can be tracked even after pixelData has been cleared (such as in CesiumGS/cesium-unreal#1330.

This is a little hacky. Arguably this information should be stored somewhere else, such as is in the Tile and/or RasterOverlayTile. But putting it here keeps things simpler, particularly around thread safety. In CesiumGS/cesium-unreal#1330, the pixel data is cleared in the worker thread. But worker threads, by design, don't have access to Tile or RasterOveralyTile. So it all gets very elaborate, and hard to justify.

j9liu commented 6 months ago

The new member is called sizeBytes. Would a name like lastKnownSizeBytes by a bit more descriptive?

If we're nitpicking names, could I suggest rewording it to sizeInBytes or byteSize? 😄

csciguy8 commented 6 months ago

@j9liu Split the difference, it is now lastKnownSizeInBytes

csciguy8 commented 6 months ago

This looks good to go, merged.

Thanks @kring !

kring commented 6 months ago

No need to change it, lastKnownSizeInBytes is a good enough name. But I think sizeBytes is also fine, perhaps a little better. The reason is that "descriptor + units" is a common naming convention used all over. See Cartographic: longitudeDegrees, heightMeters. Perhaps degreesLongitude and metersHeight would be better? It's a bit of a bike shed, but I don't think so. For one thing, bytesSize and degreeLongitude both sound awkward, so should the units be singular or plural? More importantly, putting the most important bit first (longitude, height, size) slightly improves discoverability and auto-completion experience.

As for "last known", well, that's fine I guess. It comes back to my statement at the top of this PR that this a bit hacky. It's really the size for caching purposes.

Like I said, no need to change anything, just wanted to share my thought process on this.

j9liu commented 6 months ago

The reason is that "descriptor + units" is a common naming convention used all over.

I have a bit of a gripe with this formatting too! 😅 But no need to change it when it's already widely used. I still prefer sizeBytes over lastKnownSizeInBytes. If it's okay, I'd rather push a commit to main to undo the name change