[p5.js 2.0] State machines and renderer refactoring

[limzykenneth]

Renderer state machine

The base p5.Renderer class will provide a states property that will keep track of all states within the renderer that are affected by push() and pop(). The base version looks like the following:

this.states = {
      doStroke: true,
      strokeSet: false,
      doFill: true,
      fillSet: false,
      tint: null,
      imageMode: constants.CORNER,
      rectMode: constants.CORNER,
      ellipseMode: constants.CENTER,
      textFont: 'sans-serif',
      textLeading: 15,
      leadingSet: false,
      textSize: 12,
      textAlign: constants.LEFT,
      textBaseline: constants.BASELINE,
      textStyle: constants.NORMAL,
      textWrap: constants.WORD
};

Each renderer that extends p5.Renderer should add any additional states to the states properties when it should be affected by push() and pop(), eg. this.states.someState = 'someValue'. The base implementation of push() and pop() will keep track of the states with push() pushing a copy of the current state into an array then pop() 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() and super.pop() in their own implementation of push() and pop() in addition to acting on the necessary changes in states.

OOP hierachy

The previous hierachy has p5.Element as the base class which p5.Renderer extends and p5.Renderer2D/GL again extends. This means all renderer are assumed to have a p5.Element backing. Part of the changes made here is to remove this assumption so p5.Renderer can be extended into renderers that does not render to a HTML element.

Eventually, the individual renderers p5.Renderer2D and p5.RendererGL will have their own reference to p5.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.

[limzykenneth]

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

[davepagurek]

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

[limzykenneth]

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

• WebGL attributes should be set per renderer instead of per p5 instance: Fixes p5.RendererGL -> webglVersion -> works on p5.Graphics unit test
• WebGL filter for 2D canvas not working because the copy helper using the RendererGL image() method expects this._pInst to be in WebGL mode but since the renderer for the real p5 instance is 2D, it cannot call WebGL methods (eg. noLights(). Possible solution is to move these methods to be implemented within p5.RendererGL, with the instance method acting as wrapper of renderer method.
• p5.Graphics should not be treated as a valid p5 instance, ie. it should not be used where internal this._pInst is expected. Either use its _renderer property where necessary or the actual _pInst instead.

[limzykenneth]

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 function(p5, fn) where fn is p5.prototype, instead I can attach modules and methods to p5.Graphics so that it can become a pseudo p5 instance where necessary without elements that don't make sense.

[limzykenneth]

p5.Graphics has almost all the needed methods now, with the exception of ~~blendMode()~~ and clearDepth() (clearDepth() may make sense to move to the webgl module, ~~blendMode() need a new home that is also included in p5.Graphics already~~ blendMode() now lives in color/setting.js along side functions like background()).

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

[davepagurek]

@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]

For what to attach to the graphics prototype, what would be the best way for a module to conditionally attach something? A check if fn is equal to p5.prototype? We could maybe pass an additional options object with isGraphics or something.

[limzykenneth]

isGraphics or this instanceof p5.Graphics can both work depending on what's needed, in some cases it can be refactor into getting members from p5.Graphics._renderer or from the p5 instance instead, but if there are properties that should be in p5.Graphics that are not attached yet, it can be looked into for addition.

[davepagurek]

Also once @lukeplowden's and @Garima3110's / @perminder-17's projects are merged, in addition to the renderer changes we're already planning, I'm hoping to do some WebGL restructuring because there's a lot of features whose functionality gets spread out over different files. The refactors I'm planning are:

• At a high level, I want all entrypoints into WebGL mode to be in RendererGL
  • It will delegate to subsystems in other files as necessary.
  • Contributors should always be able to start in RendererGL and then follow the information flow from there rather than trying to find the entrypoint in one of many files.
• Consolidate the rendering code in the retained/immediate files into one system.
  • e.g. in p5.RendererGL.Immediate.js, the _drawImmediateStroke function has basically the same code as a section of drawBuffers in p5.RendererGL.Retained.js, and these should really share an implementation because both draw p5.Geometry objects at the end of the day, just from different sources.
  • I'm thinking of doing something like p5.GeometryBuilder, making a little class that knows about one piece of functionality that the main renderer can call upon. In this case, it would know how to manage and draw buffers.
• Consolidate setting shader uniforms from p5.RendererGL and p5.Shader into one system
  • e.g. in p5.RendererGL, setFillUniforms reads renderer state and sets values on a shader. In p5.Shader, _setMatrixUniforms also reads renderer state and sets uniforms.
  • Like before, this could be encapsulated in an object. I'm thinking of putting all the uniform-setting helpers in one place there, and the renderer would call shaderManager.prepareFillShader(theShader) or prepareStrokeShader or prepareImageShader, and it would handle matrix uniforms and whatever else is needed based on the type.
