-
Notifications
You must be signed in to change notification settings - Fork 13
Implementing streamed tiles system for big georeferenced images #205
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
base: master
Are you sure you want to change the base?
Conversation
…d to VkBufferImageCopy used to upload tiles)
tiledGridParams.imageID = 6996; | ||
tiledGridParams.geoReferencedImage = bigTiledGrid; | ||
|
||
DrawResourcesFiller::StreamedImageManager tiledGridManager(std::move(tiledGridParams)); |
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.
Seems reasonable to have a manager that knows everything about the image placement and info, useful for tile calculation.
Just keep in mind that we want to separate the idea of a "loader" and "tileCaclulator".
One actually does loads from disks or wherever, the other calculates which tiles and which mip levels are needed.
One has access to image data, One just knows the resolution and basic information
One doesn't have info about image placement in the worldspace and the view projection, one does.
One is loader, one is tile calculator or SteamedImageManager or whatever you want to call it
62_CAD/DrawResourcesFiller.cpp
Outdated
cachedImageRecord->state = ImageState::CREATED_AND_MEMORY_BOUND; | ||
cachedImageRecord->lastUsedFrameIndex = currentFrameIndex; // there was an eviction + auto-submit, we need to update AGAIN | ||
cachedImageRecord->allocationOffset = allocResults.allocationOffset; | ||
cachedImageRecord->allocationSize = allocResults.allocationSize; | ||
cachedImageRecord->gpuImageView = allocResults.gpuImageView; | ||
cachedImageRecord->staticCPUImage = nullptr; | ||
cachedImageRecord->staticCPUImage = manager.georeferencedImageParams.geoReferencedImage; |
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.
this should stay nullptr, georeferenced images should have nothing to do with staticCPUImage. the cacheImageRecord only holds the status of the gpu allocated georef image.
then you just copy your regions with buffers into it.
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.
i.e. the cpu image should only be part of the loader and not the draw resources filler OR the tile calculator
@@ -879,11 +878,21 @@ void DrawResourcesFiller::addGeoreferencedImage(image_id imageID, const Georefer | |||
return; | |||
} | |||
|
|||
// Generate upload data | |||
auto uploadData = manager.generateTileUploadData(NDCToWorld); |
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.
Correct/Good decision to calculate the tiling and which sections to load here in addGeoreferencedImage
@@ -631,20 +631,19 @@ bool DrawResourcesFiller::ensureMultipleStaticImagesAvailability(std::span<Stati | |||
return true; | |||
} | |||
|
|||
bool DrawResourcesFiller::ensureGeoreferencedImageAvailability_AllocateIfNeeded(image_id imageID, const GeoreferencedImageParams& params, SIntendedSubmitInfo& intendedNextSubmit) | |||
bool DrawResourcesFiller::ensureGeoreferencedImageAvailability_AllocateIfNeeded(StreamedImageManager& manager, SIntendedSubmitInfo& intendedNextSubmit) |
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.
Ok so I have a few requests about this.
This function should only care about the GPU allocation and the image itself and decide which resolution to create it with. (i.e. decide to go full res or streamed with tiles)
And you can know these with only 1. OriginalImageResolution 2. ViewportResolution 3. TileSize and nothing more
I don't see any need for the manager to go through here.
If this function is doing more than just figuring out what resolution to allocate the image in the vram with, then it's a sign something is wrong.
then when you're in addGeorefImage
function, you can look up the cached image and see if it was decided to be "Stremead" or "FullRes" depending on that, you either request loading the whole shit from the loader OR you request tiles (through your tile manager or whatever)
62_CAD/DrawResourcesFiller.cpp
Outdated
{ | ||
outImageParams.extent = { georeferencedImageParams.imageExtents.x, georeferencedImageParams.imageExtents.y, 1u }; | ||
} | ||
else | ||
{ | ||
// TODO: Better Logic, area around the view, etc... | ||
outImageParams.extent = { georeferencedImageParams.viewportExtents.x, georeferencedImageParams.viewportExtents.y, 1u }; | ||
// Pad sides to multiple of tileSize. Even after rounding up, we might still need to add an extra tile to cover both sides. |
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.
continuing from https://github.com/Devsh-Graphics-Programming/Nabla-Examples-and-Tests/pull/205/files#r2242153374
I believe these calculations is better if done inside addGeorefImage
function
Run on this branch:
https://github.com/Devsh-Graphics-Programming/Nabla/tree/geotex_streaming