-
Notifications
You must be signed in to change notification settings - Fork 622
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
Implement ImageEncoder for hdr where color type is Rgba32F #2013
Implement ImageEncoder for hdr where color type is Rgba32F #2013
Conversation
please give me some hints as to what kind of tests I can add to make sure this works as expected :) |
can we add a reference test that compares images written with HDR to images written with EXR? How does that work in the current test suite? |
The relevant file is |
If you're interested in reviving this PR, I think it is in good shape. Just needs a rebase and ideally also updates to |
Sorry, I'm not good at github workflow. I don't konw how to change this pr. |
{ | ||
let cp = to_rgbe8(pix); | ||
let cp = flattened_rgbe_pixels.next().unwrap(); // we know it's here because the length is checked earlier |
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 this be included in the chain of zip
calls above? Roughly the same as the old version, but passing flattened_rgbe_pixels
instead of scanline
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 be possible, maybe this was done to minimize the diff for an easier review. unfortunately i don't have time at the moment :/
@zhouhang95 GitHub only lets the original PR author or a repository maintainer update a PR |
but it should be possible to checkout the branch of this pr and then open a new pr with your own branch :) |
Co-authored-by: Johannes-Vollmer <[email protected]>
This can be closed now as it was merged in #2246 |
Thanks everyone! |
this unlocks: