Nabla icon indicating copy to clipboard operation
Nabla copied to clipboard

Image Upload Utilities

Open Erfan-Ahmadi opened this issue 1 year ago • 11 comments

Sister PR: https://github.com/Devsh-Graphics-Programming/Nabla-Examples-and-Tests/pull/11

  • [x] Apply fixes from uploadBuffer that avoids infinite loops
  • [x] Add srcFormat to function params (or consider getting ICPUImage?)
  • [x] Use copy filter to copy regions
    • [x] Create ICPUImage from staging mem ptr
  • [x] Improve code by using an Iterator that gives you the next region to copy based on available memory and what already has been copied
  • [x] Use Convert Filter if srcFormat != dstImage->format
  • [x] Write new tests for image upload utility in Example 09 to copy mini/weird regions and present
  • [x] Remove old format promotion code and use ImageUploadUtility
    • [x] Test
  • [x] Finally remove old code in IGPUObjectFromAssetConverter and Use the imageUploadUtility
  • [x] Format promotion should take SFormatImageUsage as params
  • [x] ~~Consider Refactoring some of the functions for calculating image data size and put them in asset namespace ~~
  • [x] Decision: there is nothing at the moment in need of a functionality existing in the image upload.
  • [x] Devsh Nuke Requests
    • [x] https://github.com/Devsh-Graphics-Programming/Nabla/pull/389#issuecomment-1187245707
    • [x] https://github.com/Devsh-Graphics-Programming/Nabla/pull/389#issuecomment-1187257118
  • [x] Nuke regenerateMipmaps
  • [x] Merge Swapchain Resize Work
  • [x] Resize window to same size as the images
  • [x] E_FORMAT_CLASS for block compressed formats and more
    • [x] validation in copy filter about being in the same format class
  • [ ] Apply that IImage fix from ditt branch

Erfan-Ahmadi avatar Jul 18 '22 11:07 Erfan-Ahmadi

Can you nuke implicit format conversions in Image loaders when they encounter R8_SRGB format ?

Can you nuke implicit format conversions in Image loaders when they encounter R8_SRGB format ?

Link to code please @devshgraphicsprogramming

Erfan-Ahmadi avatar Jul 18 '22 11:07 Erfan-Ahmadi

Can you nuke implicit format conversions in Image loaders when they encounter R8_SRGB format ?

Link to code please @devshgraphicsprogramming

This function can be nuked https://github.com/Devsh-Graphics-Programming/Nabla/blob/master/include/nbl/asset/interchange/IImageAssetHandlerBase.h#L179

This can be simplified and images can be used "as is" https://github.com/Devsh-Graphics-Programming/Nabla/blob/32ccd78961a8647dd29b403f90bff50995a96ae8/src/nbl/asset/interchange/CImageLoaderTGA.cpp#L266 So this function can be greatly simplified to not do any conversions at all, just deal with image flipping: https://github.com/Devsh-Graphics-Programming/Nabla/blob/32ccd78961a8647dd29b403f90bff50995a96ae8/src/nbl/asset/interchange/CImageLoaderTGA.cpp#L159

Not sure if https://github.com/Devsh-Graphics-Programming/Nabla/blob/518ef10fc445326c9b9274216bc5800545ab2a8d/include/nbl/asset/format/EFormat.h#L594

and https://github.com/Devsh-Graphics-Programming/Nabla/blob/518ef10fc445326c9b9274216bc5800545ab2a8d/include/nbl/asset/interchange/IImageAssetHandlerBase.h#L34

duplicate each other

Can you nuke implicit format conversions in Image loaders when they encounter R8_SRGB format ?

Link to code please @devshgraphicsprogramming

This function can be nuked https://github.com/Devsh-Graphics-Programming/Nabla/blob/master/include/nbl/asset/interchange/IImageAssetHandlerBase.h#L179

This can be simplified and images can be used "as is"

https://github.com/Devsh-Graphics-Programming/Nabla/blob/32ccd78961a8647dd29b403f90bff50995a96ae8/src/nbl/asset/interchange/CImageLoaderTGA.cpp#L266

So this function can be greatly simplified to not do any conversions at all, just deal with image flipping: https://github.com/Devsh-Graphics-Programming/Nabla/blob/32ccd78961a8647dd29b403f90bff50995a96ae8/src/nbl/asset/interchange/CImageLoaderTGA.cpp#L159

