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

Erode, dilate, threshold shader filters match closer to CPU filters #6405

Merged
merged 3 commits into from Oct 7, 2023

Conversation

wong-justin
Copy link
Contributor

Resolves #6398

Changes (discussed in the issue above but summarized again here):

  • THRESHOLD shader tweaked to match CPU filter more closely. Still a few pixels different, but close enough
  • ERODE, DILATE shaders use luminance values to exactly match CPU filters
  • texelSize accounts for pixel density now

Screenshots of the change:

Before After

PR Checklist

  • npm run lint passes
  • [Inline documentation] is included / updated
  • [Unit tests] are included / updated

@@ -1096,7 +1099,7 @@ p5.RendererGL = class RendererGL extends p5.Renderer {
else {
pg.shader(this.filterShader);
this.filterShader.setUniform('tex0', this);
this.filterShader.setUniform('texelSize', [1/this.width, 1/this.height]);
this.filterShader.setUniform('texelSize', texelSize);
this.filterShader.setUniform('canvasSize', [this.width, this.height]);
Copy link
Contributor

Choose a reason for hiding this comment

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

Should these values also be multiplied by pixel density?

Copy link
Contributor

Choose a reason for hiding this comment

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

This is a good question. I think ideally, we'd want to make the shader handle pixel density rather than the user calling the filter function so that it's easier to make density-agnostic sketches. That means the shader would likely want to know about both the width and the pixel density. I think right now this is possible by comparing texelSize and canvasSize, but wouldn't be possible if we also multiply by pixel density here. (Maybe it's worth adding pixelDensity as a separate uniform so it doesn't always have to be calculated by dividing the other two? Not necessary in this PR though!)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I was thinking maybe all shader uniforms should account for pixel density, ie. texelSize = 1 / (width * pixelDensity), 1 / (height * pixelDensity) and canvasSize = width * pixelDensity, height * pixelDensity. That way they would all be doing work at the same level of resolution. Although having canvasSize != width, height is probably not what the user expects.

Copy link
Contributor

Choose a reason for hiding this comment

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

I think the main reason why we might not want that for all uniforms is that if the pixel density is always premultiplied, then the shader can't distinguish between a canvas at 2x density and a canvas at 2x the size. That would mean that on the user's screen, a filter(BLUR, 3) on a 2x display might end up looking less blurry than on a 1x display, since the blur radius would be smaller relative to the physical size of the canvas. I think I naturally interpret the 3 in filter(BLUR, 3) as being the same units the rest of p5 uses, pixels times pixel density, rather than real pixels.

I think it's ok having most uniforms come with pixel density premultiplied as long as we also pass the density into the shader on its own too though.

@aferriss
Copy link
Contributor

aferriss commented Sep 13, 2023

Thanks for making the PR Justin! Apologies for steering you wrong initially with the non-luminance based erode/dilate filters but I'm glad we got there in the end.

I wonder if the small differences in threshold is due to different interpolations/anti-alias settings when in webgl mode. Do you notice the difference go away when drawing a rect instead of an ellipse? Either way it seems like the difference is very minor.

It looks good to me!

Copy link
Contributor

@davepagurek davepagurek left a comment

Choose a reason for hiding this comment

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

Nice work @wong-justin!

@wong-justin
Copy link
Contributor Author

Ah yep @aferriss, antialiasing makes a ton of sense. Sure enough, rects match up perfectly.

@Qianqianye
Copy link
Contributor

Thanks @wong-justin for working on it and @davepagurek for the approval. @aferriss, do you think this PR is ready to be merged? Thanks!

@aferriss
Copy link
Contributor

aferriss commented Oct 7, 2023

Yes, lgtm!!

@Qianqianye Qianqianye merged commit bb78e67 into processing:main Oct 7, 2023
2 checks passed
AmrikSD pushed a commit to Potato-Blood/pogo that referenced this pull request Nov 10, 2023
This PR contains the following updates:

| Package | Type | Update | Change |
|---|---|---|---|
| [p5](https://togithub.com/processing/p5.js) | devDependencies | minor
| [`1.7.0` -> `1.8.0`](https://renovatebot.com/diffs/npm/p5/1.7.0/1.8.0)
|
|
[@types/p5](https://togithub.com/DefinitelyTyped/DefinitelyTyped/tree/master/types/p5)
([source](https://togithub.com/DefinitelyTyped/DefinitelyTyped)) |
devDependencies | patch | [`1.7.0` ->
`1.7.3`](https://renovatebot.com/diffs/npm/@types%2fp5/1.7.0/1.7.3) |

---

### Release Notes

<details>
<summary>processing/p5.js (p5)</summary>

###
[`v1.8.0`](https://togithub.com/processing/p5.js/releases/tag/v1.8.0)

[Compare
Source](https://togithub.com/processing/p5.js/compare/v1.7.0...v1.8.0)

<!-- Release notes generated using configuration in .github/release.yml
at v1.8.0 -->

#### What's Changed 🎊

##### WebGL

In this release, p5.js added some new WebGL mode tools. Filters now run
in shaders for extra speed, and you can now run custom filter shaders,
even on 2D canvases. You can now cut holes in shapes with
`beginContour()` and apply vector masks with `beginClip()`. You can
reuse shapes more efficiently with `buildGeometry()` and instanced
rendering. Finally, we have also fixed a number of bugs. *- Summary
written by [@&#8203;davepagurek](https://togithub.com/davepagurek) ✨*

- Add support for beginContour() and endContour() in Webgl mode by
[@&#8203;davepagurek](https://togithub.com/davepagurek) in
[processing/p5.js#6297
- Fix stroke rendering when drawing to framebuffers by
[@&#8203;davepagurek](https://togithub.com/davepagurek) in
[processing/p5.js#6304
- Adds createFilterShader() and custom shader support to the webGL
filter() function by
[@&#8203;wong-justin](https://togithub.com/wong-justin) in
[processing/p5.js#6237
- Fix WebGL text not rendering when rotated 90 degrees by
[@&#8203;davepagurek](https://togithub.com/davepagurek) in
[processing/p5.js#6316
- Fix reading between nested active framebuffers by
[@&#8203;davepagurek](https://togithub.com/davepagurek) in
[processing/p5.js#6314
- Add methods to construct p5.Geometry from other p5 drawing functions
by [@&#8203;davepagurek](https://togithub.com/davepagurek) in
[processing/p5.js#6287
- Handle missing exact edge vertices in buildGeometry by
[@&#8203;davepagurek](https://togithub.com/davepagurek) in
[processing/p5.js#6320
- Fix strokes on framebuffers with different aspect ratios by
[@&#8203;davepagurek](https://togithub.com/davepagurek) in
[processing/p5.js#6339
- Fix freed geometry leaving attributes in a broken state by
[@&#8203;davepagurek](https://togithub.com/davepagurek) in
[processing/p5.js#6323
- Improve performance of line rendering by
[@&#8203;davepagurek](https://togithub.com/davepagurek) in
[processing/p5.js#6230
- Add support for webGL instancing by
[@&#8203;RandomGamingDev](https://togithub.com/RandomGamingDev) in
[processing/p5.js#6276
- Add shaders for filter() constants, and use them by default in P2D by
[@&#8203;wong-justin](https://togithub.com/wong-justin) in
[processing/p5.js#6324
- Fix clip() on both the main canvas and framebuffers by
[@&#8203;davepagurek](https://togithub.com/davepagurek) in
[processing/p5.js#6376
- fixed texture filtering bug in p5.Framebuffer by
[@&#8203;KeyboardSounds](https://togithub.com/KeyboardSounds) in
[processing/p5.js#6420
- Fix clear() on framebuffers on Intel macs by
[@&#8203;davepagurek](https://togithub.com/davepagurek) in
[processing/p5.js#6429
- Fix textureMode(IMAGE) + beginShape(TESS) by
[@&#8203;davepagurek](https://togithub.com/davepagurek) in
[processing/p5.js#6366
- fixed issue
[#&#8203;6440](https://togithub.com/processing/p5.js/issues/6440) by
[@&#8203;Gaurav-1306](https://togithub.com/Gaurav-1306) in
[processing/p5.js#6446
- Erode, dilate, threshold shader filters match closer to CPU filters by
[@&#8203;wong-justin](https://togithub.com/wong-justin) in
[processing/p5.js#6405
- Update WebGL blur filter to match CPU blur more by
[@&#8203;davepagurek](https://togithub.com/davepagurek) in
[processing/p5.js#6460
- Fix camera flipping on framebuffers between push/pop calls by
[@&#8203;davepagurek](https://togithub.com/davepagurek) in
[processing/p5.js#6471
- Setuniform by [@&#8203;Gaurav-1306](https://togithub.com/Gaurav-1306)
in
[processing/p5.js#6474
- resolved issue
[#&#8203;6399](https://togithub.com/processing/p5.js/issues/6399) by
[@&#8203;Gaurav-1306](https://togithub.com/Gaurav-1306) in
[processing/p5.js#6480
- Auto-bind filter shaders to the filter graphic by
[@&#8203;davepagurek](https://togithub.com/davepagurek) in
[processing/p5.js#6482
- new PR for issue
[#&#8203;6383](https://togithub.com/processing/p5.js/issues/6383)(Problem
for diagonal) by
[@&#8203;perminder-17](https://togithub.com/perminder-17) in
[processing/p5.js#6488

##### Friendly Error System (FES)

- Add Hindi translation to FES by
[@&#8203;Ayush23Dash](https://togithub.com/Ayush23Dash) in
[processing/p5.js#6272
- Re-worded lines 413 and 446 of FES Developer Notes by
[@&#8203;OnexiMedina](https://togithub.com/OnexiMedina) in
[processing/p5.js#6307
- Reference FES Contributor Docs inside FES Directory along with a
diagram to understand usages of FES functions by
[@&#8203;Ayush23Dash](https://togithub.com/Ayush23Dash) in
[processing/p5.js#6335
- Fixed typing errors in fes_core.js documentation by
[@&#8203;Garima3110](https://togithub.com/Garima3110) in
[processing/p5.js#6478
- Update friendly_error_system.md by
[@&#8203;Garima3110](https://togithub.com/Garima3110) in
[processing/p5.js#6481
- Update fes_reference_dev_notes.md by
[@&#8203;Garima3110](https://togithub.com/Garima3110) in
[processing/p5.js#6486

##### Reference Documentation Update

We updated a group of p5.js Reference pages as part of 2023 Season of
Docs (SoD) program, with a goal to make them more accessible and
beginner-friendly. Thanks to the SoD technical writer
[@&#8203;nickmcintyre](https://togithub.com/nickmcintyre) ✨.

- Edit docs for math functions by
[@&#8203;nickmcintyre](https://togithub.com/nickmcintyre) in
[processing/p5.js#6281
- docs(typography): fix typos in example for textFont by
[@&#8203;meezwhite](https://togithub.com/meezwhite) in
[processing/p5.js#6401
- Edit docs for p5.Vector by
[@&#8203;nickmcintyre](https://togithub.com/nickmcintyre) in
[processing/p5.js#6340
- Edit docs for pixels functions by
[@&#8203;nickmcintyre](https://togithub.com/nickmcintyre) in
[processing/p5.js#6390
- Edit docs for loading & displaying images by
[@&#8203;nickmcintyre](https://togithub.com/nickmcintyre) in
[processing/p5.js#6425
- Update docs for p5.Image by
[@&#8203;nickmcintyre](https://togithub.com/nickmcintyre) in
[processing/p5.js#6434
- Edit docs for p5.Font by
[@&#8203;nickmcintyre](https://togithub.com/nickmcintyre) in
[processing/p5.js#6453
- Edit docs for image by
[@&#8203;nickmcintyre](https://togithub.com/nickmcintyre) in
[processing/p5.js#6424
- Edit docs for typography load and display by
[@&#8203;nickmcintyre](https://togithub.com/nickmcintyre) in
[processing/p5.js#6450

##### Google Summer of Code (GSoC) 2023 Wrap up

- 🌸 Added GSoC wrap up! by
[@&#8203;dewanshDT](https://togithub.com/dewanshDT) in
[processing/p5.js#6403
- Gsoc 23 Wrapup post by
[@&#8203;Ayush23Dash](https://togithub.com/Ayush23Dash) in
[processing/p5.js#6415
- add GSoC'23 wrapup post for Justin Wong by
[@&#8203;wong-justin](https://togithub.com/wong-justin) in
[processing/p5.js#6418
- Create lichlyter_gsoc\_2023.md by
[@&#8203;katlich112358](https://togithub.com/katlich112358) in
[processing/p5.js#6455
- Create munusshih_gsoc\_2023.md by
[@&#8203;munusshih](https://togithub.com/munusshih) in
[processing/p5.js#6461

##### Other Code Update

- Ask to disable printing when print() called with no arguments by
[@&#8203;davepagurek](https://togithub.com/davepagurek) in
[processing/p5.js#6253
- fix textWidth() and textToPoints() by
[@&#8203;munusshih](https://togithub.com/munusshih) in
[processing/p5.js#6184
- Fix issue where nf with 0 'right' parameter returns undefined in
string by [@&#8203;limzykenneth](https://togithub.com/limzykenneth) in
[processing/p5.js#6291
- Update environment.js with fix for frameRate description by
[@&#8203;quinton-ashley](https://togithub.com/quinton-ashley) in
[processing/p5.js#6269
- Implement clip() to shapes by
[@&#8203;davepagurek](https://togithub.com/davepagurek) in
[processing/p5.js#6306
- Clarified workflow for contributing documentation by
[@&#8203;thatguyseven](https://togithub.com/thatguyseven) in
[processing/p5.js#6312
- Clears MediaElement canvas at the beginning of every frame by
[@&#8203;donaldzhu](https://togithub.com/donaldzhu) in
[processing/p5.js#6309
- Clean up gruntfile release related steps by
[@&#8203;Qianqianye](https://togithub.com/Qianqianye) in
[processing/p5.js#6321
- fix-return-type by
[@&#8203;asukaminato0721](https://togithub.com/asukaminato0721) in
[processing/p5.js#6326
- fix HALF_FLOAT by
[@&#8203;asukaminato0721](https://togithub.com/asukaminato0721) in
[processing/p5.js#6330
- Added .gitattributes to Increase compatability with Window users and
line endings by [@&#8203;SilasVM](https://togithub.com/SilasVM) in
[processing/p5.js#6317
- update all contributors setup by
[@&#8203;gr2m](https://togithub.com/gr2m) in
[processing/p5.js#6341
- refine canvas' type by
[@&#8203;asukaminato0721](https://togithub.com/asukaminato0721) in
[processing/p5.js#6328
- MouseEvent, WheelEvent and KeyboardEvent type by
[@&#8203;asukaminato0721](https://togithub.com/asukaminato0721) in
[processing/p5.js#6329
- fixed-wrong-capture-size-and-freeze-issue by
[@&#8203;Prateek93a](https://togithub.com/Prateek93a) in
[processing/p5.js#5159
- add more event type by
[@&#8203;asukaminato0721](https://togithub.com/asukaminato0721) in
[processing/p5.js#6379
- Main by [@&#8203;j-adel](https://togithub.com/j-adel) in
[processing/p5.js#6374
- Update labeler Github Action by
[@&#8203;stampyzfanz](https://togithub.com/stampyzfanz) in
[processing/p5.js#6395
- add unregisterMethod function by
[@&#8203;capGoblin](https://togithub.com/capGoblin) in
[processing/p5.js#6426
- add before/after preload and setup by
[@&#8203;capGoblin](https://togithub.com/capGoblin) in
[processing/p5.js#6433
- Fix: Misleading error message when NaN passed by
[@&#8203;capGoblin](https://togithub.com/capGoblin) in
[processing/p5.js#6464
- Support pixel density on p5.Image (fixes issue
[#&#8203;6114](https://togithub.com/processing/p5.js/issues/6114)) by
[@&#8203;Gaurav-1306](https://togithub.com/Gaurav-1306) in
[processing/p5.js#6447
- Fix orphan canvas when sketch is removed before canvas creation by
[@&#8203;limzykenneth](https://togithub.com/limzykenneth) in
[processing/p5.js#6355

##### Other Documentation Update

- Fixed GitHub capitalization typo in contributor_docs by
[@&#8203;SilasVM](https://togithub.com/SilasVM) in
[processing/p5.js#6284
- Fixing typo in "What are issues?" by
[@&#8203;snwarner22](https://togithub.com/snwarner22) in
[processing/p5.js#6288
- Fixed GitHub spelling in CONTRIBUTING.md by
[@&#8203;SilasVM](https://togithub.com/SilasVM) in
[processing/p5.js#6295
- Fixed grammatical errors in contributor_guidelines.md by
[@&#8203;thatguyseven](https://togithub.com/thatguyseven) in
[processing/p5.js#6296
- Update documentation_style_guide.md with new guideline by
[@&#8203;zelf0](https://togithub.com/zelf0) in
[processing/p5.js#6334
- add missing code contributors to all contributors in README and
`.all-contributors.rc` file by [@&#8203;gr2m](https://togithub.com/gr2m)
in
[processing/p5.js#6349
- docs(all-contributors): remove
[@&#8203;stellartux](https://togithub.com/stellartux) as requested by
[@&#8203;gr2m](https://togithub.com/gr2m) in
[processing/p5.js#6368
- docs(src/utilities): Use `describe()` instead of `@alt` by
[@&#8203;Zearin](https://togithub.com/Zearin) in
[processing/p5.js#5598
- Fix typo in export path to fix dev mode by
[@&#8203;mykongee](https://togithub.com/mykongee) in
[processing/p5.js#6373
- Improve Readme for future Contributors to codebase by
[@&#8203;Ayush23Dash](https://togithub.com/Ayush23Dash) in
[processing/p5.js#6260
- Fixed mousePressed() Example Error by
[@&#8203;Utkarsh3128](https://togithub.com/Utkarsh3128) in
[processing/p5.js#6413
- Update README.md by
[@&#8203;katlich112358](https://togithub.com/katlich112358) in
[processing/p5.js#6458
- Fixed typing errors in validate_params.js file's documentation by
[@&#8203;Garima3110](https://togithub.com/Garima3110) in
[processing/p5.js#6473
- typo and unused variable from core by
[@&#8203;benschac](https://togithub.com/benschac) in
[processing/p5.js#6476

#### New Contributors 💗

- [@&#8203;munusshih](https://togithub.com/munusshih) made their first
contribution in
[processing/p5.js#6184
- [@&#8203;SilasVM](https://togithub.com/SilasVM) made their first
contribution in
[processing/p5.js#6284
- [@&#8203;snwarner22](https://togithub.com/snwarner22) made their first
contribution in
[processing/p5.js#6288
- [@&#8203;thatguyseven](https://togithub.com/thatguyseven) made their
first contribution in
[processing/p5.js#6296
- [@&#8203;OnexiMedina](https://togithub.com/OnexiMedina) made their
first contribution in
[processing/p5.js#6307
- [@&#8203;donaldzhu](https://togithub.com/donaldzhu) made their first
contribution in
[processing/p5.js#6309
- [@&#8203;gr2m](https://togithub.com/gr2m) made their first
contribution in
[processing/p5.js#6341
- [@&#8203;RandomGamingDev](https://togithub.com/RandomGamingDev) made
their first contribution in
[processing/p5.js#6276
- [@&#8203;mykongee](https://togithub.com/mykongee) made their first
contribution in
[processing/p5.js#6373
- [@&#8203;j-adel](https://togithub.com/j-adel) made their first
contribution in
[processing/p5.js#6374
- [@&#8203;meezwhite](https://togithub.com/meezwhite) made their first
contribution in
[processing/p5.js#6401
- [@&#8203;dewanshDT](https://togithub.com/dewanshDT) made their first
contribution in
[processing/p5.js#6403
- [@&#8203;Utkarsh3128](https://togithub.com/Utkarsh3128) made their
first contribution in
[processing/p5.js#6413
- [@&#8203;KeyboardSounds](https://togithub.com/KeyboardSounds) made
their first contribution in
[processing/p5.js#6420
- [@&#8203;capGoblin](https://togithub.com/capGoblin) made their first
contribution in
[processing/p5.js#6426
- [@&#8203;Gaurav-1306](https://togithub.com/Gaurav-1306) made their
first contribution in
[processing/p5.js#6446
- [@&#8203;katlich112358](https://togithub.com/katlich112358) made their
first contribution in
[processing/p5.js#6455
- [@&#8203;Garima3110](https://togithub.com/Garima3110) made their first
contribution in
[processing/p5.js#6473
- [@&#8203;benschac](https://togithub.com/benschac) made their first
contribution in
[processing/p5.js#6476
- [@&#8203;perminder-17](https://togithub.com/perminder-17) made their
first contribution in
[processing/p5.js#6488
- [@&#8203;lakshay451](https://togithub.com/lakshay451) made their first
contribution in
[processing/p5.js#6493

**Full Changelog**:
processing/p5.js@v1.7.0...v1.8.0

</details>

---

### Configuration

📅 **Schedule**: Branch creation - At any time (no schedule defined),
Automerge - At any time (no schedule defined).

🚦 **Automerge**: Disabled by config. Please merge this manually once you
are satisfied.

♻ **Rebasing**: Whenever PR becomes conflicted, or you tick the
rebase/retry checkbox.

🔕 **Ignore**: Close this PR and you won't be reminded about these
updates again.

---

- [ ] <!-- rebase-check -->If you want to rebase/retry this PR, check
this box

---

This PR has been generated by [Renovate
Bot](https://togithub.com/renovatebot/renovate).

<!--renovate-debug:eyJjcmVhdGVkSW5WZXIiOiIzNy4yMS4wIiwidXBkYXRlZEluVmVyIjoiMzcuMzUuMiIsInRhcmdldEJyYW5jaCI6Im1haW4ifQ==-->

Co-authored-by: Renovate Bot <bot@renovateapp.com>
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.

Discrepancies with shader-based THRESHOLD, ERODE, and DILATE Filters at higher pixel densities
4 participants