registerPreloadMethod failing on various types of objects

[dhowe]

A question: let's say I have a p5-compatible library (MyLib) with an object literal in it (MyLibObj), and this object literal has a function that the user would normally call as MyLibObj.loadObj(). Is there a nice way to register this function for preloading, without redefining it in some other scope?

[lmccart]

hey @dhowe does registerPreloadFunc do the trick? it was recently updated so you can specify both the name of the method and the object that method is attached to. https://github.com/processing/p5.js/wiki/libraries#use-registerpreloadmethod-to-register-names-of-methods-with-p5-that-may-be-called-in-preload

[dhowe]

No. I tried a variety of versions of that call before writing the ticket, but no luck..

 p5.prototype.registerPreloadMethod('MyLibObj.loadObj');
 p5.prototype.registerPreloadMethod('loadObj', 'MyLibObj');
 p5.prototype.registerPreloadMethod('MyLibObj.loadObj', 'MyLibObj');
 etc.

[dhowe]

The relevant lines are here in core.js (line 269), where 'f' is the first argument above, and 'o' the 2nd. Not positive, but I think the issue is due to the fact that the function in question is not attached to the object's prototype (it is a singleton object literal), so window['MyLibObj'].prototype['loadObj'] is undefined, Rather, what we want to be able to register is window['MyLibObj']['loadObj'].

  if (typeof context.preload === 'function') {
      for (var f in this._preloadMethods) {
        var o = this._preloadMethods[f];
        context[f] = window[o].prototype[f];
      }
    }

[lmccart]

aha ok. I don't think we actually need the prototype in there... do you want to test the newest I just pushed?

[dhowe]

I'm thinking about the various use-cases. But first off, is it expected that an empty preload() function will hang the sketch (with setup() never being called)?

[dhowe]

Types of things we might want to register as preload functions (as best I can tell only the first case currently works):

• a function attached to p5 (e.g., p5.sound lib)

p5.prototype.registerPreloadMethod('loadSound'); 
OR
p5.prototype.registerPreloadMethod('loadSound', 'p5'); 
OR
p5.prototype.registerPreloadMethod('loadSound', p5); // ?

• a globally defined function

window.myFunc = function(a,b) {...};
p5.prototype.registerPreloadMethod('myFunc', window);
OR
p5.prototype.registerPreloadMethod('myFunc'); // prob better

• a function on an object literal

var myObject = {
  myPreloadFunction: function(a,b,) {...}
};
p5.prototype.registerPreloadMethod('myPreloadFunction', myObject);

• a function on an instantiated object (with prototype)

function MyObject() {...}
MyObject.prototype.myPreloadFunction = function(a,b,) {...}

var obj = new MyObject();

p5.prototype.registerPreloadMethod('myPreloadFunction', obj);

[lmccart]

Looking at this again now. I've just pushed a change that takes an object rather than a string as the second argument. However, I don't think this yet supports all cases outlined above. The problematic part is this line where we set the variable context to either p5 or window (to account for instance or global mode). However, there might be cases where the function called is not a top level one but something inside another object, in which case the context would need to be set to this. We are coming up against this in trying to get the tone.js addon working. Will look into it further.

[polyrhythmatic]

I've been taking a crack at this and I've gotten somewhere although I'm not sure where. I'm trying to register the 4th case @dhowe outlined - registering instantiated objects.

in the start function i've added an if statement - if the function is an internal object it registers it off the window directly, otherwise it will look within Tone.

    if(typeof methods[f]._preloadMethods == "object"){
            context[f] = function() {
              var argsArray = Array.prototype.slice.call(arguments);
              return context._preload(f, methods[f], argsArray);
            };
        } else {
            //adding a check to see if it is not an internal
            //p5 function
            context["Tone"][f] = function() {
              var argsArray = Array.prototype.slice.call(arguments);
              return context._preload(f, methods[f], argsArray);
            }
        }

I've finally got a Tone.Player method registering with the preload (which wasn't working for the longest time) but I think it may be registering the constructor function rather than something else, because it now calls itself recursively until it exceeds stack size.

