Converting code-blocks to literalincludes in ReadingAndWritingImageFiles.rst #1583
Conversation
MeghaS94
force-pushed
the
docs/Use-literalinclude-readWriteImageFilesrst
branch
from
9e9b137
to
3eabcc2
Compare
There was a problem hiding this comment.
This looks good, thank you!
Something is going wrong with the use of clamp, leading the automated build to fail. Is it failing for you? CLick on the "details" link in the "Some checks were not successful" box at the bottom of the PR page, then click on the link for one of the builds with the red 'x'. Then click on the gear icon in the upper right, then select "View Raw Logs", which will take you here: https://pipelinesghubeus23.actions.githubusercontent.com/zPfJKzHjlSLzAWgFtXFhgrPxbuE47i1FSnIRmNn4KqKh3jJuNs/_apis/pipelines/1/runs/3733/signedlogcontent/20?urlExpires=2023-10-27T00%3A50%3A46.8203549Z&urlSigningMethod=HMACV1&urlSignature=AQi%2Bc3%2FzEYYbJcSNIktfzgHf8NH%2B7nYlYoI1Sl7JbYI%3D
I'm not entirely sure what is causing this to fail when the previous code succeeded. A solution is to change the calls to clamp to std::clamp, which is preferable anyway.
Also, the new code produces a number of warning messages, mostly for uninitialized variables, could you possibly look into modifying the example code so it compiles without warnings? Don't worry about the unused variable warnings, those are to be expected given the incomplete nature of the example code.
| @@ -9,7 +9,7 @@ Document Purpose and Audience | |||
| ============================= | |||
|
|
|||
| This document shows how to write C++ code that reads and writes OpenEXR | |||
| 2.0 image files. | |||
| 3.0 image files. | |||
There was a problem hiding this comment.
The 2.0 is actually correct here, since it refers to version 2 of the file format, not of the library release, although the text should make that clear, it is indeed confusing.
There was a problem hiding this comment.
My mistake! I will revert this change.
I also get the same build failures with the use of clamp, I will take a look and see if using std::clamp gets rid of them. Taking a look at the other comments as well.
| @@ -81,8 +81,6 @@ namespace XXX { | |||
| #include "C_IStream_read.cpp" | |||
| #include "C_IStream_seekg.cpp" | |||
| #include "C_IStream_tellg.cpp" | |||
| #include "gamma.cpp" | |||
There was a problem hiding this comment.
If gamma.cpp and makePreviewImage.cpp have been subsumed by previewImageExamples.cpp, they should be removed from the git repo, via
git rm gamma.cpp makePreviewImage.cpp
There was a problem hiding this comment.
One question before pushing new changes - does it make sense to use Imath::clamp() instead of std::clamp()? clamp is only available in std from c++17.
|
@MeghaS94, will you have time to finish off these changes? I'd like to accept them if we can resolve the remaining issues. I'll be away through December but will have time to follow up more then. Thanks! |
|
Yes, apologies for the delay, I am resuming work on this now, I'll be able to finish off these changes. |
…les.rst Signed-off-by: Megha S <megha199501@gmail.com>
Signed-off-by: Megha S <megha199501@gmail.com>
MeghaS94
force-pushed
the
docs/Use-literalinclude-readWriteImageFilesrst
branch
from
00a02fa
to
8ca895f
Compare
Signed-off-by: Megha S <megha199501@gmail.com>
…les.rst (AcademySoftwareFoundation#1583) * Converting code-blocks to literalincludes in ReadingAndWritingImageFiles.rst Signed-off-by: Megha S <megha199501@gmail.com> * Addressed review comments Signed-off-by: Megha S <megha199501@gmail.com> * Changing from std::clamp to Imath::clamp Signed-off-by: Megha S <megha199501@gmail.com> --------- Signed-off-by: Megha S <megha199501@gmail.com>
…les.rst (#1583) * Converting code-blocks to literalincludes in ReadingAndWritingImageFiles.rst Signed-off-by: Megha S <megha199501@gmail.com> * Addressed review comments Signed-off-by: Megha S <megha199501@gmail.com> * Changing from std::clamp to Imath::clamp Signed-off-by: Megha S <megha199501@gmail.com> --------- Signed-off-by: Megha S <megha199501@gmail.com>
fixes #1549