-
Notifications
You must be signed in to change notification settings - Fork 468
Add support for LoadFromEXRMemory as other formats #645
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: main
Are you sure you want to change the base?
Conversation
|
Can you rebase this? |
ba1573c to
cbb8f27
Compare
|
@walbourn rebased and rearranged the code a little bit to give better diff. Please note, that I haven't tested non-windows desktop systems for the template argument deduction |
| _Use_decl_annotations_ | ||
| HRESULT DirectX::LoadFromEXRFile(const wchar_t* szFile, TexMetadata* metadata, ScratchImage& image) | ||
| template <typename StreamType> | ||
| HRESULT LoadFromEXRCommon(StreamType stream, TexMetadata* metadata, ScratchImage& image) |
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 template is not public so it should be placed in an anonymous namespace.
_Use_decl_annotations_ is not valid here and instead it needs to be SAL annotated directly. In this case, it's sufficient to make it:
HRESULT LoadFromEXRCommon(StreamType stream, _Out_opt_ TexMetadata* metadata, ScratchImage& image)
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.
The diff was really bad when it was in the anonymous namespace. Can move it after approval with the removal of _Use_decl_annotations_
| _Use_decl_annotations_ | ||
| HRESULT DirectX::LoadFromEXRFile(const wchar_t* szFile, TexMetadata* metadata, ScratchImage& image) | ||
| template <typename StreamType> | ||
| HRESULT LoadFromEXRCommon(StreamType stream, TexMetadata* metadata, ScratchImage& image) |
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.
Should stream be passed by reference here instead of by value?
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.
It was, however the EXR API expects constchar * which fails the posix builds with Error: /home/runner/work/DirectXTex/DirectXTex/Auxiliary/DirectXTexEXR.cpp:503:44: error: cannot bind non-const lvalue reference of type ‘const char*&’ to an rvalue of type ‘const char*’.
Instead, exploited the rvalue object stream for Win32. Could modify it with std::move for better visibillity?
| #else | ||
| std::wstring wFileName(szFile); | ||
| std::string fileName(wFileName.cbegin(), wFileName.cend()); | ||
| return LoadFromEXRCommon(fileName.c_str(), metadata, image); |
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 code won't build. You are passing a std::wstring as a StreamType.
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.
It is const char*, as it was in the before changes Imf::RgbaInputFile file(fileName.c_str()); Line 375
Tested from memory and from file the following exr: https://polyhaven.com/a/qwantani_dusk_2_puresky
Changes:
Rename *Stream -> *FileStream
Add InputStream wrapper to read from memory
Cut-paste the whole LoadFromEXRFile try-catch into LoadFromEXRCommon
Remove the equal sign from COMBINED_OPENEXR_VERSION as I am exactly on this version and I don't have this virtual method, todo: implement it in the future