-
Notifications
You must be signed in to change notification settings - Fork 330
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 compute_tile_highlight to foundation/ColorMap #2801
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.
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!
@@ -93,6 +96,10 @@ class ColorMap | |||
|
|||
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
.
@@ -93,6 +96,10 @@ class ColorMap | |||
|
|||
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.