P5.js: Set perPixelLighting to true by default

[stalgiag]

Nature of issue?

• [ ] Found a bug
• [x] Existing feature enhancement
• [ ] New feature request

Most appropriate sub-area of p5.js?

• [ ] Color
• [ ] Core/Environment/Rendering
• [ ] Data
• [ ] Events
• [ ] Image
• [ ] IO
• [ ] Math
• [ ] Typography
• [ ] Utilities
• [x] WebGL
• [ ] Other (specify if possible)

Feature enhancement details:

Should we set perPixelLighting to true by default? Copying a discussion from #3913 here:

@stalgiag
Part of me feels like we should set 'perPixelLighting' to true by default. These lines can feel unnecessarily complicating for the lighting references.

@aferriss
I agree, and I think the per-vertex lighting looks pretty ugly. I also feel like the setAttributes function is a little arcane.

@stalgiag
Yeah I am not sure if there are performance hits with per pixel. The setAttributes function is definitely intended to only be used by people that want a non-traditional webgl setup for whatever reason. The most commonly used attribute modification would theoretically be 'antialias' which would likely be changed by smooth() for most users.

Doing this would be straightforward, and I agree that the per-vertex lighting doesn't look great with the default primitives which have relatively few faces.

I guess my only concern would be performance.

[aferriss]

There likely is a performance hit, but imo is worth the trade off for better rendering. I think unless you're drawing tons and tons of highly tessellated pieces of geometry you won't notice the performance hit.

[LisaMabley]

Would this be a good issue for someone who is new to open source contributing (but an experienced software developer) to tackle? I haven't used p5.js yet and am not yet familiar with the codebase, but I only see perPixelLighting mentioned in a handful of places. Are you hoping that all the places the code calls setAttributes('perPixelLighting', true); would be unnecessary because it would just do this by default?

[aferriss]

Hi @LisaMabley ! Yes I believe your assumption is correct. We can remove the setAttributes calls in the examples once per pixel lighting is the default.

And yep I think this probably is a good issue for a new contributor and you can get familiar with the webGL renderer :D

[stalgiag]

Hello @LisaMabley thanks for the interest! This would be a great place to start contributing. As @aferriss already confirmed, the examples can have the setAttributes('perPixelLighting', true) calls removed. Doing that along with changing the default in this function and changing the documentation for setAttributes() to reflect the new default value, should do the job quite nicely!

[stalgiag]

I just realized that before doing this we should check to see if this bug still exists. Does anyone have access to an iOS12 device that they can check this on? The bottom example here should suffice for testing. I now realize that this example should also probably be removed if we make 'perPixelLighting' the default? Haha 🤦‍♂

[aferriss]

@stalgia I feel like it's worth keeping that example in there just to show what this function does, but maybe we flip the behavior so that it disables per pixel lighting on click rather than enabling?

I've got an iOS 12 device at work that I can check for the bug with, but will have to wait until tomorrow : D

[aferriss]

So unfortunately it seems like per-pixel lighting is pretty damaged on both iOS (6s plus iOS 12.2) and Android (Pixel 2 Android 9). Per-vertex lighting also seemed pretty broken on iOS. The rendering looked the same in all browsers that I tested (Chrome, FF, and Safari on iOS, and Chrome + FF on Android).

I was using the reference example but pasted into the editor (0.9.0), but probably worth testing with master as well:
https://editor.p5js.org/aferriss/sketches/0h-Eh_sPF

Here's what it looked like on Android with per pixel lighting = true
(sorry I don't have a screen recorder on android)

Rendering looked correct when android had per pixel lighting = false

And on iOS with per pixel lighting = true

On iOS without per pixel lighting = false

[stalgiag]

Oof just as I feared. Thank you for the thorough testing @aferris. I opened the appropriate issues. I am going to close this for now as it doesn't make sense to default to per-pixel until it is functioning properly on all devices.

[aferris]

Hi @stalgiag-

You have the wrong person. It looks like you meant to contact @aferriss.

Thanks!

[stalgiag]

Oops sorry about that @aferris 🤦‍♂