-
-
Notifications
You must be signed in to change notification settings - Fork 3.5k
Fix background(img) handling in WEBGL mode in RendererGL #7963
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
base: main
Are you sure you want to change the base?
Conversation
🎉 Thanks for opening this pull request! For guidance on contributing, check out our contributor guidelines and other resources for contributors! Thank You! |
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 for taking this on! To make sure this continues to work, do you think we could add a unit test or a visual test? Here's an example of a visual test that loads and uses an image, maybe we can add a similar one to that file to ensure background(img)
works?
p5.js/test/unit/visual/cases/webgl.js
Lines 160 to 175 in 746a481
visualTest( | |
'Object with different texture coordinates per use of vertex keeps the coordinates intact', | |
async function(p5, screenshot) { | |
p5.createCanvas(50, 50, p5.WEBGL); | |
const tex = await p5.loadImage('/unit/assets/cat.jpg'); | |
const cube = await new Promise(resolve => p5.loadModel('/unit/assets/cube-textures.obj', resolve)); | |
cube.normalize(); | |
p5.background(255); | |
p5.texture(tex); | |
p5.rotateX(p5.PI / 4); | |
p5.rotateY(p5.PI / 4); | |
p5.scale(80/400); | |
p5.model(cube); | |
screenshot(); | |
} | |
); |
To test those, you can run npm run test
(or npm run test/unit/visual
to just run visual tests.) It will create a screenshot in test/unit/visual/screenshots
that you can examine to see if it looks correct, and if it isn't, you can delete the screenshot and JSON file it makes, edit the code, and then rerun the tests to generate a new one.
@@ -2545,4 +2557,24 @@ p5.prototype._assert3d = function (name) { | |||
|
|||
p5.RendererGL.prototype.tessyVertexSize = 12; | |||
|
|||
p5.RendererGL.prototype.background = function (...args) { |
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.
It looks like we have a duplicate implementation here that overrides the method inside the class above, are we able to merge this with the implementation above? It looks like this is the code currently being executed, so we could replace the implementation above with the body of this function and then remove it.
@@ -2545,4 +2557,24 @@ p5.prototype._assert3d = function (name) { | |||
|
|||
p5.RendererGL.prototype.tessyVertexSize = 12; | |||
|
|||
p5.RendererGL.prototype.background = function (...args) { | |||
if (args.length === 1 && args[0] instanceof p5.Image) { |
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.
There are some other image-like things that we probably want to work here, e.g. p5.Graphics
or p5.MediaElement
(for a webcam capture) or p5.Texture
or p5.Framebuffer
. (Maybe at that point it makes sense to flip the logic to be, if the argument is NOT a string or color or number?)
const img = args[0]; | ||
this.clear(); // clear previous frame | ||
this._pInst.push(); | ||
this._renderer._setDefaultCamera(); // reset camera |
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.
Since this
is already a renderer, does this._renderer
here work, or does it need to just be this._setDefaultCamera
?
This PR adds support for using background(p5.Image) in WEBGL mode, addressing issue #7917.
What was done:
Notes: