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

Implementation of gaussian blur and box blur and raster node category #2477

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

calvintvu
Copy link

Closes #995

Implements both gaussian and box blurs under raster category

Screen.Recording.mp4

@calvintvu
Copy link
Author

Typo in commit message

@TrueDoctor TrueDoctor self-requested a review March 25, 2025 06:44
@Keavon
Copy link
Member

Keavon commented Mar 25, 2025

I'll try to review this tomorrow.

@TrueDoctor
Copy link
Member

Is there a specific reason why you chose to convert this to an image data type first instead of using or inbuilt one? Also @Keavon should blur happen in srgb or linear? They should give different results, so we have to think about which one is correct / what is compatible with other tools

@TrueDoctor
Copy link
Member

It also looks like the current implementation quantizes the values to 8 bit which is most likely not what we want. We should also take SIMD friendliness into account here and potentially try to re-enable that target flag and hope that the chrome bug from two years ago has been fixed

@Keavon
Copy link
Member

Keavon commented Mar 25, 2025

We should have both linear and gamma, with linear as default. https://www.youtube.com/watch?v=LKnqECcg6Gw

@calvintvu calvintvu force-pushed the blur-implementation branch from 606c186 to 4182032 Compare March 26, 2025 01:09
@calvintvu
Copy link
Author

I've updated it to convert from linear/nonlinear before/after blurring using the functions from graphene_core::raster::Channel and changed from u8 values to f32

@Keavon
Copy link
Member

Keavon commented Mar 26, 2025

Thanks. In the future, could you please add your followup work as additional commits instead of amending the original commit, so it's possible to compare them?

@calvintvu
Copy link
Author

Yes ill keep that in mind

@Keavon
Copy link
Member

Keavon commented Mar 26, 2025

A couple code review notes before I do a proper code review:

  • You're repeating a whole lot of code by doing X then Y without reusing the logic, please do a self-review pass keeping DRY in mind
  • Please replace whole-number decimals so they end in a decimal point (.) instead of point-zero (.0), per our style guide
  • Run cargo clippy

@Keavon Keavon force-pushed the master branch 4 times, most recently from aa7ff13 to e11b57a Compare April 6, 2025 11:41
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.

Gaussian blur node
3 participants