Skip to content

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.

Sign up for GitHub

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

Merged
davepagurek merged 10 commits into from
Jun 6, 2025
Merged

Create Graphics fixing in dev-2.0 branch. #7829

Show file tree
Hide file tree
Changes from 6 commits
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
b78818b
updated-createGraphics
perminder-17 May 18, 2025
178d2cb
fixes
perminder-17 May 18, 2025
50d6530
fixes
perminder-17 May 18, 2025
78cb39d
fixing-tests
perminder-17 May 18, 2025
3d7d1c5
removing sketches
perminder-17 May 18, 2025
c91eb44
fixes
perminder-17 May 18, 2025
3933911
fixing accidental line change
perminder-17 May 26, 2025
63cc5a0
Fixing suggestions from kit
perminder-17 May 26, 2025
4fe8021
Adding Dave's suggestion
perminder-17 May 26, 2025
1f61661
Adding inline docs for (?.)
perminder-17 May 27, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion lib/empty-example/index.html
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,7 @@
background-color: #1b1b1b;
}
</style>
<script src="../p5.rollup.min.js"></script>
<script src="../p5.min.js"></script>
<!-- <script src="../addons/p5.sound.js"></script> -->
<script src="sketch.js"></script>
</head>
Expand Down
4 changes: 2 additions & 2 deletions src/core/p5.Graphics.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,7 +30,7 @@ class Graphics {
const r = renderer || constants.P2D;

this._pInst = pInst;
this._renderer = new renderers[r](this._pInst, w, h, false, canvas);
this._renderer = new renderers[r](this, w, h, false, canvas);

this._initializeInstanceVariables(this);

Expand Down Expand Up @@ -695,4 +695,4 @@ function graphics(p5, fn){
}

export default graphics;
export { Graphics };
export { Graphics };
Copy link
Member

@ksen0 ksen0 May 19, 2025

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

13 changes: 6 additions & 7 deletions src/core/p5.Renderer2D.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -75,7 +75,6 @@ class Renderer2D extends Renderer {
}
// Set and return p5.Element
this.wrappedElt = new Element(this.elt, this._pInst);

this.clipPath = null;
}

Expand Down Expand Up @@ -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?.()) {
Copy link
Member

@ksen0 ksen0 May 19, 2025

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 '&&'?

Copy link
Collaborator Author

@perminder-17 perminder-17 May 26, 2025

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.

p5.js/src/webgl/p5.Shader.js

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.

Copy link
Member

@ksen0 ksen0 May 27, 2025

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?

Copy link
Collaborator Author

@perminder-17 perminder-17 May 27, 2025

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.

this._pInst._accsBackground?.(color._getRGBA([255, 255, 255, 255]));
}

const newFill = color.toString();
Expand Down Expand Up @@ -212,8 +211,8 @@ class Renderer2D extends Renderer {
this._setFill(color.toString());

//accessible Outputs
if (this._pInst._addAccsOutput()) {
this._pInst._accsCanvasColors('fill', color._getRGBA([255, 255, 255, 255]));
if (this._pInst._addAccsOutput?.()) {
this._pInst._accsCanvasColors?.('fill', color._getRGBA([255, 255, 255, 255]));
}
}

Expand All @@ -223,8 +222,8 @@ class Renderer2D extends Renderer {
this._setStroke(color.toString());

//accessible Outputs
if (this._pInst._addAccsOutput()) {
this._pInst._accsCanvasColors('stroke', color._getRGBA([255, 255, 255, 255]));
if (this._pInst._addAccsOutput?.()) {
this._pInst._accsCanvasColors?.('stroke', color._getRGBA([255, 255, 255, 255]));
}
}

Expand Down
12 changes: 9 additions & 3 deletions src/dom/p5.Element.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -64,10 +64,16 @@ class Element {
}

// delete the reference in this._pInst._elements
const index = this._pInst._elements.indexOf(this);
if (index !== -1) {
this._pInst._elements.splice(index, 1);
let sketch = this._pInst;
if (sketch && !sketch._elements && sketch._pInst) {
Copy link
Member

@ksen0 ksen0 May 19, 2025

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

sketch = sketch._pInst; // climb one level up
}

if (sketch && sketch._elements) { // only if the array exists
const i = sketch._elements.indexOf(this);
if (i !== -1) sketch._elements.splice(i, 1);
Copy link
Member

@ksen0 ksen0 May 19, 2025

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?

Copy link
Collaborator Author

@perminder-17 perminder-17 May 26, 2025

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:-

  1. Sketch is null / undefined, so there isn’t any p5 sketch to talk to.

  2. 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.

}


// deregister events
for (let ev in this._events) {
Expand Down