Fix set() to support p5.Graphics objects #8326
Conversation
|
@ksen0 @davepagurek I've implemented the fix for set() to support p5.Graphics objects. The solution adds a check for Graphics instances in p5.Renderer2D.prototype.set() and uses drawImage() with the graphics canvas directly (avoiding .get() as suggested). |
|
@ksen0, is this change satisfactory or is there anything further that needs to be done? |
|
Hi @ksen0 and @davepagurek, |
|
@ksen0 or @davepagurek , could you please review this so that I can continue working on it?. |
davepagurek
left a comment
davepagurek
left a comment
There was a problem hiding this comment.
looking good so far, just one code style comment. The other suggestion I have is to add a visual test so that we can automatically catch it if this breaks in the future.
| y = Math.floor(y); | ||
| if (imgOrCol instanceof Image) { | ||
| if (imgOrCol instanceof Graphics) { | ||
| this.drawingContext.save(); |
There was a problem hiding this comment.
This looks good, is the code in this branch the same as in the Image branch though? if so maybe we can simplify this by using an || condition and just having it be one branch?
There was a problem hiding this comment.
Yes, they are the same. I will update it.
|
@davepagurek I have added your recommendation and also test cases. Should I also add manual test examples? |
davepagurek
left a comment
davepagurek
left a comment
There was a problem hiding this comment.
Thanks, this is looking almost ready to go! I think tests for just this feature don't need to be in their own file, can we move them into maybe https://github.com/processing/p5.js/blob/dev-2.0/test/unit/core/rendering.js ?
|
@davepagurek I have added the test to your preferred location. Please let me know if anything else is required. |
2 participants
Resolves #2984
Changes:
Screenshots of the change:
Before vs After Comparison
PR Checklist
npm run lintpasses