Skip to content
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

Add ProPhoto RGB standard #413

Merged
merged 1 commit into from
Aug 18, 2024
Merged

Add ProPhoto RGB standard #413

merged 1 commit into from
Aug 18, 2024

Conversation

Kirk-Fox
Copy link
Contributor

This pull request adds the ProPhoto RGB standard along with a u16 -> f64 lookup table for IntoLinear. Ideally, I would like to implement a fast lookup for FromLinear similar to that of fast-srgb8, but I need to figure out how to adapt that to u16 first.

Completes prophoto-rgb task of #408, by implementing the ProPhoto RGB standard.

Copy link

codspeed-hq bot commented Aug 13, 2024

CodSpeed Performance Report

Merging #413 will not alter performance

Comparing Kirk-Fox:prophoto (c808f52) with master (5476be6)

Summary

✅ 47 untouched benchmarks

@Ogeon
Copy link
Owner

Ogeon commented Aug 17, 2024

Thanks, this looks good, but I think it's a bit excessive to have a 16bit lookup table enabled by default. I would like to explore a code generation option for cases like these later, so let's skip it for now.

@Kirk-Fox
Copy link
Contributor Author

That's fair. I went ahead and removed the lookup table. I'm planning on opening a new pull request that will add macros that will implement both lookup tables and fast f32 to uint conversion similar to that of fast-srgb8.

@Ogeon
Copy link
Owner

Ogeon commented Aug 17, 2024

Thank you! As one last thing, do you think you could rebase this to a single commit? The old P3 commit(s) are still in there, and it would be nice to not have them in the main branch.

I went ahead and removed the lookup table. I'm planning on opening a new pull request that will add macros that will implement both lookup tables and fast f32 to uint conversion similar to that of fast-srgb8.

My hunch is that it would fit best to be generated by the build script, behind a feature flag. Similar to the SVG/CSS color names. That way, only those who want the big tables get them. Alternatively, let people generate them during runtime. That's another research project...

@Kirk-Fox
Copy link
Contributor Author

I'm sorry. I'm still getting used to using git. How would I rebase this into a single commit?

@Ogeon
Copy link
Owner

Ogeon commented Aug 17, 2024

No worries! The merge in there makes it a bit harder than it usually is, so it may be worth following the answer here: https://stackoverflow.com/questions/30136558/how-to-squash-commits-which-have-merge-commit-in-between

Make a note of the commit hash first, so you can easily go back to it (restore it with git reset --hard my-hash). It's visible here on github, too, so no harm done unless you force push.

@Ogeon
Copy link
Owner

Ogeon commented Aug 17, 2024

An alternative approach, if the merge conflict fix isn't too bad to lose, is to reset the branch to the commit before you merged and squash from there. Then, you can follow a simpler guide and finally use git rebase master to bring everything up to date. I would type something more detailed out, but I think any guide is more reliable than me when I'm tired while having a bit of a cold. 😬

Copy link

codecov bot commented Aug 17, 2024

Codecov Report

Attention: Patch coverage is 81.81818% with 12 lines in your changes missing coverage. Please review.

Project coverage is 82.90%. Comparing base (fab4412) to head (c808f52).
Report is 8 commits behind head on master.

Additional details and impacted files
@@            Coverage Diff             @@
##           master     #413      +/-   ##
==========================================
+ Coverage   82.80%   82.90%   +0.09%     
==========================================
  Files         130      132       +2     
  Lines       20039    20260     +221     
  Branches    20039    20260     +221     
==========================================
+ Hits        16593    16796     +203     
- Misses       3319     3337      +18     
  Partials      127      127              
Components Coverage Δ
palette 82.96% <91.85%> (+0.10%) ⬆️
palette_derive 81.98% <ø> (ø)

@Kirk-Fox
Copy link
Contributor Author

I think that was successful. Thank you! I think I understand git much better now.

@Ogeon
Copy link
Owner

Ogeon commented Aug 17, 2024

Yep, looks great! Git is sort of conceptually simple (a directed graph of versions, where branches are "bookmarks"), but the interface and terminology isn't always easy to learn at first.

I'll merge it after the last test is done, but this is all for this one. Thank you! 🙏

@Kirk-Fox
Copy link
Contributor Author

My hunch is that it would fit best to be generated by the build script, behind a feature flag. Similar to the SVG/CSS color names. That way, only those who want the big tables get them. Alternatively, let people generate them during runtime. That's another research project...

This seems to be the best option, yeah. I'm hard at work on it =)

@Ogeon
Copy link
Owner

Ogeon commented Aug 18, 2024

It could be nice to start with the u8 tables, at least, but I can't say I have thought about all the details. Let's see how it turns out, I guess. 😅

@Ogeon Ogeon merged commit dcc802a into Ogeon:master Aug 18, 2024
19 of 20 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants