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

Allow texture and specularMaterial to coexist, explicitly defaulting _hasSetAmbient to false #6135

Closed
wants to merge 7 commits into from
5 changes: 5 additions & 0 deletions src/color/setting.js
Original file line number Diff line number Diff line change
Expand Up @@ -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
Copy link
Contributor

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().

Copy link
Contributor Author

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.

* set color, you should call this function and then call ambientMaterial() to set it
* before drawing.
*
* A <a href="#/p5.Color">p5.Color</a> object can also be provided to set the fill color.
*
* @method fill
Expand Down
52 changes: 45 additions & 7 deletions src/webgl/material.js
Original file line number Diff line number Diff line change
Expand Up @@ -388,6 +388,12 @@ p5.prototype.resetShader = function() {
* You can view more materials in this
* <a href="https://p5js.org/examples/3d-materials.html">example</a>.
*
* If you call this, the color used when computing the ambient term for lighting
* will be the same color as determined by the texture.
* If you want to use a different color than the one given by the texture,
* you should set it by calling ambientMaterial() after calling this but before
* drawing.
*
* @method texture
* @param {p5.Image|p5.MediaElement|p5.Graphics|p5.Texture|p5.Framebuffer|p5.FramebufferTexture} tex image to use as texture
* @chainable
Expand Down Expand Up @@ -512,11 +518,10 @@ p5.prototype.texture = function(tex) {
}

this._renderer.drawMode = constants.TEXTURE;
this._renderer._useSpecularMaterial = false;
this._renderer._useEmissiveMaterial = false;
this._renderer._useNormalMaterial = false;
this._renderer._tex = tex;
this._renderer._setProperty('_doFill', true);
this._renderer._hasSetAmbient = false;

return this;
};
Expand Down Expand Up @@ -719,8 +724,12 @@ p5.prototype.normalMaterial = function(...args) {
/**
* Sets the ambient color of the material.
*
* The ambientMaterial() color is the color the object will reflect
* under **any** lighting.
* The color set with ambientMaterial() is multiplied by the color set
* with ambientLight() and adding up during lighting calculations.
* However, if you call fill() or texture() after calling this,
* the colors determined by those will be used instead.
* Therefore, when using the colors set here, you must either not use
* these functions, or call this function after calling these functions.
*
* Consider an ambientMaterial() with the color yellow (255, 255, 0).
* If the light emits the color white (255, 255, 255), then the object
Expand Down Expand Up @@ -825,7 +834,6 @@ p5.prototype.ambientMaterial = function(v1, v2, v3) {
this._renderer.curAmbientColor = color._array;
this._renderer._useNormalMaterial = false;
this._renderer._enableLighting = true;
this._renderer._tex = null;
this._renderer._setProperty('_doFill', true);
return this;
};
Expand Down Expand Up @@ -897,7 +905,6 @@ p5.prototype.emissiveMaterial = function(v1, v2, v3, a) {
this._renderer._useEmissiveMaterial = true;
this._renderer._useNormalMaterial = false;
this._renderer._enableLighting = true;
this._renderer._tex = null;

return this;
};
Expand Down Expand Up @@ -952,6 +959,38 @@ p5.prototype.emissiveMaterial = function(v1, v2, v3, a) {
* }
* </code>
* </div>
*
* @example
* <div>
* <code>
* let img;
* function preload() {
* img = loadImage('assets/rockies128.jpg');
* }
*
* function setup() {
* createCanvas(100, 100, WEBGL);
* noStroke();
* describe('textured torus with specular material');
* }
*
* function draw() {
* background(0);
* texture(img);
*
* ambientLight(60);
*
* // add point light to showcase specular material
* let locX = mouseX - width / 2;
* let locY = mouseY - height / 2;
* pointLight(255, 255, 255, locX, locY, 50);
*
* specularMaterial(250);
* shininess(50);
* torus(30, 10, 64, 64);
* }
* </code>
* </div>
* @alt
* torus with specular material
*/
Expand Down Expand Up @@ -984,7 +1023,6 @@ p5.prototype.specularMaterial = function(v1, v2, v3, alpha) {
this._renderer._useSpecularMaterial = true;
this._renderer._useNormalMaterial = false;
this._renderer._enableLighting = true;
this._renderer._tex = null;

return this;
};
Expand Down
1 change: 1 addition & 0 deletions src/webgl/p5.RendererGL.js
Original file line number Diff line number Diff line change
Expand Up @@ -711,6 +711,7 @@ p5.RendererGL.prototype.fill = function(v1, v2, v3, a) {
this.drawMode = constants.FILL;
this._useNormalMaterial = false;
this._tex = null;
this._hasSetAmbient = false;
};

/**
Expand Down
108 changes: 108 additions & 0 deletions test/unit/webgl/p5.RendererGL.js
Original file line number Diff line number Diff line change
Expand Up @@ -389,6 +389,114 @@ suite('p5.RendererGL', function() {
expect(pixel[1]).to.equal(128);
expect(pixel[2]).to.equal(128);
});

test('fill() sets _hasSetAmbient to false', function() {
myp5.createCanvas(100, 100, myp5.WEBGL);
myp5.noStroke();
myp5.lights();
myp5.ambientMaterial(255, 255, 255);
myp5.fill(255, 0, 0);
myp5.plane(100);
const pixel = myp5.get(50, 50);
expect(pixel[0]).to.equal(221);
expect(pixel[1]).to.equal(0);
expect(pixel[2]).to.equal(0);
});

test('texture() sets _hasSetAmbient to false', function() {
myp5.createCanvas(100, 100, myp5.WEBGL);
const tex = myp5.createGraphics(256, 256);
tex.noStroke();
for (let i=0; i<256; i++) {
tex.fill(i, i, 255);
tex.rect(0, i, 256, 1);
}
myp5.noStroke();
myp5.lights();
myp5.ambientMaterial(128, 128, 128);
myp5.texture(tex);
myp5.plane(100);
const pixel = myp5.get(50, 50);
expect(pixel[0]).to.equal(112);
expect(pixel[1]).to.equal(112);
expect(pixel[2]).to.equal(221);
});

test('texture() does not set _useSpecularMaterial to false',
function() {
myp5.createCanvas(100, 100, myp5.WEBGL);
const tex = myp5.createGraphics(256, 256);
tex.noStroke();
for (let i=0; i<256; i++) {
tex.fill(i, i, 255);
Copy link
Contributor

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?

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 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.

tex.rect(0, i, 256, 1);
}
myp5.noStroke();
myp5.lights();
myp5.specularMaterial(128, 128, 128);
myp5.texture(tex);
myp5.plane(100);
const pixel = myp5.get(50, 50);
expect(pixel[0]).to.equal(240);
expect(pixel[1]).to.equal(240);
expect(pixel[2]).to.equal(255);
}
);

test('ambientMaterial() does not null texture', function() {
myp5.createCanvas(100, 100, myp5.WEBGL);
const tex = myp5.createGraphics(256, 256);
tex.noStroke();
for (let i=0; i<256; i++) {
tex.fill(i, i, 255);
tex.rect(0, i, 256, 1);
}
myp5.noStroke();
myp5.lights();
myp5.texture(tex);
myp5.ambientMaterial(128, 128, 128);
myp5.plane(100);
const pixel = myp5.get(50, 50);
expect(pixel[0]).to.equal(111);
expect(pixel[1]).to.equal(111);
expect(pixel[2]).to.equal(158);
});
test('specularMaterial() does not null texture', function() {
myp5.createCanvas(100, 100, myp5.WEBGL);
const tex = myp5.createGraphics(256, 256);
tex.noStroke();
for (let i=0; i<256; i++) {
tex.fill(i, i, 255);
tex.rect(0, i, 256, 1);
}
myp5.noStroke();
myp5.lights();
myp5.texture(tex);
myp5.specularMaterial(128, 128, 128);
myp5.plane(100);
const pixel = myp5.get(50, 50);
expect(pixel[0]).to.equal(240);
expect(pixel[1]).to.equal(240);
expect(pixel[2]).to.equal(255);
});
test('emissiveMaterial() does not null texture', function() {
myp5.createCanvas(100, 100, myp5.WEBGL);
const tex = myp5.createGraphics(256, 256);
tex.noStroke();
for (let i=0; i<256; i++) {
tex.fill(i, i, 255);
tex.rect(0, i, 256, 1);
}
myp5.noStroke();
myp5.lights();
myp5.texture(tex);
myp5.emissiveMaterial(128, 128, 128);
myp5.plane(100);
const pixel = myp5.get(50, 50);
expect(pixel[0]).to.equal(240);
expect(pixel[1]).to.equal(240);
expect(pixel[2]).to.equal(255);
});
});

suite('loadpixels()', function() {
Expand Down