-
-
Notifications
You must be signed in to change notification settings - Fork 3.3k
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
Allow texture and specularMaterial to coexist, explicitly defaulting _hasSetAmbient to false #6135
Allow texture and specularMaterial to coexist, explicitly defaulting _hasSetAmbient to false #6135
Conversation
When drawing using fill(), uAmbientMatColor will look better if it is the same color as the color determined by fill(), so this is the default.
Stop losing the effects of specularMaterial and emissiveMaterial by texture(). Also, the specification that texture is discarded by ambientMaterial(), specularMaterial(), emissiveMaterial() is removed.
Unit test seems fine. Below, I will add screenshots of the change, unit tests, references, and examples. |
In Three.js, specular can coexist naturally with texture as part of a material called phongMaterial. It seems to be a natural process after all, so let's incorporate it into p5.js. |
Make sure fill() and texture() set _hasSetAmbient to false. Make sure texture() does not set _useSpecularMaterial to false. Make sure that material setting functions such as ambientMaterial() do not destroy the contents of the texture.
I forgot that the get function was fixed so fixed some numbers. excuse me.
Unit test completed. Next, I will add the contents that reflect this specification change to each reference of fill(), texture(), and ambientMaterial(). |
Note about fill() setting _hasSetAmbient to false
Added that texture() sets _hasSetAmbient to false, and that the contents of ambientMaterial() can no longer be used by calling fill() or texture().
Added an example where texture and specular can coexist.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry for the delay, I've been recovering the past few days! I think this is a good change, would you be interested in reopening this?
@@ -359,6 +359,11 @@ p5.prototype.colorMode = function(mode, max1, max2, max3, maxA) { | |||
* and all named color strings are supported. In this case, an alpha number | |||
* value as a second argument is not supported, the RGBA form should be used. | |||
* | |||
* In webgl, calling this will use the color set by this function when calculating | |||
* the ambient term in the lighting process. If instead you want to use your own |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe instead of mentioning the ambient term here, we could phrase it like: fill
sets all reflective material colors to the specified color, but if you want to set them individually, use speculatMaterial()
and ambientMaterial()
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Regarding the ambientMaterial flag, I decided not to set it to false. I will close this pull request once. I will launch a pull request on another topic (about omitting processing to null _tex), so please review it.
If I don't call ambientMaterial(), the color won't fade, so that's fine. thank you.
const tex = myp5.createGraphics(256, 256); | ||
tex.noStroke(); | ||
for (let i=0; i<256; i++) { | ||
tex.fill(i, i, 255); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If we're just doing assertions on one pixel, maybe we can do tex.background()
instead of doing a gradient here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I will recreate it when I create a unit test in a new pull request. Later I realized that textures can be monochromatic. excuse me. I will do so when I recreate it.
Setting _hasSetAmbient to false when doing fill() and texture() will make that look the default. Coexistence is possible by calling ambientMaterial() afterwards.
Stop texture() from disabling Materials such as specular. Conversely, it also prevents specularMaterial() etc. from destroying the texture. Allow both to coexist.
Resolves #6133
Changes:
Make _hasSetAmbient false when you call fill().
Set _hasSetAmbient to false when you call texture(). Also, stop setting _useSpecularMaterial and _useEmissiveMaterial to false.
Prevent _tex from being null when calling ambientMaterial(), specularMaterial(), emissiveMaterial().
Screenshots of the change:
latest version output / expected output
PR Checklist
npm run lint
passes