I'm going to see if there is a way to sniff out constructors and register them separately, because it would be great to be able to register constructors in the preload. Here is the script in the main page:

                var player;

        function preload(){
            player = new Tone.Player("./ohh.mp3").toMaster();
        }

        function setup() {
            player.start();
        }


        function draw() {
        }

[polyrhythmatic]

I've made a pull request that might be a solution to @dhowe's problem. As we integrate more libraries, it may make sense to consider adding some options to the registration API such as the ability to increment/decrement the preload count.

[indefinit]

This might be unrelated to the recent merge, but I noticed that setup is no longer called after preload even if I do something simple inside preload like console.log. Instead I get the "loading..." screen.

When I run the code here: http://p5js.org/reference/#p5/preload I see the loading screen.

Can anyone else confirm this issue? or is it just me?

[polyrhythmatic]

@indefinit I'm getting this error as well, but it never showed up when working on the changes for p5.Tone. I checked out a previous branch and noticed it was still an issue. Looking at it quickly, it looks like some sort of issue with a success callback.

[indefinit]

@polyrhythmatic looks like the bug is here:

p5.js/src/core/core.js

Lines 231 to 238 in effc454

	Object.keys(methods).forEach(function(f) {
	var methodContext = methods[f];
	var originalFunc = methodContext[f];
	methodContext[f] = function() {
	var argsArray = Array.prototype.slice.call(arguments);
	return context._preload(originalFunc, methodContext, argsArray);
	};
	});

Changing those lines back to:

Object.keys(methods).forEach(function(f) {
        context[f] = function() {
          var argsArray = Array.prototype.slice.call(arguments);
          return context._preload(f, methods[f], argsArray);
        };
      });

and tweaking the _preload return statement here (

p5.js/src/core/core.js

Line 264 in effc454

return func.apply(obj, args);

) like this:

return obj[func].apply(context, args);

fixes it for me, since otherwise methodContext[f] is out of scope later on. I can make the change and issue a PR, but does it break your other preload methods fix? I can't seem to find any tests for the method preload registration.

[lmccart]

If I'm understanding correctly, this essentially undoes the previous PR #861. I think it's helpful to revert this to get preload working right now in general, but @polyrhythmatic what is your suggestion on how to proceed?

[indefinit]

Been thinking about the issue some more and I remember someone mentioning promises in another Issue. I wonder whether it makes more sense to take a promises-based approach to the preload function altogether. I've personally always really liked how jquery handles $.deferred and when(), like this: https://api.jquery.com/jquery.when/

Sent from my mobile.

On Aug 21, 2015, at 6:31 AM, Lauren McCarthy [email protected] wrote:

If I'm understanding correctly, this essentially undoes the previous PR #861. I think it's helpful to revert this until to get preload working right now in general, but @polyrhythmatic what is your suggestion on how to proceed?

—
Reply to this email directly or view it on GitHub.

[polyrhythmatic]

@indefinit @lmccart sorry guys! I was wrong! I met with @tambien today and he straightened it out #866 . Yotam also refactored the code a bit so its more readable. The big problem was that the original code looked on the window for all objects, regardless of their original library

      Object.keys(methods).forEach(function(f) {
        context[f] = function() {
          var argsArray = Array.prototype.slice.call(arguments);
          return context._preload(f, methods[f], argsArray);
        };
      });

Yotam added this statement to account for that

      for (var method in this._preloadMethods){
        var obj = this._preloadMethods[method];
        //it's p5, check if it's global or instance
        if (obj === p5.prototype){
          obj = this._isGlobal ? window : this;
        }
        this._registeredPreloadMethods[method] = obj[method];
        obj[method] = this._wrapPreload(obj, method);
      }

He also added an object to keep track of the original function. This prevents the wrapped function from continuously recalling itself.

This can later be returned from the wrapper function

  this._wrapPreload = function(obj, fnName){
    return function(){
      //increment counter
      this._incrementPreload();
      //call original function
      var args = Array.prototype.slice.call(arguments);
      args.push(this._decrementPreload.bind(this));
      return this._registeredPreloadMethods[fnName].apply(obj, args);
    }.bind(this);
  };

