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

ShaderTrap: Examples of computing a histogram of values #68

Merged
merged 17 commits into from
Jun 19, 2021

Conversation

Mostafa-ashraf19
Copy link
Contributor

This PR Fixes #63. Providing 6 examples of histogram compute shaders.

@google-cla google-cla bot added the cla: yes label Jun 9, 2021
Copy link
Contributor

@afd afd left a comment

Choose a reason for hiding this comment

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

This looks like a very good start!

I have provided detailed feedback on example 1. Please can you propagate that feedback through all the examples?

Example 2 looks incorrect and I have commented on why. I did not look beyond example 2 - so please can you revise the PR in light of my current feedback and let me know when you're ready for another review?

Also: regarding copyright headers, please see this comment that I made on Shiyu's PR:

#66 (review)

If you can follow equivalent advice for yourself that would be great.

Copy link
Contributor

@afd afd left a comment

Choose a reason for hiding this comment

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

Thanks for this!

I had made a mistake in the issue description for version 3. I have tried to clarify things in the comments here, with a suggestion on how to rewrite version 3 to use vector operations.

Copy link
Contributor

@afd afd left a comment

Choose a reason for hiding this comment

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

Looking good! Next round of changes requested.

Copy link
Contributor

@afd afd left a comment

Choose a reason for hiding this comment

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

I'm running out of time for reviewing today, so here's a very preliminary review with some nits about style. Perhaps you could look through the rest of the PR and fix up any similar style nits? I'll do a review of the actual content next week.

Copy link
Contributor

@afd afd left a comment

Choose a reason for hiding this comment

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

This is looking good. Example 5 doesn't match the requirements of the issue, but other than that the code all looks good - my comments are mainly on your comments :)

Copy link
Contributor

@afd afd left a comment

Choose a reason for hiding this comment

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

One nit - but otherwise looks good to merge assuming the bots pass!

Copy link
Contributor

@afd afd left a comment

Choose a reason for hiding this comment

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

Just noticed that the contributors list is not quite right.

CONTRIBUTORS Outdated Show resolved Hide resolved
CONTRIBUTORS Outdated Show resolved Hide resolved
Copy link
Contributor

@afd afd left a comment

Choose a reason for hiding this comment

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

Thank you for this contribution!

@afd afd merged commit f5227f4 into google:main Jun 19, 2021
@Mostafa-ashraf19 Mostafa-ashraf19 deleted the histogramShaders branch July 19, 2021 13:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Write a series of histogram examples
2 participants