Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Feature/1187 Read EXR files from memory #1448
base: master
Are you sure you want to change the base?
Feature/1187 Read EXR files from memory #1448
Changes from 10 commits
a9d19fa
ec8dc6c
e38c111
2e0ab33
35e9c9f
5f9ae1a
c4a65f0
6d66a8f
69dc5e8
d15f044
88622bc
4ea8564
a9a6c81
ef76977
82e2b40
767eec2
e513d7a
0f910f8
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
im not sure how that .rst file is related to the .exr file ?
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 rst file is just a markdown file on github showing the exr file and it details. I just grabbed it because it was smaller than the other exr files in the repo which made the unit test run a but faster.
https://github.com/AcademySoftwareFoundation/openexr/blob/370db2835843ac75f85e1386c05455f26a6ff58c/website/test_images/Chromaticities/Rec709.rst
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 think it would be better to just use an existing one.
grayscale.exr
for example is very small.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.
indeed, better to use an existing one.
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.
you can remove this line, since you list it in data licenses now.
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.
please remove this line
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.
you can remove this line
Check warning on line 40 in vtkext/private/module/vtkF3DEXRReader.cxx
Codecov / codecov/patch
vtkext/private/module/vtkF3DEXRReader.cxx#L40
Check warning on line 47 in vtkext/private/module/vtkF3DEXRReader.cxx
Codecov / codecov/patch
vtkext/private/module/vtkF3DEXRReader.cxx#L47
Check warning on line 49 in vtkext/private/module/vtkF3DEXRReader.cxx
Codecov / codecov/patch
vtkext/private/module/vtkF3DEXRReader.cxx#L49
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.
Why isn't this line covered? Do we need to override this function if it's not called?
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.
from this page
https://openexr.com/en/latest/ReadingAndWritingImageFiles.html#low-level-i-o
it seems like that function needs to be overriden to be a proper interface
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.
not sure to follow that answer
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.
if that method is not used you can just remove 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.
it is a pure virtual function in the base class so i believe it needs an implementation. I can try removing it and see what happens. In the documentation i posted it says it needs an implementation.
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 lets try that. Maybe its needed in some case, but not in our case.
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 does not build without that function being implemented
error: cannot declare variable ‘memoryStream’ to be of abstract type ‘MemStream’
198 | MemStream memoryStream("EXRmemoryStream", this->MemoryBuffer, this->MemoryBufferLength);
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, lets keep it in for now.
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.
can you put this check in the line above ?
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 think this assert should not be here for the memory stream part. BTW it may be better to make it an actual if test since we now can have an empty InteralFileName, I think.
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.
Yes when loading a USD scene the EXRReader will have an empty filename. Interestingly in the test case TestF3DEXRMemReader, If i omit the filename on line 33 I get an error in vtkImageReader2.cxx:111 for not setting a filename.
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->MemoryBuffer
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 is the mother class implementation, why not just rely on it instead of defining your own ?
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 used this for debugging, it can now be removed.