It passed all tests, works with p5.Tone, and works with the async instance and global examples. Please let me know if you find any problems with it! I'd also like to test compliance with other libraries. Can anyone think of any that would be good to test?

[dhowe]

Do we have an example that works with a different object than p5.prototype? Something like:

p5.prototype.registerPreloadMethod('myPreloadFunction', myLibraryObject);

[lmccart]

I don't think we have an example posted somewhere but it should work. @polyrhythmatic does the p5.js-tone.js code include this, and is it posted somewhere?

[dhowe]

yes, I also looked for p5.tone, but didn't find any source with a registerPreload() function...

[dhowe]

I think we also need to update the docs here: https://github.com/processing/p5.js/wiki/Libraries

[dhowe]

So, I think the problem was that the function I was testing returned a string... and as best I can tell (due to the immutability of js strings) there's no way to make this work in preload(), at least not without changing the function and wrapping the result in some other object... (if so, we may want to document this for library authors?)

[dhowe]

Any thoughts @lmccart @polyrhythmatic ?
Here's a test, in case anyone wants to give it a try (the same code works correctly when result is either an array, or an object with a string property):

<html>
<head>
  <script src="https://cdnjs.cloudflare.com/ajax/libs/p5.js/0.4.17/p5.js"></script>
  <script>

    // a simple library-like object
    var myLib = {
      loadTest: function(callback) {

        p5.prototype.registerPreloadMethod('loadTest', myLib);

        var result = 'nope';

        setTimeout(function () {

          result = 'got it';
          console.log('done');
          callback(result);

        }, 2000); // simulate a slow load

        return result;
      }
    };

    // register the library function for preload
    p5.prototype.registerPreloadMethod('loadTest', myLib);

    var data;

    function preload() {
      data = myLib.loadTest();  // do the preload function
    }

    function setup() {
      createCanvas(500, 500);
      text(data, 20, 20); // expecting 'got it'
    }

  </script>
</head>
</html>

[lmccart]

indeed, this seems to be the issue. for example, if I change the code to an object it works:

    // a simple library-like object
    var myLib = {
      loadTest: function(callback) {

        //p5.prototype.registerPreloadMethod('loadTest', myLib);

        var result = {text: 'nope'};

        setTimeout(function () {

          result.text = 'got it';
          console.log('done');
          callback(result);

        }, 2000); // simulate a slow load

        return result;
      }
    };

    // register the library function for preload
    p5.prototype.registerPreloadMethod('loadTest', myLib);

    var data;

    function preload() {
      data = myLib.loadTest();  // do the preload function
    }

    function setup() {
      createCanvas(500, 500);
      text(data.text, 20, 20); // expecting 'got it'
    }

I'll add this to the libraries wiki page.

[dhowe]

I wonder if there is a workaround (I haven't come up with one as yet) -- its not ideal to require libraries to potentially change their APIs just to integrate with p5js...

[dhowe]

One other thing I notice is that external code will fail in preload() when a callback (for example, an error-handler) is passed as an argument -- which is different than the behavior of p5js' loadXXX functions. This is due to the absence of the following in the external code:

    // these 3 lines are repeated in each of our loadXXX functions
    var decrementPreload = p5._getDecrementPreload.apply(this, arguments);
    if (decrementPreload && (successCallback !== decrementPreload)) {
      decrementPreload();
    }

Not necessarily a problem, but a) we should document this, and b) library authors might be tempted to use p5._getDecrementPreload() to achieve the same effect, so we may want to remove the underscore and properly document it as a public function ?

Though my sense is there may be a more elegant solution here... @polyrhythmatic ?

[dhowe]

Here is another use case that appears to fail with our current preload() mechanism (this is actually the 4th case listed above on Jul29).

Imagine we have a library with a class inside, and that class has a function we want to be able to preload, something like this:

    // a library-like object
    var MyLib = {

      // a class in the library
      MyLibObject : function(type) {

        // a test load function in the class
        this.loadSumthin = function(callback) {

          var result = { text: 'nope' };

          setTimeout(function ()  {
            result.text = 'got it';
            if (callback)
              callback(result);
            else
              console.log("NO CALLBACK");

          }, 1000); // simulate a slow load

          return result;
        };
      }
    }

