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 compute_tile_highlight to foundation/ColorMap #2801

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

Mango-3
Copy link
Member

@Mango-3 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.

Copy link
Member

@dictoon dictoon left a 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(
Copy link
Member

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,
Copy link
Member

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);
Copy link
Member

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));
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Make all variables const.

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