ffd8 / P5LIVE

p5.js collaborative live-coding vj environment!
https://p5live.org
GNU General Public License v3.0
226 stars 35 forks source link

Soft compile function-matching edge cases #24

Closed JunShern closed 4 years ago

JunShern commented 4 years ago

Hey @ffd8, thanks for this awesome and exciting environment!

I'm taking a dive into your develop branch where I see that you've been working on the soft compile feature (mentioned in https://github.com/ffd8/P5LIVE/issues/19).

I'm super keen to get this feature working for my use-cases, and I fixed a few small bugs I came across while using it (https://github.com/ffd8/P5LIVE/pull/23 and https://github.com/ffd8/P5LIVE/pull/22), but I can still see a few more issues which are a bit trickier to solve.

For example, this example illustrates three cases:

let dots = [];
function setup() {
    createCanvas(windowWidth, windowHeight);
    noStroke();
    for (let i=0; i<100; i++) {
        let dot = new Dot();
        dots.push(dot);
    }
}

function draw() {
    background(0);
    dots.forEach(function (dot) {
        dot.draw();
        dot.size += random(-2, 2);
    });
}

class Dot {
    constructor() {
        this.pos = createVector(random(width), random(height));
        this.size = 10;
        this.color = [random(255), random(255), random(255)];
    }

    draw() {
        ellipse(this.pos.x, this.pos.y, this.size, this.size);
    }
}
  1. Unnamed functions

At the current moment, running a soft-compile on the following line actually crashes the program:

    dots.forEach(function (dot) {

As far as I can tell, this is because of the unnamed function. If instead I change the line to

    dots.forEach(function f(dot) {

Then the program doesn't crash. So this workaround works, but it would be nice to support unnamed functions since they are a big part of JavaScript.

  1. Class methods

In the example, I have a class Dot which has a method called draw().

Since the current implementation relies on the function keyword, this method isn't recognized as a function and so changing things inside Dot.draw() triggers a hard recompile, which is not ideal since we should be able to just update the method and do a soft compile.

  1. Support for same function names in different scope/contexts

I haven't explored this one fully yet, but I see a potential issue being that my method Dot.draw() and the main draw() have the same name. This is legal since they exist in different scopes, but at the moment the parser would treat them as the same function (?). Again, not yet tested since anyway class methods are not yet working for the soft compile.


At the end of the day, there are still workarounds which require the user to design their programs to not have these edge cases, but I just wanted to draw your attention to these so that we might get these issues resolved further down the road.

Again, thanks so much for the great work on this platform! :100:

ffd8 commented 4 years ago

Hey @JunShern thanks for pointing this out and the recent special case catches! Indeed – you probably saw the comment in code: // how to undeclare class/function?? – I was writing the softCompile in an airplane haha so I couldn't research how one can re/over-declare a class in an iFrame instance. I think all that's necessary is to replace the entire instance of the class (and internal functions) on any changes.. so I'll have a look now at what's necessary to override the declaration of a class (since my function method of simply overwriting, doesn't fly due to already being declared..). Happy for any ideas/research you find.

ffd8 commented 4 years ago

Aha, this might be the answer, to simply update the code with the class redefined as a variable:

class Polygon {
  constructor(height, width) {
    this.height = height;
    this.width = width;
  }
}

can be replaced by:

Polygon = class {
  //... new stuff here
}
ffd8 commented 4 years ago

Figured it out! Now searching through all lines for classes too, catching their open/close curly braces and can replace them on the fly. Works great for all new instances of class, but not for any that exist.. which makes sense, since those items contain all the methods/vars within the old instance of the class and don't refer to it again. I think this should be expected behaviour.. if one changes the class, then they need to replace the instances of it for new functions.. not sure how easy/hard it would be to both find and replace the methods within each instance (since you wouldn't want to overwrite its constructor).

Here's an example, that let's you quickly replace the instances (since there's a max 20 allows) – which is still smooth in softCompile:

let balls = [];

function setup() {
    createCanvas(windowWidth, windowHeight);
    balls.push(new Balls());
}

function draw() {
    background(0, 15);
    for(let i = 0; i < balls.length; i++) {
        balls[i].draw();
    }

    if(mouseIsPressed) {
        balls.push(new Balls());
        if(balls.length > 20) {
            balls.shift();
        }
    }

    // balls = []; // toggle to purge array
}

class Balls {
    constructor() {
        this.x = random(width);
        this.y = random(height);
        this.s = random(200); // 200;
        this.v = random(1, 5);
    }

    draw() {
        //fill(255, 0, 0);
        ellipse(this.x + frameCount * this.v % width, this.y, this.s);
    }
}
JunShern commented 4 years ago

That was fast, thank you for the quick and effective fix! :tada:

Something which is much more low priority, and may occur only in bad JavaScript practices anyway:

Previously, I was using static variables something like the following (adapted to your example):

class Balls {
    constructor() {
        this.x = random(width);
        this.y = random(height);
        this.own_count = Balls.static_count;
        Balls.static_count = (Balls.static_count + 1) % 20;
    }

    draw() {
        ellipse(this.x, this.y, 100, 100);
        text(this.own_count, this.x, this.y);
    }
}
Balls.static_count = 0;

And since in this case, the static variable Balls.static_count = 0 is not included within the curly braces of the class definition, the soft compile doesn't include that line in the redefinition of Balls.

Being a JavaScript novice, I wrote the static variable in this way because of some tutorial I found online. But now I realize that instead, I can write the same class in a different way, instead keeping the static variable inside the class definition:

class Balls {
    constructor() {
        this.x = random(width);
        this.y = random(height);
        Balls.static_count = ((Balls.static_count || 0) + 1) % 20;
        this.own_count = Balls.static_count;
    }

    draw() {
        ellipse(this.x, this.y, 100, 100);
        text(this.own_count, this.x, this.y);
    }
}

In my opinion, this is a good workaround and also neater in this specific case. But because JavaScript is such a flexible language, it makes handling all these cases very difficult.

But in any case, you've solved my main blockage so I am happy to close this issue. Thanks, @ffd8!

ffd8 commented 4 years ago

Yeah – it's a really fun how flexible the language is and many ways to solve the same thing (looots to learn myself). Oh I like that solution using Balls.static_count within the class. I was playing with passing a param into the constructor instead and came up with a similar: balls.push(new Balls((balls[balls.length-1].own_count+1)%20)); – but again, nice to have such an ID built in.