The fake library function works as expected when passed a callback, as in the following sketch:

    var data, libObj;

    function setup() {
      createCanvas(200, 200);
      libObj = new MyLib.MyLibObject()
      data = libObj.loadSumthin(function()  {
          text(data.text, 20, 20); // expecting 'got it'
      });
    }

However I cannot get it to work with any preload syntax. Here is one attempt:

    p5.prototype.registerPreloadMethod('loadSumthin', MyLib.MyLibObject.prototype);

    var data, libObj;

    function preload() {
      libObj = new MyLib.MyLibObject()
      data = libObj.loadSumthin();
    }

    function setup()  {
      createCanvas(500, 500);
      text(data.text, 20, 20); // expecting 'got it', but fails
    }

In another version I instead register the preload function in the object's constructor:

    p5.prototype.registerPreloadMethod('loadSumthin', this);

but still no luck. Any thoughts on a solution (seems a common case for external libraries)?

[dhowe]

Any thoughts @lmccart @polyrhythmatic ?

[lmccart]

oops sorry i missed this latest post.. I'll look into it this week.

[dhowe]

👍

[nbriz]

hi there! just wondering if there's been any progress on this? is there a recommended way of registering these preload methods? I've been using p5.js in class but also writing tiny helpers/libraries for specific projects && would be rad to have access to preload :)

[lmccart]

the information on how to register preload methods is here:
https://github.com/processing/p5.js/wiki/Libraries

unfortunately there's been no progress on dealing with other sorts of objects as discussed above yet.

[nbriz]

thnx @lmccart that worked perfectly :)

[Zalastax]

Looking at registerPreloadMethod it seems to me that its main feature is that the p5 instance is bound to every registered method during preload(the registry is shared among all instances). It also calls _incrementPreload but the methods themselves have to call _decrementPreload so they might as well call _incrementPreload as well. Doing so should, as far as I see, make _wrapPreload do no useful job at all when the registered object is p5 and we could just call the methods directly instead.

Looking at

p5.js/src/core/main.js

Line 268 in a64959e

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

we currently have a race condition if two p5 instances preload at the same time - the second preload will overwrite the methods of obj.
With that in mind I propose that we should not bind p5 to obj at all. Instead we can provide two events beforePreload and afterPreload to which you can register callbacks (instance: p5) => void. That would distribute the p5 instance without enforcing a particular way to bind p5. Alternatively, library writers could just force their users to manually provide the p5 instance when calling custom load functions since I believe race conditions might be unavoidable otherwise.

TL;DR: Make load functions also call _incrementPreload, make _incrementPreload and _decrementPreload available for public use, stop binding p5 during preload, and possibly add two events beforePreload and afterPreload.

[mikima]

Hello,
I'm trying to fix the p5.geolocate library. At the moment, if used with p5 0.7.2 it throws an error.

The error was related to the prototype.registerPreloadMethod, this is the reason why i'm writing here.

Looking to the documentation it seems that prototype.registerPreloadMethod requires the name of the function and the prototype object. I've added it but now if i use the function in the preload it get stuck preloading. It's not clear to me if it is needed to do something else in order to make it working.

Below the function:

p5.prototype.getCurrentPosition = function(callback, errorCallback) {

  var ret = {};

  if (navigator.geolocation) {
    navigator.geolocation.getCurrentPosition(success, geoError);
  }else{
    geoError("geolocation not available");
  };

    function geoError(message){
      console.log(message.message);
      ret.error = message.message;
       if(typeof errorCallback == 'function'){ errorCallback(message.message) };
    }

    function success(position){
          for(var x in position.coords){
            ret[x] = position.coords[x];
          }

      if(typeof callback == 'function'){ callback(position.coords) };
    }

    return ret;
};


p5.prototype.registerPreloadMethod('getCurrentPosition', p5.prototype);

Can you point me in the right direction?