-
-
Notifications
You must be signed in to change notification settings - Fork 3.5k
Add Tint support in WebGL #3709
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
Conversation
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.
Good first pass! A few requested changes inline.
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.
One simple problem that I explained inline. Other than that I'd just like to see 1 or 2 unit tests. I think that you should probably go the route of my first suggestion in the inline comment (initialize _tint
with a default value in RendererGL), then you can add a unit test that checks to make sure that RendererGL._tint is not null after construction. Ping me if you need help figuring out how to make the unit test. Simplest todo here would be to make a new suite in the RendererGL unit tests after the renderer has been made and assert that myp5._renderer._tint
is not null.
Also I would make a new manual test as well.
src/webgl/p5.RendererGL.js
Outdated
@@ -1083,6 +1086,10 @@ p5.RendererGL.prototype._setFillUniforms = function(fillShader) { | |||
if (this._tex) { | |||
fillShader.setUniform('uSampler', this._tex); | |||
} | |||
if (this._tint) { | |||
fillShader.setUniform('uTint', this._tint); | |||
} |
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.
This causes this example sketch to break:
let img;
function preload() {
img = loadImage('../webgl/assets/UV_Grid_Sm.jpg');
}
function setup() {
createCanvas(400, 200, WEBGL);
image(img, 0, 0, 100, 100);
tint(0, 153, 204); // Tint blue
image(img, 100, 0, 100, 100);
}
The uTint
uniform in the shader will have a value of vec4(0, 0, 0, 0) when drawing the first image. There are a number of different solutions that will work. You can initialize _tint
with a value in RendererGL or add an else to this conditional that send a default value when _tint
is null.
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 would recommend using a default value instead of a conditional.
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 have added default value for tint and a couple of unit tests. I wanted to add a unit test for testing whether _tint
is reset after draw()
loop. How can do that?
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 think you need to use a Promise that tests _tint on the second pass through draw
before calling resolve
or reject
. There may be better examples elsewhere but one place to look is the structure.js tests.
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.
Thanks @vedhant!
Thanks for the efforts. So is this feature available in p5.js 1.0.0 now? If not, when will it become available? Thanks. |
I just noticed that using TypeError: canvas.getContext(...) is null (sketch: line 14) |
Thanks @micuat ! Will you open an issue for that? I think it could be related to something else but it will be easier to track in a new thread. |
@stalgiag done! |
Fixes #2530.