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

Fix textureWrap REPEAT for WebGL Power Of Two Image Element #5987

Merged
merged 2 commits into from
Feb 4, 2023

Conversation

kate-grant
Copy link
Contributor

Resolves #5668
Previously setWrapMode checked isPowerOfTwo on element dimensions and not on src image dimensions. This caused textureWrap(REPEAT) to set to CLAMP if this was an HTML element with non-power-of-two element dimensions, but this outcome was designed for non-power-of-two src image dimensions.

Changes:

  • I changed p5.Texture.prototype.setWrapMode to add wrapWidth and wrapHeight to differentiate between HTML element src image dimensions vs. the element dimensions.
  • Added unit tests for textureWrap REPEAT on textures from image elements for power-of-two and non-power-of-two.
  • The power-of-two case should allow REPEAT and the non-power-of-two case should set to CLAMP.

PR Checklist

  • npm run lint passes
  • [Inline documentation] is included / updated
    inline documentation already existed but the code did not perform as described
  • [Unit tests] are included / updated

@welcome
Copy link

welcome bot commented Feb 1, 2023

🎉 Thanks for opening this pull request! Please check out our contributing guidelines if you haven't already. And be sure to add yourself to the list of contributors on the readme page!

Copy link
Contributor

@davepagurek davepagurek left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This solution looks great, and thanks for adding tests! I just have a comment about making the test setup a little more predictable but then I think this is good to go!

p.createImg(texImg2.canvas.toDataURL(), '', 'anonymous', el => {
el.size(50, 50);
imgElementPowerOfTwo = el;
p.texture(imgElementPowerOfTwo);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It looks like this is working right now, but maybe we can make it a tad clearer for readers of the tests which texture will be active at the end of setup? I think the success callback here will always run immediately due to it being a data URL, but I think done() gets called before those callbacks run. Maybe we could do something like this to ensure we always end with texImg1 as the active texture, and that we call done after?

new Promise(resolve => {
  p.createImg(texImg2.canvas.toDataURL(), '', 'anonymous', el => {
    el.size(50, 50);
    imgElementPowerOfTwo = el;
    p.texture(imgElementPowerOfTwo);
    resolve();
  });
}).then(() => new Promise(resolve => {
  p.createImg(texImg3.canvas.toDataURL(), '', 'anonymous', el => {
    el.size(50, 50);
    imgElementNotPowerOfTwo = el;
    p.texture(imgElementNotPowerOfTwo);
    resolve();
  });
})).then(() => {
  p.texture(texImg1);
  done();
});

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

sure thing--added this.

@davepagurek
Copy link
Contributor

Awesome, thanks!

@davepagurek davepagurek merged commit 12ee435 into processing:main Feb 4, 2023
@kate-grant
Copy link
Contributor Author

@davepagurek Thanks! Would it be ok to go ahead and add myself to the contributors list?

@davepagurek
Copy link
Contributor

Oh yes thanks for reminding me!

@davepagurek
Copy link
Contributor

@all-contributors please add @kate-grant for bug, code, test

@allcontributors
Copy link
Contributor

@davepagurek

I've put up a pull request to add @kate-grant! 🎉

@kate-grant
Copy link
Contributor Author

@davepagurek thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Texture from IMG element does not use actual dimensions of image
2 participants