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.

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

Jump to bottom

[p5.js 2.0] State machines and renderer refactoring #7270

Draft
wants to merge 24 commits into
base: dev-2.0
Choose a base branch
from
Draft

[p5.js 2.0] State machines and renderer refactoring #7270

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
24 commits
Select commit Hold shift + click to select a range
09f57c9
Set up states machine in p5.Renderer
limzykenneth Sep 15, 2024
ab22f1d
Expose p5.Element methods to p5.Renderer2D directly
limzykenneth Sep 17, 2024
c5441dc
Move more things into state
davepagurek Sep 17, 2024
e93cbd5
Start converting WebGL state
davepagurek Sep 17, 2024
94e8267
Move more properties into states
davepagurek Sep 17, 2024
c80e364
Merge branch 'dev-2.0' into 2.0-modules
davepagurek Sep 18, 2024
f0b3766
Fix some uses of .elt
davepagurek Sep 18, 2024
7299220
Minor cleanup
limzykenneth Sep 18, 2024
84ecc61
Rework how variables are exposed in global mode
limzykenneth Sep 18, 2024
282cabb
Global functions now also use getter
limzykenneth Sep 18, 2024
3a356d5
Move DOM initialization from p5.Renderer to individual renderers
limzykenneth Sep 19, 2024
6ef1afc
Concentrate DOM creation of renderer in the createCanvas method
limzykenneth Sep 20, 2024
2f7f824
Indentation
limzykenneth Sep 20, 2024
9c47a4c
Fix p5.Graphics creation
limzykenneth Sep 21, 2024
c39cfa6
p5.Graphics acts as wrapper of p5.Renderer
limzykenneth Sep 21, 2024
263ae57
Fix p5.Graphics.remove and simplify it
limzykenneth Sep 21, 2024
8427978
Fix webgl canvas creation
limzykenneth Sep 21, 2024
5c68e24
Minor adjustment to p5.Renderer and p5.Graphics remove
limzykenneth Sep 21, 2024
e3c3683
Make resizeCanvas() independent of DOM
limzykenneth Sep 22, 2024
f1d8735
Remove renderer createCanvas() method as it is redundant with constru…
limzykenneth Sep 22, 2024
7a91e53
Global width/height read directly from renderer
limzykenneth Sep 22, 2024
b07b438
Move ownership of pixel density to renderer
limzykenneth Sep 22, 2024
55c45ed
Fix a few tests
limzykenneth Sep 22, 2024
beb432f
Fix a few more tests
limzykenneth Sep 22, 2024
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
63 changes: 1 addition & 62 deletions src/core/p5.Renderer.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,8 +19,6 @@ import * as constants from '../core/constants';
*/
p5.Renderer = class Renderer {
constructor(elt, pInst, isMainCanvas) {
// this.elt = new p5.Element(elt, pInst);
this.elt = elt;
this._pInst = this._pixelsState = pInst;
this._events = {};
this.canvas = elt;
Expand Down Expand Up @@ -60,46 +58,11 @@ p5.Renderer = class Renderer {
textWrap: constants.WORD
};
this.pushPopStack = [];


this._pushPopDepth = 0;

this._clipping = false;
this._clipInvert = false;

// this._textSize = 12;
// this._textLeading = 15;
// this._textFont = 'sans-serif';
// this._textStyle = constants.NORMAL;
// this._textAscent = null;
// this._textDescent = null;
// this._textAlign = constants.LEFT;
// this._textBaseline = constants.BASELINE;
// this._textWrap = constants.WORD;

// this._rectMode = constants.CORNER;
// this._ellipseMode = constants.CENTER;
this._curveTightness = 0;
// this._imageMode = constants.CORNER;

// this._tint = null;
// this._doStroke = true;
// this._doFill = true;
// this._strokeSet = false;
// this._fillSet = false;
// this._leadingSet = false;

this._pushPopDepth = 0;
}

id(id) {
if (typeof id === 'undefined') {
return this.elt.id;
}

this.elt.id = id;
this.width = this.elt.offsetWidth;
this.height = this.elt.offsetHeight;
return this;
}

// the renderer should return a 'style' object that it wishes to
Expand All @@ -109,26 +72,6 @@ p5.Renderer = class Renderer {
const currentStates = Object.assign({}, this.states);
Copy link
Member Author

Choose a reason for hiding this comment

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

This should probably be using structuredClone() because states may have objects as properties that won't be fully preserved if copied in this way. It is currently not structuredClone() because somewhere in the WebGL renderer states is a reference to window which cannot be cloned.

Copy link
Contributor

Choose a reason for hiding this comment

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

I believe only some base types work in structured clone, and I'm not certain all the things we put into this state conform to that (e.g. in WebGL, currently we have a camera object.) We have a few options here, do you have any preference between:

  • making an interface like clone() that state properties can optionally implement so that we can have custom object types in the state
  • forcing all state to be a base type and not storing custom classes

I think the former might be a smaller refactor, but that extra flexibility might also mean a higher surface area for bugs. If we go for the latter, for WebGL, I think that would mean storing camera properties rather than the camera itself, and adding a custom handler on pop() to reapply those properties to the active camera.

Copy link
Contributor

Choose a reason for hiding this comment

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

I think for performance we might want the option of not deep copying everything if it isn't necessary, so I maybe slightly lean towards the former?

Copy link
Member Author

@limzykenneth limzykenneth Sep 17, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Some of the key things not cloneable for our use will be (including nested in objects) symbols, functions, and DOM nodes.

making an interface like clone() that state properties can optionally implement so that we can have custom object types in the state

Does this mean we will loop through each direct properties of the state object and if there is a method clone(), call it and store that?

forcing all state to be a base type and not storing custom classes

I'm slightly more in favor of this and I think it is similar to the above option (if I understand it correctly) but with the renderer itself doing the clone() step before setting it in the state object.

For something like p5.Camera it was never suited for deep cloning anyway as it would be duplicating the underlying p5 instance and what not. Saving the camera properties then reapplying them to the active camera feels more semantic but I'm not sure if there will be any performance impact (through repeated instantiation of heavy object for example)?

Another option is that the base p5.Renderer.states object is essentially a convenience in that the push and pop stack will be implemented already in the base class so the derived class don't need to implement it, but if the derived class needs more complex behavior including using non-structuredCloneable states, it can perhaps be handled independently in the derived class's push() pop() implementation, if that made sense.

Copy link
Contributor

Choose a reason for hiding this comment

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

Does this mean we will loop through each direct properties of the state object and if there is a method clone(), call it and store that?

Yep!

Another option is that the base p5.Renderer.states object is essentially a convenience in that the push and pop stack will be implemented already in the base class so the derived class don't need to implement it, but if the derived class needs more complex behavior including using non-structuredCloneable states, it can perhaps be handled independently in the derived class's push() pop() implementation, if that made sense.

My comment about perf was mostly about avoiding automatic deep copying of everything, so I'm good with this too if the more expensive clones are done explicitly when needed. So maybe this method would mean that by default, everything is a simple type that should be replaced instead of mutated in place so we can keep doing Object.assign(...) instead of a structuredClone(...), and then a renderer can do whatever copy it needs manually in its push override? And it should be just a few things needing that custom behaviour.

Copy link
Member Author

Choose a reason for hiding this comment

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

If we can further limit state properties to be basically primitives only (copy types only), Object.assign() should be enough, it is just when the state properties are objects or arrays then structuredClone() would be needed. I'm fine going with the primitives only approach for an overall lighter state management by default if there's no obvious need for reference types to be used as state properties?

If that sounds good, I can try to have a look at WebGL renderer states or since there are other WebGL stuff going on as well, maybe you'd have a better idea of when to review them? I'm going to move back onto the hierachy refactor and instance states next so it's not immediately blocking at this point.

Copy link
Contributor

Choose a reason for hiding this comment

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

hmm right, I guess we have a lot of array states currently in WebGL as that's the format colors are stored in, and those are all treated as reference types right now. To avoid deep copies on all those, we'd have to wrap them in some other class to avoid the structured clone, so it feels like maybe that'd add more overhead than it's worth. Are there many 2D properties that need a clone? Would relying on object reference for everything by default be a possibility?

I tried doing some quick find-and-replacing this morning to start converting state into WebGL here: https://github.com/limzykenneth/p5.js/compare/2.0-modules...davepagurek:p5.js:2.0-webgl?expand=1 This is currently using the .copy() method on things if it exists, but we can change how that's done based on what we decide here, and currently this changeover has broken more tests that will need looking into. But anyway since this is just changing how some properties are accessed, I think it's ok to make these changes whenever, and it won't be too big of a merge for the other contributors.

Copy link
Member Author

Choose a reason for hiding this comment

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

For 2D the only one using reference type is tint using an array of colors as far as I remember, but it is actually currently on the base p5.Renderer class as WebGL can tint as well. If there is either a guarantee or just enforcement through documentation that if reference types are set as state properties, it must not be separately modified, eg. additional tints creates new array and never reuses it, it would still be fine to just Object.assign().

You should be able to push to this PR directly if you want to do the conversion and test fixing as necessary. I'll merge from dev-2.0 regularly as I work on this too.

this.pushPopStack.push(currentStates);
return currentStates;
// return {
// properties: {
// _doStroke: this._doStroke,
// _strokeSet: this._strokeSet,
// _doFill: this._doFill,
// _fillSet: this._fillSet,
// _tint: this._tint,
// _imageMode: this._imageMode,
// _rectMode: this._rectMode,
// _ellipseMode: this._ellipseMode,
// _textFont: this._textFont,
// _textLeading: this._textLeading,
// _leadingSet: this._leadingSet,
// _textSize: this._textSize,
// _textAlign: this._textAlign,
// _textBaseline: this._textBaseline,
// _textStyle: this._textStyle,
// _textWrap: this._textWrap
// }
// };
}

// a pop() operation is in progress
Expand All @@ -137,10 +80,6 @@ p5.Renderer = class Renderer {
pop (style) {
this._pushPopDepth--;
Object.assign(this.states, this.pushPopStack.pop());
// if (style.properties) {
// // copy the style properties back into the renderer
// Object.assign(this, style.properties);
// }
}

beginClip(options = {}) {
Expand Down
13 changes: 13 additions & 0 deletions src/core/p5.Renderer2D.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,19 @@ class Renderer2D extends Renderer {
super(elt, pInst, isMainCanvas);
this.drawingContext = this.canvas.getContext('2d');
this._pInst._setProperty('drawingContext', this.drawingContext);
this.elt = elt;

// Extend renderer with methods of p5.Element with getters
this.wrappedElt = new p5.Element(elt, pInst);
for (const p of Object.getOwnPropertyNames(p5.Element.prototype)) {
if (p !== 'constructor' && p[0] !== '_') {
Object.defineProperty(this, p, {
get() {
return this.wrappedElt[p];
}
})
}
}
Copy link
Member Author

Choose a reason for hiding this comment

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

For changing the hierachy of the OOP structure, p5.Element is created as a property wrappedElt of p5.Renderer2D as p5.Renderer may not need any DOM nodes. This creates a problem which is that p5.Element methods are expected to be present in the renderer object (returned by createCanvas() for example) so that one may call .id() or .mousePressed() on it.

This change above more or less preserve the existing behavior outwardly by aliasing p5.Renderer.id() to p5.Renderer.wrappedElt.id() using getter (may not be necessary and can just be directly assigned). This gives p5.Renderer2D the appearance of being p5.Element but not technically an instance of it.

Another less magical option is to make a breaking change so that the renderer's underlying p5.Element can only be accessed through a property of the renderer, and so getting the id of the element backing a renderer would be more like canvas.element.id() instead of the current canvas.id(). This option has potential to break quite a bit of existing sketches though as it is quite common to do canvas.parent("#some-div") to place a canvas in the DOM.

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm ok with either of those (maybe we could make a mixin() function so that other renderers can easily do the same thing if we go with the first option?)

Another thought: do users expect a renderer to come out of createCanvas or could that return the element of the renderer? Off the top of my head I can't think of (public) APIs on a renderer itself that aren't just on p5.Element that people might be using. I suppose not all renderers have an element to return though (maybe undefined is a valid return type here?)

Copy link
Member Author

Choose a reason for hiding this comment

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

Hmm...perhaps returning p5.Element for those using it could be something createCanvas returns as it aligns with the other createX() functions in the DOM module as well. However in this case I guess the created renderer will not be available publicly necessarily then.

Copy link
Contributor

Choose a reason for hiding this comment

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

Right, I think that's OK if renderers are private if renderers have a way of adding public methods to p5. So things like sphere() could exist just in the WebGL renderer, added to p5

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah, that one is somewhere on my list for this PR, might go with the existing method (defining p5.prototype.sphere() that calls this._renderer.sphere() or might go with the getter technique here as well. We can discuss when I get to that implementation.

}

getFilterGraphicsLayer() {
Expand Down
1 change: 0 additions & 1 deletion test/unit/accessibility/describe.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,6 @@ suite('describe', function() {
let cnv = p.createCanvas(100, 100);
cnv.id(myID);
myp5 = p;
console.log("here", p.describe);
};
});
});
Expand Down
Loading