• Move all shape shape drawing handling to one file
  • Apart from the rendering stuff that I'm already going to move elsewhere, p5.RendererGL.Immediate.js handles converting arbitrary polygons and curve calls into triangles, and part of that implementation is in p5.RendererGL when it sets up tessy, the tessellator library.
  • I think this all can be moved into a single shape manager object
  • @GregStanton's shape drawing refactor will also remove a lot of the implementation details into a common shape drawing module, so the base Renderer will be able to build up a shape using this common code. In endShape, RendererGL's shape manager will just query the result for polylines of vertices, pass that through the tessellator, and then hand the resulting p5.Geometry off to the buffer manager to draw
    • There's also a fair amount of duplicate code right now just interpolating colors along curves, for different types of curves. Luke's project also adds arbitrary numeric attributes in addition to colors. I'm hoping Greg's shape drawing refactor will make it easier for us to do all that interpolation in one place instead of the current 3x duplication of it that we have.
• Move WebGL text rendering in text.js into a manager object like the others above

One other thing that I'm not 100% sure about yet is how to fit in a minimal renderer that can be used by 2D mode to run shader filters. Currently 2D mode uses a WebGL p5.Graphics, which means importing the entire WebGL module, preventing us from unbundling it for size without also losing the new filter implementations. Ideally, we can make a minimal WebGL renderer that imports just a subset of the subsystems, e.g. skipping the shape drawing subsystem because it will always just be drawing a fullscreen rectangle, skipping the text subsystem, etc.

[limzykenneth]

Sounds like a very good plan overall, a few extra points:

At a high level, I want all entrypoints into WebGL mode to be in RendererGL

One thing that would still be external is the extra p5 instance binding (eg. box()) that still needs its own definition attached to p5.prototype. These will be the ones that p5.Graphics attach to itself to gain possibilities of drawing in 3D.

Consolidate the rendering code in the retained/immediate files into one system.

I like the idea of a geometry builder class, one thing to consider is whether to attach it to p5 or not (same with other classes). The case to not attach it would be when it is not expected to be available to the user, ie. only the WebGL renderer use it internally.

One other thing that I'm not 100% sure about yet is how to fit in a minimal renderer that can be used by 2D mode to run shader filters.

I don't know enough about the system so might be wrong but I guess the parts that are needed are essentially textures and fragment shaders? Since the plan is to split things up into smaller subsystems, if these subsystem can be reused (similar to how the module syntax is reused by p5.Graphics) then the filter code or p5.Graphics code or some new internal class (with minimal WebGL context setup) can just import these subsystem independently and leave everything else alone. That way on a build without WebGL, unused code like geometry won't be included; while on full build the same module will only be included once and imported in two places.

[davepagurek]

One thing that would still be external is the extra p5 instance binding (eg. box()) that still needs its own definition attached to p5.prototype. These will be the ones that p5.Graphics attach to itself to gain possibilities of drawing in 3D.

Makes sense! Maybe that can be done in the same file, even if it's done in a different way, so the entrypoints are easier to find.

I don't know enough about the system so might be wrong but I guess the parts that are needed are essentially textures and fragment shaders? Since the plan is to split things up into smaller subsystems, if these subsystem can be reused (similar to how the module syntax is reused by p5.Graphics) then the filter code or p5.Graphics code or some new internal class (with minimal WebGL context setup) can just import these subsystem independently and leave everything else alone. That way on a build without WebGL, unused code like geometry won't be included; while on full build the same module will only be included once and imported in two places.

I think that'll be the plan! I'm not sure yet just how minimal its dependencies can be -- we'll have to strike a balance between using as little as possible, while trying to avoid having duplicate but more minimal implementations of stuff also in other subsystems, which just means more stuff to update and test when we make changes -- so I'll maybe figure that out as we go. As an example, I think it's fine to hardcode a single rectangle into it without relying on the whole shape drawing subsystem, but probably we want the whole shader subsystem to handle all the different ways one might use the filter shader.

[limzykenneth]

Makes sense! Maybe that can be done in the same file, even if it's done in a different way, so the entrypoints are easier to find.

Probably don't recommend having them in the same file as the renderer itself as the primitives are the ones that will be attached to p5.Graphics using the module syntax and the renderer itself don't need to be attached.

[limzykenneth]

@davepagurek I'm trying to fix framebuffer generated from a webgl p5.Graphics at the moment and since what I'm planning now is for p5.Graphics to be a p5 instance lite but without many of the runtime related stuff, the p5.Framebuffer class is trying to access many of the runtime related stuff from the target which will be the p5.Graphics at the moment and so it won't exist.