@devshgraphicsprogramming convertR8ToR8G8B8Image is a non-trivial promotion, it's not a simple convert filter I do in the image upload, It uses a special swizzle (RRRN) so that the first component will be copied to the others to get a grayscale colour rather than red.

So I'm not sure how to handle this best, if we want to handle in image upload convert filter, Should we always copy the first components to the next ones? It's a bit complex for other formats.

I think it's best to handle it in my convert filter and use CSwizzleAndConvertImageFilter instead of CConvertFormatImageFilter, but again I'm not sure exactly how to. Some guidance would be much appreciated.

And for the ones in TGALoader that use createAndconvertImageData mentioned in your message, I think we can get rid of those because they don't do any special swizzle they just use convert filter https://github.com/Devsh-Graphics-Programming/Nabla/blob/32ccd78961a8647dd29b403f90bff50995a96ae8/src/nbl/asset/interchange/CImageLoaderTGA.cpp#L266 https://github.com/Devsh-Graphics-Programming/Nabla/blob/32ccd78961a8647dd29b403f90bff50995a96ae8/src/nbl/asset/interchange/CImageLoaderTGA.cpp#L159

Erfan-Ahmadi avatar Aug 10 '22 10:08 Erfan-Ahmadi

Can you nuke implicit format conversions in Image loaders when they encounter R8_SRGB format ?

Link to code please @devshgraphicsprogramming

This function can be nuked https://github.com/Devsh-Graphics-Programming/Nabla/blob/master/include/nbl/asset/interchange/IImageAssetHandlerBase.h#L179 This can be simplified and images can be used "as is" https://github.com/Devsh-Graphics-Programming/Nabla/blob/32ccd78961a8647dd29b403f90bff50995a96ae8/src/nbl/asset/interchange/CImageLoaderTGA.cpp#L266

So this function can be greatly simplified to not do any conversions at all, just deal with image flipping: https://github.com/Devsh-Graphics-Programming/Nabla/blob/32ccd78961a8647dd29b403f90bff50995a96ae8/src/nbl/asset/interchange/CImageLoaderTGA.cpp#L159

@devshgraphicsprogramming convertR8ToR8G8B8Image is a non-trivial promotion, it's not a simple convert filter I do in the image upload, It uses a special swizzle (RRRN) so that the first component will be copied to the others to get a grayscale colour rather than red.

So I'm not sure how to handle this best, if we want to handle in image upload convert filter, Should we always copy the first components to the next ones? It's a bit complex for other formats.

I think it's best to handle it in my convert filter and use CSwizzleAndConvertImageFilter instead of CConvertFormatImageFilter, but again I'm not sure exactly how to. Some guidance would be much appreciated.

And for the ones in TGALoader that use createAndconvertImageData mentioned in your message, I think we can get rid of those because they don't do any special swizzle they just use convert filter

https://github.com/Devsh-Graphics-Programming/Nabla/blob/32ccd78961a8647dd29b403f90bff50995a96ae8/src/nbl/asset/interchange/CImageLoaderTGA.cpp#L266

https://github.com/Devsh-Graphics-Programming/Nabla/blob/32ccd78961a8647dd29b403f90bff50995a96ae8/src/nbl/asset/interchange/CImageLoaderTGA.cpp#L159

Ah, that thing, I think its quite inconsequential, the only reason we did a (RRR,1) swizzle is because these channels need to be filled with something and this swizzle looked nice in debug tools.

In reality, any further "real use" of such image will only ever use the red channel (use as bumpmap in shader or creating a derivative map).

You can use the (0,0,0,1) swizzle for channels that are not present in the promoted input during the conversion (i.e. if you have a RG texture that gets promoted to RGBA, fill B with 0 and A with 1).

My review here is complete, can you fix the Clang (Android CI) errors, they're all about templates

My review here is complete, can you fix the Clang (Android CI) errors, they're all about templates

I'll try

Erfan-Ahmadi avatar Aug 25 '22 13:08 Erfan-Ahmadi

Ok I'm done with my first review.

/ci test this

Erfan-Ahmadi avatar Aug 26 '22 11:08 Erfan-Ahmadi

/ci test this

Erfan-Ahmadi avatar Aug 26 '22 11:08 Erfan-Ahmadi