-
Notifications
You must be signed in to change notification settings - Fork 345
Add compute_tile_highlight to foundation/ColorMap #2801
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: master
Are you sure you want to change the base?
Conversation
Mango-3
commented
Mar 14, 2020
- Adds the compute_tile_highlight color function to foundation/ColorMap and makes it dll exportable for usage with the plugins.
dictoon
left a comment
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.
Thanks man!
|
|
||
| Color3f evaluate_palette(float x) const; | ||
|
|
||
| APPLESEED_DLLSYMBOL std::array<float,4> compute_tile_highlight_color( |
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.
That's not exactly how I saw it:
First, I would suggest to rename the function since it's not really specific to tile highlights. Maybe evaluate_cosine_palette()? There are many more of those by the way: https://www.shadertoy.com/view/ll2GD3, in the future we could consider adding more as needed.
Also, I think this should be a free function instead of static method as it's not related to the ColorMap class.
Finally, it would be more logical to return a Color3f.
| Color3f evaluate_palette(float x) const; | ||
|
|
||
| APPLESEED_DLLSYMBOL std::array<float,4> compute_tile_highlight_color( | ||
| const size_t thread_index, |
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 we rename the method, those should be renamed as well (index, count sound good enough).
| // Choose one color per thread (see https://www.shadertoy.com/view/wlKXDm). | ||
| std::array<float,4> frame_color; | ||
|
|
||
| assert(thread_count != 0); |
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.
Move first.
|
|
||
| assert(thread_count != 0); | ||
|
|
||
| float t = 2 * 3.14159265359f * (thread_index / static_cast<float>(thread_count)); |
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.
Make all variables const.