-
Notifications
You must be signed in to change notification settings - Fork 206
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
Track image data size independently of the pixelData
array size
#804
Conversation
And use it to track Tile and RasterOverlayTile sizes.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Look good @kring .
Tested this in Unreal and the new code executes just fine.
Just a couple of suggestions before I merge:
- The new member is called
sizeBytes
. Would a name likelastKnownSizeBytes
by a bit more descriptive? - How do you feel about making it a
std::optional
instead of using -1 to indicate not set yet?
If we're nitpicking names, could I suggest rewording it to |
@j9liu Split the difference, it is now |
This looks good to go, merged. Thanks @kring ! |
No need to change it, 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. |
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 |
Added a
sizeBytes
property toImageCesium
, so that its size can be tracked even afterpixelData
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/orRasterOverlayTile
. 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 toTile
orRasterOveralyTile
. So it all gets very elaborate, and hard to justify.