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.

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

Library functions registered with registerPreloadMethod keep calling _incrementPreload after preloading is done. #5677

Open
1 of 17 tasks
lindapaiste opened this issue May 18, 2022 · 0 comments

Comments

@lindapaiste
Copy link
Contributor

lindapaiste commented May 18, 2022

Most appropriate sub-area of p5.js?

  • Accessibility (Web Accessibility)
  • Build tools and processes
  • Color
  • Core/Environment/Rendering
  • Data
  • DOM
  • Events
  • Friendly error system
  • Image
  • IO (Input/Output)
  • Localization
  • Math
  • Unit Testing
  • Typography
  • Utilities
  • WebGL
  • Other (specify if possible)

p5.js version

1.4.0

Web browser and version

chrome 100.0.4896.127

Operating System

Windows 10

Steps to reproduce this

Steps:

  1. Load up this p5 web editor replicating the issue.
  2. Watch the console to see how it calls setup() repeatedly in an infinite loop.
  3. Notice how window.myLibFunction was initially undefined, but later gets assigned a value.
  4. Keep reading this issue and I'll explain why that happens. 😄

Snippet:

myLib.js -- a package that contains a named object myLib, and registers object methods with p5

const myLib = {
  myLibFunction: (...args) => {
    console.log("called myLibFunction with arguments", ...args);
    return new Promise((resolve) => {
      setTimeout(() => {
        window._decrementPreload();
        console.log("myLibFunction complete");
        resolve();
      }, 2000);
    })
  }
}

window.p5.prototype.registerPreloadMethod('myLibFunction', myLib);

sketch.js -- a p5 sketch that calls the preload-registered function myLib.myLibFunction() in setup() rather than in preload()

function preload() {
  // will be: false, undefined
  console.log("is myLibFunction on window?", !!window.myLibFunction, window.myLibFunction);
}

function setup() {
  noLoop();
  createCanvas(400, 400);
  // call it twice, just for example.
  myLib.myLibFunction().then(myLib.myLibFunction).then(redraw);
  console.log("preload count in setup()", window._preloadCount);
  // will be: true, function
  console.log("is myLibFunction on window?", !!window.myLibFunction, window.myLibFunction);
}

function draw() {
  console.log("preload count in draw()", window._preloadCount);
  background(220);
}

complete example on p5 web editor

Issue:

I've been trying to fix some bugs with registering preloads in ml5.js, which is a p5.js library. It turns out that one of the issues that we are having is actually a bug in p5 itself! I dug pretty deep into the source code to figure things out so I can explain exactly what the problem is.

The issue occurs when calling registerPreloadMethod with a property of a custom object (ie. the library) rather than with p5 as the source object. When a custom object is used, the method gets wrapped in a function that calls _incrementPreload, but it never gets unwrapped. Things will work ok if the method is called in preload(). But if it is called in setup() then it will increment the _preloadCount up to 1 when the function starts and decrement it back to 0 when the function resolves, which then triggers p5 calling setup() again...and again...in an infinite loop.

The looped function chain is myLib.myLibFunction => _decrementPreload => _runIfPreloadsAreDone => _setup() => myLib.myLibFunction.

Here are the relevant lines of code:

obj[method] = this._wrapPreload(obj, method);

obj here is the myLib object. So this code is setting myLib.myLibFunction to the version which calls _incrementPreload. That's all good, until:

p5.js/src/core/main.js

Lines 325 to 334 in 196d3af

// return preload functions to their normal vals if switched by preload
const context = this._isGlobal ? window : this;
if (typeof context.preload === 'function') {
for (const f in this._preloadMethods) {
context[f] = this._preloadMethods[f][f];
if (context[f] && this) {
context[f] = context[f].bind(this);
}
}
}

The code to reset the function back to its previous version ignores obj as the source.

  • context is the context of p5 which is the window
  • f is the name of the method 'myLibFunction'
  • this._preloadMethods is a dictionary of method names to source objects, so:
  • this._preloadMethods[f] is myLib
  • this._preloadMethods[f][f] is myLib.myLibFunction

In short, the assignment on line 329 is equivalent to window.myLibFunction = myLib.myLibFunction. Its sets myLibFunction on the window, even though it wasn't a global variable before. It does not set myLib.myLibFunction back to its original version.

Fix?

The "unwrap" code block needs to be modified to support the case where the method is not attached to p5.

There needs to be a reference to the original version of the method. I believe this._preloadMethods[f] would point to the same object instance as myLib so it would contain the overwritten function. It seems like this._registeredPreloadMethods holds the necessary reference to the original, based on:

p5.js/src/core/main.js

Lines 266 to 267 in 196d3af

this._registeredPreloadMethods[method] = obj[method];
obj[method] = this._wrapPreload(obj, method);

Edit: it looks like this has been a known issue since 2015!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants