-
-
Notifications
You must be signed in to change notification settings - Fork 3.3k
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
[p5.js 2.0] State machines and renderer refactoring #7270
base: dev-2.0
Are you sure you want to change the base?
Conversation
@davepagurek Following on what we discussed regarding the renderer, I've set up an implementation of the renderer state machine here. For 2D most of the things are working since it is relatively simple, for WebGL however it seems there are some states not being tracked correctly, causing tests to fail. |
oops I resolved the merge conflicts with the dev-2.0 updates, but that also brought in some more bits that I need to convert to use states. I'll push another commit soon |
Some properties and methods of p5 instance still need to be taken cared of. Remove references to DOM in constructor.
This need to be refactored out eventually as many don't make sense being on p5.Graphics
@davepagurek I don't want to change too much of WebGL at the moment seeing that you and others are also working on it so I can't fix all the WebGL tests. The main failure is likely linked to p5.Framebuffer and possibly also the WebGL filters. The overall code is still a bit too spaghetti, I'll try to get things a bit more streamlined but have a busy period coming up so I'll see how much I can complete. I've added some notes below regarding things that eventually should be implemented, let me know if they don't make sense. Let me know if discussing over a call is easier as well.
|
I just thought of a possible idea for p5.Graphics, perhaps I can leverage the new module API where it is a function taking in the signature of |
Move "push()" and "pop()" functions to core/transform module
Also WebGL methods are not attached to p5.Graphics yet as they need to use the new module syntax, @davepagurek do you have an idea of around when would be a good point to do that conversion for WebGL? Probably need to align with other things going on so we don't convert in the middle of someone's work. Another thing is that not everything make sense to attach to p5.Graphics, with a general guideline being only renderer related stuff should be attached, other environment related stuff should use the p5 instance version (eg. constants, framerates, data loading, interactions, objects such as p5.Color so probably also p5.Texture etc as well). |
@limzykenneth Luke's project should be wrapping up this week, so that could potentially be a good point to start? But also if we're mostly renaming properties and maybe moving where they're defined, I can resolve any conflicts that come up with Luke's and Garima's projects if this is in the way of more refactor work. |
@davepagurek The For state management solutions, each have their own trade off I think. I like the stateless version of a function idea but it introduces double implementation for one functionality which can be a maintenance overhead. Now that I think about it, going back to your original idea, what if all the rendering functions in the renderers are stateless, then the p5 instance will handle all state management and also state translation? For that to work we need a few requirements to be met:
Just thinking out loud at the moment and just looking at what I came up with above, it might not be very feasible? It is probably desirerable for addons to call the p5 instance Reset states feels like a wrapper around what we currently do as it will need to be wrapped in I probably need to think a bit (while resisting the urge to rewrite...) 🤔 |
I was thinking of tackling the things in #7270 (comment) after this PR and Garima's PR are merged in. For the For renderer statelessness, it does seem like it'd be a big refactor to fully pull things apart, and you're right that addon authors and contributors will probably be more familiar with the public APIs, so it'd be good to let them just use those. Generally not all methods need it anyway (e.g. for defaultStates() {
return { ... }
}
constructor() {
this._defaultStates = this.defaultStates()
this.states = this.defaultStates()
}
runStateless(keys, cb) {
this.push()
for (const key in keys) {
this.states[key] = this._defaultStates[key] // No deep copy here because it won't modify it anyway
}
cb()
this.pop()
}
filter(filterType) {
this.runStateless(['imageMode', 'rectMode', 'userFillShader'], () => {
// do the filter
})
} |
Many of them don't have the implementation on the renderer so we basically need to move the implementation over so just a find and replace might not work fully. I'll do a bit of clean up and refactor first to see how far I can take it. I think a selective resetable state might be the way, although I want to think a bit more about the API, eg. the default states might make more sense statically attached to the renderer (or not). |
@davepagurek I think I got graphics frame buffer working! Have a try and see if it works for you as well? |
Looks like it's mostly working now! One thing I notice is that something might be a bit off with the cameras. I'm using a webgl main canvas and this test sketch: const sketch = function(p){
let myBuffer, pg, torusLayer, boxLayer;
p.setup = function(){
p.createCanvas(200, 200, p.WEBGL);
pg = p.createGraphics(100, 100, p.WEBGL);
torusLayer = pg.createFramebuffer();
};
p.draw = function(){
drawTorus();
p.background(200);
pg.background(50);
pg.imageMode(pg.CENTER);
pg.image(torusLayer, 0, 0);
p.imageMode(p.CENTER)
p.image(pg, 0, 0, 100, 100);
};
function drawTorus() {
// Start drawing to the torus p5.Framebuffer.
torusLayer.begin();
// Clear the drawing surface.
pg.clear();
// Turn on the lights.
pg.lights();
// Rotate the coordinate system.
pg.rotateX(p.frameCount * 0.01);
pg.rotateY(p.frameCount * 0.01);
// Style the torus.
pg.noStroke();
pg.box(20);
// Start drawing to the torus p5.Framebuffer.
torusLayer.end();
}
};
new p5(sketch); The 2.0 branch looks like this: While on 1.11.0 it looks like this, which is expected: I'm not 100% sure what's up yet. If the graphic is the same size as the main canvas, the box is still in the lower right corner, so it's not that the graphic is using the main canvas's viewport size. Drawing directly to the graphic is fine without the intermediate framebuffer. Drawing to a framebuffer on the main canvas is fine too. So it seems like there's something about framebuffers on graphics that still has an issue. |
Might be a pixel density mismatch, I'll look into it. |
Actually I think it might be @davepagurek Got it working, just one change: p.draw = function(){
drawTorus();
p.background(200);
pg.background(50);
pg.imageMode(p.CENTER); // <---- from `pg.imageMode(pg.CENTER)`
pg.image(torusLayer, 0, 0);
p.imageMode(p.CENTER)
p.image(pg, 0, 0, 100, 100);
}; The constants are not attached to the graphics anymore and should just use the ones belonging to the instance. |
Ahhhh that makes sense, good catch! I think everything's fine then. I've updated #7230 to add a line about adding constants to |
@davepagurek I think this is about as much as I can do to fix tests without doing the larger WebGL refactor, I'll see if there are other parts to work on first but do let me know when we are ready to start refactoring RendererGL. |
This is necessary to avoid cyclic imports causing ReferenceError. Extrating implementation into own exported properties can help reduce code duplicated in the future.
Garima's project is merged into |
I had a look at the merge conflicts and I'm not immediately confident about resolving it, not sure if it will be too complicated if you were to do the conflict resolution along with some of the refactoring? We can do a session together at the start so that we cover both set of changes in case either of us missed anything if it helps? |
I think most of the merge conflicts were because the indentation changed (e.g. moving some things into a function, or moving some things out of a function) so I tried just adjusting the indentation of the conflicting files to match, merging, and then putting the indentation back. That made the vast majority of the conflicts go away! So I'll probably do that again if we have more upstream changes in the future. Anyway, I think this should be good to go now. |
Many of the remaining will need a better implementation of p5.Color internal API
More p5.Color normalization required
Renderer state machine
The base
p5.Renderer
class will provide astates
property that will keep track of all states within the renderer that are affected bypush()
andpop()
. The base version looks like the following:Each renderer that extends
p5.Renderer
should add any additional states to thestates
properties when it should be affected bypush()
andpop()
, eg.this.states.someState = 'someValue'
. The base implementation ofpush()
andpop()
will keep track of the states withpush()
pushing a copy of the current state into an array thenpop()
restores the last saved state in the array to the renderer's current state. (See line comments for a bit more details below)The individual renderer should call
super.push()
andsuper.pop()
in their own implementation ofpush()
andpop()
in addition to acting on the necessary changes in states.OOP hierachy
The previous hierachy has
p5.Element
as the base class whichp5.Renderer
extends andp5.Renderer2D/GL
again extends. This means all renderer are assumed to have ap5.Element
backing. Part of the changes made here is to remove this assumption sop5.Renderer
can be extended into renderers that does not render to a HTML element.Eventually, the individual renderers
p5.Renderer2D
andp5.RendererGL
will have their own reference top5.Element
that they can operate through.Overall event handling is still something that needs to be think through for this case though.
Global mode
_setProperty()
is no longer used. Global variables will now all be using getters that refers to the value in the p5 instance.Pending global functions to be attached in the same way.