-
-
Notifications
You must be signed in to change notification settings - Fork 3.5k
Create Graphics fixing in dev-2.0 branch. #7829
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?
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.
A few minor things
src/core/p5.Graphics.js
Outdated
@@ -695,4 +695,4 @@ function graphics(p5, fn){ | |||
} | |||
|
|||
export default graphics; | |||
export { Graphics }; | |||
export { Graphics }; |
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.
Very minor: trailing slash / accidental line change here
@@ -179,8 +178,8 @@ class Renderer2D extends Renderer { | |||
const color = this._pInst.color(...args); | |||
|
|||
//accessible Outputs | |||
if (this._pInst._addAccsOutput()) { | |||
this._pInst._accsBackground(color._getRGBA([255, 255, 255, 255])); | |||
if (this._pInst._addAccsOutput?.()) { |
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.
Also minor: I don't think I've seen the ?.
pattern in this codebase before, maybe it's more easy to read to explicitly check existance and then '&&'?
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.
Hi Kit, I think here in dev-2.0 branch, we uses chaining as well.
Line 711 in d51184b
this._renderer = context?._renderer?.filterRenderer?._renderer || context; |
I think, if I choose optional chaining operator that will keep our code a bit cleaner than repeating the property name with &&
. If you just confirm me to choose &&
pattern, I can quickly change it.
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.
Minor: No worries, my oversight! Please keep the chaining use but please add a more descriptive incline comment to explain? To make it more easy to understand for new contributors.
Rather than "accessible Outputs" something more like "add accessible outputs if possible; on success...", what do you think?
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.
Yes, sure. I'll do it now.
if (index !== -1) { | ||
this._pInst._elements.splice(index, 1); | ||
let sketch = this._pInst; | ||
if (sketch && !sketch._elements && sketch._pInst) { |
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 a little more inline docs could be useful here to explain the logic
|
||
if (sketch && sketch._elements) { // only if the array exists | ||
const i = sketch._elements.indexOf(this); | ||
if (i !== -1) sketch._elements.splice(i, 1); |
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.
When is this not true / should that path also be handled?
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.
Hi kit,
That condition is false in two cases:-
-
Sketch is null / undefined, so there isn’t any p5 sketch to talk to.
-
Sketch exists but it never set up its
_elements
list.
In both sutiotions there is no list we can remove ourselves from, so the safest action is to do nothing and just reutrn. Trying to touch a list that isn’t there would crash, so skipping the code is exactly what we want.
I think this approach makes sense! Just to think it through, are there any methods that we don't put on graphics that would be necessary for a renderer? e.g. I don't think we add |
Thanks @davepagurek and @ksen0 for your valuable suggestions. I have tried implementing your suggestion, can you please give a final review if everything looks good and we are good to go? |
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.
Looks great, thanks for answering my questions, nothing else from me. Thanks for the great docs
@@ -179,8 +178,8 @@ class Renderer2D extends Renderer { | |||
const color = this._pInst.color(...args); | |||
|
|||
//accessible Outputs | |||
if (this._pInst._addAccsOutput()) { | |||
this._pInst._accsBackground(color._getRGBA([255, 255, 255, 255])); | |||
if (this._pInst._addAccsOutput?.()) { |
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.
Minor: No worries, my oversight! Please keep the chaining use but please add a more descriptive incline comment to explain? To make it more easy to understand for new contributors.
Rather than "accessible Outputs" something more like "add accessible outputs if possible; on success...", what do you think?
Fixes #7821
The renderer writes to
this._pInst.canvas
. With the old codethis._pInst
pointed at the main sketch, sobezier()
drawing went to the main canvas. By passing the wrapper instead,this._pInst.canvas
is the off-screen canvas, so every draw call(including Bézier, spline, …)
ends up in the right place.