I'm wondering if it would be possible to split out the renderer (which p5.Graphics will provide) and the runtime (provided with a reference to the current p5 instance, similar to pInst in many classes) to get this to work in the signature of the p5.Framebuffer class?

[davepagurek]

It might be that p5.Framebuffer only needs the renderer and not the runtime. Right now it looks like it uses target directly (as opposed to target._renderer) for:

• Getting webglVersion. This is specific to the WebGL renderer, so rather than having the runtime own it, we could maybe expose it on the runtime via a getter that accesses something on the renderer so that the renderer doesn't have to reach "up" to the runtime to get it
• In Framebuffer.begin/end, calling push, setCamera, resetMatrix, and pop.
  • resetMatrix and setCamera, like the above, could be shifted around so that they can be called directly on the renderer
  • push and pop previously had return values that mattered so they couldn't be called directly on the renderer, but I think that's no longer the case. Is there any difference between calling those methods on the runtime vs on the renderer now? If not, maybe this is also ok to just do on the renderer

This also might be related to something that was the source of a number of bugs in the filter shader implementation, which is that filters were applied using end-user functions like image() and shader() and such, similar to what framebuffers are doing in begin. This lead to some issues where we had to make sure that user state such as imageMode() or translations didn't trickle down and affect the filter runner. I think we've currently got it resetting all the state it needs to, but it feels odd to have to reach "up" to the runtime to call these, and it seems brittle. Maybe ideally there are stateless renderer functions that one could call, and runtime functions internally pass whatever necessary state down to the stateless versions on the renderer?

[limzykenneth]

I think the two fixes should fix most of the things but I'll try and make it just refers to the renderer, although there may be places where p5.Framebuffer itself or something else is looking for a runtime function (I think color() is called somewhere but not sure where). push() and pop()'s implementation are all done in renderer, the runtime just passes it on.

I generally want to keep renderer states with the renderer, I think in the case of something like the filter shader, what we were briefly thinking about before around a minimal WebGL renderer for 2D and minimal 2D renderer for WebGL may be how we solve this? Alternatively and possible simpler, without tanking performance, we can also use the regular renderers we currently have and we instantiate them at the first call then caching them for later use?

[limzykenneth]

There's too many blocking stuff because of it so I've refactored all classes to be by default exported. It turns out the vast majority of the don't need reference to p5 or p5.prototype meaning they don't need to be defined in the new library syntax function, which also means they can be exported.

Going forward, all classes will be defined outside of the new library syntax scope, attached to p5 within that scope, exported and can be imported by other modules.

@davepagurek I've got no errors with testing frame buffer code but it is showing nothing and the tests are rather broken at the moment. I may need to refactor the tests as part of this PR as the sketches I've tested are actually working quite well other than frame buffers.

[davepagurek]

How are you testing framebuffers currently? e.g. using them in the sketch in the preview directory with npm run dev? I can debug that a bit if so, although currently I'm getting some unrelated issues with some lingering references to p5 in p5.Geometry to go through first.

[limzykenneth]

Yes I'm using them in preview with the p5.Graphics createFramebuffer() reference example adapted to instance mode.

const sketch = function(p){
  let myBuffer, pg, torusLayer, boxLayer;
  p.setup = function(){
    p.createCanvas(200, 200);
    pg = p.createGraphics(100, 100, p.WEBGL);

    torusLayer = pg.createFramebuffer();
  };

  p.draw = function(){
    drawTorus();
    p.background(200);
    pg.background(50);
    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();

    // Draw the torus.
    pg.torus(20);

    // Start drawing to the torus p5.Framebuffer.
    torusLayer.end();
  }
};

[davepagurek]

Oh at a glance it looks like you need to draw the framebuffer to the graphic before drawing the graphic back to the main canvas.

I tried adding in pg.image(torusLayer, 0, 0) although it looks like I'm now hitting this error:

Uncaught (in promise) Error: noLights() is only supported in WEBGL mode. If you'd like to use 3D graphics and WebGL, see  https://p5js.org/examples/form-3d-primitives.html for more information.
    _assert3d p5.RendererGL.js:2414
    noLights light.js:1743
    image 3d_primitives.js:3537
    draw index.html:39
    redraw structure.js:381
    _draw main.js:280
    _draw main.js:302
    _draw main.js:302
    _draw main.js:302
    #_start main.js:208
    p5 main.js:182
    <anonymous> index.html:69
p5.RendererGL.js:2414:12

Looks like this._renderer.isP3D is undefined when calling pg.image() instead of on the main instance

[davepagurek]

Also, going back to this:

I generally want to keep renderer states with the renderer, I think in the case of something like the filter shader, what we were briefly thinking about before around a minimal WebGL renderer for 2D and minimal 2D renderer for WebGL may be how we solve this? Alternatively and possible simpler, without tanking performance, we can also use the regular renderers we currently have and we instantiate them at the first call then caching them for later use?

