-
-
Notifications
You must be signed in to change notification settings - Fork 3.5k
Fix MediaElement.copy #7980
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: dev-2.0
Are you sure you want to change the base?
Fix MediaElement.copy #7980
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.
It looks good to me, tagging also @limzykenneth
A reference to global |
@limzykenneth Thanks for the review! Just out of curiosity — I noticed that other MediaElement methods, such as mask, also reference the global p5 object: mask(...args) {
this.loadPixels();
this.setModified(true);
p5.Image.prototype.mask.apply(this, args);
} Do these methods have the same issue, or are they handled differently? |
@pearmini Good spot, I think they should have the same issue as well. |
@limzykenneth Since the other methods have the same issue, would it make sense to address them all in a separate PR? I suspect similar problems might exist elsewhere in the p5 codebase as well. For this PR specifically, would it be possible to merge it as-is? At least, this change ensures that |
@limzykenneth That would be great! We're planning to release support for p5's async setup soon! |
Resolves ml5js/ml5-next-gen#258
Changes:
I'm currently working on updating ml5 to support p5.js version 2.0. However, the following example breaks during inspection:
After debugging, I found that
video.mask(segmentation.mask)
throws the following error:So I updated
fn
top5.prototype
to fix it, based on the reference from the source code in the main branch.p5.js/src/dom/dom.js
Line 4991 in eadd761
Screenshots of the change:
PR Checklist
npm run lint
passes