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

Feature implementation: spotlight() #3913

Merged
merged 23 commits into from
Aug 11, 2019

Conversation

sanketsingh24
Copy link
Contributor

@sanketsingh24 sanketsingh24 commented Jul 24, 2019

Rebased on top of feature-specularlight, so that needs to be merged first.

fixes- #3894

See manual test /webgl/lights/spotlight/.

Work done-

  • Implemented spotlight uniform in lighting.glsl.
  • Implemented spotLight() in /src/webgl/light.js
  • Added a manual-test-example in /webgl/lights/spotLight/
  • Unit tests
  • Documentation
  • Example

To do-

More to follow...

@sanketsingh24 sanketsingh24 marked this pull request as ready for review July 29, 2019 12:57
@aferriss
Copy link
Contributor

aferriss commented Aug 6, 2019

I think that this PR is in a pretty good state (once the conflicts from specularLight getting merged are resolved). I wanted to again call out to see if @stalgiag @Spongman or anyone else had feedback on this feature or the PR before it gets merged.

</head>

<body>
<header>
Copy link
Contributor

Choose a reason for hiding this comment

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

Small note, can you remove this? Not applicable here as far as I can tell

let color, position, direction;
const length = arguments.length;

switch (length) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Good job juggling so many different parameters!

* <code>
* function setup() {
* createCanvas(100, 100, WEBGL);
* setAttributes('perPixelLighting', true);
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

I'll break this out into an issue.

Copy link
Contributor

@stalgiag stalgiag left a comment

Choose a reason for hiding this comment

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

Great job! Small comments inline but I generally approve :-D

@aferriss aferriss merged commit 7343f16 into processing:master Aug 11, 2019
@sanketsingh24 sanketsingh24 deleted the feature-spotlight branch August 11, 2019 16:21
@aferriss aferriss mentioned this pull request Aug 11, 2019
2 tasks
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.

3 participants