I was initially suggesting still having all state within the renderer, but doing something like this:

class MyRenderer {
  constructor() {
    this.states = {
      imageMode: 'corner'
    }
  }

  // TODO: better naming
  statelessImage(img, x, y, width, height) {
    // draw the image without modifying internals based on state
  }

  image(img, x, y, width, height) {
    if (imageMode === 'center') {
      x -= width/2
      y -= height/2
    }
    this.statelessImage(img, x, y, width, height)
  }
}

...and then we would encourage internals to only use the stateless version.

While a minimal renderer could address this issue, I also wonder if addons are going to have similar issues. e.g. if something is doing some math internally and relies on p5 math functions, they would have to remember to change the angle mode back or the internals will break. I imagine it being easy to run into this problem from Bret Victor's article on learnable programming if you were to try to package a helper function as an addon (at least a similar class of problem, since this case just needs push/pop):

Another possible way to address the same problem without this separation could be a way for addons or internals to quickly reset all state except what they care about, e.g.:

class MyRenderer {
  defaultState() {
    return {
      // ...
    }
  }

  constructor() {
    this.states = this.defaultState()
  }

  resetStates(omit) {
    const newStates = this.defaultState()
    for (const key in omit) {
      newStates[key] = this.states[key]
    }
    this.states = newStates;
  }
}

[limzykenneth]

@davepagurek The noLights() error will need a bigger fix in that ideally the implementation for renderer stays in the renderer, eg. noLights() only changes the internal states of the renderer so when the renderer wants to turn all lights off, it should just call this.noLights() instead of this._pInst.noLights(), that way things can be a bit more portable as well espcially if we need to reuse the noLights() function for a different renderer. Although to do this quite a lot of RendererGL may need to be refactored, I don't want to introduce breaking changes everywhere so want to check when would be a good time to start, also if you have plans for RendererGL refactor as well? We can discuss over a call if it's easier.

---

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:

• Renderers (present and future) can all be implemented stateless
  • Not sure if that works for 2D renderer where its states are handled by the 2D drawing context (this.drawingContext.save())
• All renderers can use the same state translation result, eg. rectMode(CENTER) translate arguments passed to rect() always to the same set of four numbers and all renderers must work with the same set of numbers
• Non renderer states (states not affected by push/pop() may also cause the same thing with potential addons for example

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 elllipse() rather than the renderer's ellipse() for example.

Reset states feels like a wrapper around what we currently do as it will need to be wrapped in push/pop() as well and don't feel as efficient.

I probably need to think a bit (while resisting the urge to rewrite...) 🤔

[davepagurek]

I was thinking of tackling the things in https://github.com/processing/p5.js/pull/7270#issuecomment-2380656539 after this PR and Garima's PR are merged in.

For the this._pInst.something --> this.something change, I wonder if that's something we could do with a find-and-replace just in the webgl folder? I ran find src/webgl -name "*.js" | xargs egrep 'this\._pInst\.\w+\(\)' and it looks like we call createFramebuffer, push, pop, clear, clearDepth, noStroke, noLights, resetMatrix, resetShader, and pixelDensity. Are all of those things that we would be (or should be) able to call directly on the renderer?

---

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 sphere theres usually no need to explicitly pass in all the material properties, its pretty unlikely that you actually want a default-state sphere.) It might be lighter to do a way to reset specific states within a push/pop, and shallow copying just the state you need in? e.g.

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
  })
}

[limzykenneth]

For the this._pInst.something --> this.something change, I wonder if that's something we could do with a find-and-replace just in the webgl folder? I ran find src/webgl -name "*.js" | xargs egrep 'this._pInst.\w+()' and it looks like we call createFramebuffer, push, pop, clear, clearDepth, noStroke, noLights, resetMatrix, resetShader, and pixelDensity. Are all of those things that we would be (or should be) able to call directly on the renderer?

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

[limzykenneth]

@davepagurek I think I got graphics frame buffer working! Have a try and see if it works for you as well?

[davepagurek]

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.

[limzykenneth]

Might be a pixel density mismatch, I'll look into it.

[limzykenneth]

Actually I think it might be imageMode not working.

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

[davepagurek]

Ahhhh that makes sense, good catch!

I think everything's fine then. I've updated https://github.com/processing/p5.js/issues/7230 to add a line about adding constants to p5.Graphics in the backwards compatibility addon.

[limzykenneth]

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

[davepagurek]

Garima's project is merged into dev-2.0 now so I think once those changes are merged into this branch too then I'm good to get started on the refactoring! I can also look into doing that merge manually if things have already moved around too much for git to recognize where things should go.