Skip to content

Conversation

@Spongman
Copy link
Contributor

@Spongman Spongman commented Dec 24, 2017

setAttributes('perPixelLighting', true) enables use of a Phong specular lighting model.

before & after:
image

this PR includes and relies on some other PRs:

this PR should simplify significantly if those are merged first.

@stalgiag
Copy link
Contributor

Cool! A few thoughts as I am testing everything and preparing to merge your various awesome pull requests over the next few days -

Can you make sure to label something if it is a work-in-progress?
Also do you think I should address your pending GL pull requests in a specific order? Should I assume the three you mentioned above are first in line? Just want to make sure there are as few conflicts as possible with merge order.

@Spongman
Copy link
Contributor Author

awesome! yes, all these PRs are ready to commit (aside from all the bugs i missed, of course ;-)

if you merge those three PRs first (they're all independent), this one should be much simpler. i'll try to fix any conflicts that come up if i catch them in time.

@kjhollen
Copy link
Member

😻 😻 😻 wow these results look awesome! @mlarghydracept do you want to review this one?

@stalgiag
Copy link
Contributor

stalgiag commented Jan 2, 2018

Yeah I agree. That shader has fantastic results! I would say it might be a good idea to write a manual test for this one. Something that switches in and out of the phong specular. Just to have something to test with down the line. Otherwise I would say this looks good to merge but I would like to merge #2446 first. I don't feel qualified to evaluate that one so I pinged @lmccart for a review of it. Will merge this as soon as that is merged.

@lmccart
Copy link
Member

lmccart commented Jan 5, 2018

@mlarghydracept #2446 has been merged

@stalgiag
Copy link
Contributor

stalgiag commented Jan 6, 2018

Okay cool just tested a bunch and this is fantastic! Ready for a merge except two really small requests:

1: Could you put a short instruction in the perPixelLighting manual test? Just for future contributors to know that they should click to test. You can put it in the HTML if that is easier.
2. Also in the manual-test, can you load the teapot model in preload()? I know that several other manual tests don't do this but it is something I realize we should fix as we go because sometimes it is hard to test on slower machines because you can't tell if it is broken while loading.

Thanks again!

@stalgiag stalgiag merged commit e13c9ca into processing:master Jan 6, 2018
@stalgiag
Copy link
Contributor

stalgiag commented Jan 6, 2018

This is a great addition - thank you!

@sanketsingh24
Copy link
Contributor

@Spongman I just wanted to know your logic behind using 500 in line 89 of phong.frag. I am implementing a falloff function which will set custom attenuation, so just wanted to know about this.

@Spongman
Copy link
Contributor Author

Spongman commented Jun 1, 2019

oh you can ignore my code there... it's not correct because it's not scale-invariant. just replace it with whatever you have. i had a PR a while back that included an implementation of Processing's falloff, but it didn't make it in, unfortunately.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants