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

Supporting p5.js preload #29

Closed
ziyuan-linn opened this issue Aug 8, 2023 · 12 comments · Fixed by #81
Closed

Supporting p5.js preload #29

ziyuan-linn opened this issue Aug 8, 2023 · 12 comments · Fixed by #81

Comments

@ziyuan-linn
Copy link
Member

@shiffman Hi everyone, I am starting this discussion because I just realized that initialization functions like ml5.handpose() need to be hooked into p5's preload() function.

@gohai's mentioned here that the old p5PreloadHelper does not seem to be working and instead called _incrementPreload() and _decrementPreload(). I am not really familiar with p5's implementation, so I am not sure how preload() works under the hood. Would it make sense for us to update the p5PreloadHelper so we could easily hook into the preload function?

@ziyuan-linn
Copy link
Member Author

Hi @limzykenneth! I am currently working on a new API for some of the ml5 models and would like to get some insight from you about the p5 preload function.

We are trying to make the model initialization functions compatible with p5's preload() functions. For example:

let handpose, img;

function preload() {
  handpose = ml5.handpose(); //ml5 handpose model initialization function
  img = loadImage("file.jpg");
}

function setup() {
  handpose.detect(img, gotHands);
}

gotHands() {
  ...
}

I am not very familiar with how p5's preload() works under the hood, but our current implementation calls window._incrementPreload() and window._decrementPreload() to achieve this. We also have a helper code for p5 preload from a few years ago, but it was never used and does not seem to work.

Would calling window._incrementPreload() and window._decrementPreload() at the beginning and end of the async load process be the best way to support preload()? Or would there be a more elegant way to do it? Thank you in advance for your help!

@limzykenneth
Copy link

This part of the API is not really very well documented so I'm mostly going by looking at the source and what I've tested. For the most part it seems _incrementPreload() and _decrementPreload() are internal methods and registerPreloadMethod is meant to be the canonical way to handle these, although I'm not finding it easy to use nor consistent.

It should be fine to use _incrementPreload() and _decrementPreload() for the most part but I don't suggest using window._incrementPreload() and window._decrementPreload() as these will only work if the user is using global mode. You will probably need a wrapper that attaches to p5.prototype that calls this._incrementPreload() and this._decrementPreload() instead. It would be best if you can contain as much p5.js specific implementation around this in the wrapper as possible and use more modern API internally (ie. async/await) yourself. I'm thinking around a future implementation of p5.js (v2 for example) that overhaul this to be easier and probably using promises as well so insulating yourself with some adjustable wrapper code can save trouble in the future.

@ziyuan-linn
Copy link
Member Author

Thank you so much for the information! A wrapper that contains all the p5-related code definitely sounds like a good idea. Also looking forward to the future implementations of p5.js!

@shiffman
Copy link
Member

Thank you so much @limzykenneth for the advice and support!

@lindapaiste
Copy link
Contributor

lindapaiste commented Dec 5, 2023

@limzykenneth I've tried working on this part of the ml5 code before and I found that it's actually impossible to implement properly without some changes on the p5 side. The p5 code only works properly with methods that are bound to the p5 prototype. See processing/p5.js#5677 and processing/p5.js#815. The result is a long-standing bug which prevents using ml5 functions with Promise.then() syntax when using p5.

We are calling registerPreloadMethod but it seems like we're also supposed to call _decrementPreload() within the preloaded function to tell p5 when the loading is done. That's a weird setup since it involves calling a private p5 function. It would be nice if we could just return a promise and resolve it when done, or if calling registerPreloadMethod returned some sort of done function that we call when done.

@limzykenneth
Copy link

Yes, I have recently gone through the documentation to try things out and as you identified, _decrementPreload() need to be called manually (updates to the documentation will come probably later this month or early next year as part of the website working going on).

I would like preload to be promised based as well and is looking into it, although it probably will only be released as part of p5.js 2.0.

@lindapaiste
Copy link
Contributor

Part of the reason that the current code is complicated and buggy is that we are trying to support two different usage patterns at once.

Hi @limzykenneth! I am currently working on a new API for some of the ml5 models and would like to get some insight from you about the p5 preload function.

We are trying to make the model initialization functions compatible with p5's preload() functions. For example:

let handpose, img;

function preload() {
  handpose = ml5.handpose(); //ml5 handpose model initialization function
  img = loadImage("file.jpg");
}

function setup() {
  handpose.detect(img, gotHands);
}

gotHands() {
  ...
}

In the p5 use-case that you are describing, calling ml5.handPose(); returns a HandPose object. The function itself is a sync function so it does not return a Promise. The returned HandPose instance is not fully loaded at first, and we would use the _decrementPreload() stuff to indicate to p5 when the loading is complete. But the handpose variable is always the same HandPose instance.

We get into trouble when we also want to support this sort of setup, for use in a vanilla JS environment. (This example is terrible because we load the model in draw() but I won't get into that).

ml5.handPose()
  .then(model => {
    // Show 'Model Loaded!' message
    statusMsg.html('Model Loaded!');

    // Call transfer function after the model is loaded
    transfer(model);

    // Attach a mousePressed event to the button
    transferBtn.mousePressed(function() {
      transfer(model);
    });
  });

Or this

const loadedModel = await ml5.handPose();

In both of these examples, we are expecting that calling ml5.handPose() returns a Promise which resolves to a HandPose instance.

The current behavior, shown below, is that we return the instance directly when the arguments of handPose include a callback function. When there is no callback we return a Promise which resolves to the instance (instance.ready). And then we need the p5PreloadHelper to override this behavior so that we return the instance when calling handPose() inside of the p5 preload() function even when there is no callback.

const handpose = (...inputs) => {
  const { video, options = {}, callback } = handleArguments(...inputs);
  const instance = new Handpose(video, options, callback);
  return callback ? instance : instance.ready;
};

It seems like the next-gen version of the library has decided to drop this switching behavior, which means that you can no longer do ml5.handPose().then((model) => { }); or const loadedModel = await ml5.handPose();. TBH that's fine, so long as we have a clear and documented way to wait for the model to load for people who want to await it.

I am not very familiar with how p5's preload() works under the hood, but our current implementation calls window._incrementPreload() and window._decrementPreload() to achieve this. We also have a helper code for p5 preload from a few years ago, but it was never used and does not seem to work.

Actually it is used! We are passing the models that we want to preload in the index.js.

Would calling window._incrementPreload() and window._decrementPreload() at the beginning and end of the async load process be the best way to support preload()? Or would there be a more elegant way to do it? Thank you in advance for your help!

Due to the long-standing problems on the p5 side, I think that directly calling _incrementPreload() and _decrementPreload() is going to work a lot better than using registerPreloadMethod. That's what you are doing in the new next-gen models, though we definitely want to move parts of that logic into some sort of utility, and also make sure that it works when using p5 in instance mode.

Is there any desire to keep the current behavior of supporting both async and callback usage? I might be able to figure something out if there is.

@limzykenneth
Copy link

Just to chime in for p5.js side, I have reviewed the library authoring side of things relatively recently and calling _incrementPreload() and _decrementPreload() is likely to be the more stable approach (in any case _decrementPreload() should be called manually by library authors).

Moving forward, as part of investigation into potentiall p5.js 2.0, it is proposed that loading will use async/await in an async setup function in the future, meaning that if the proposal is accepted, promise based loading with async/await syntax will be the recommendation for library authors and users. As long as the function returns a promise, there won't be need of using additional API from p5.js.

The timeline towards p5.js 2.0 is not fixed though and we are looking a relatively slow progress because of team capacity. That being said, feel free to create addional proposals if anyone here wants to see some potentially larger or breaking changes that can be introduced in p5.js 2.0, both in terms of library author and user perspectives.

@gohai
Copy link
Member

gohai commented Feb 28, 2024

@lindapaiste Perhaps a new preload helper could use _incrementPreload() and _decrementPreload() but address @limzykenneth earlier comment that we shouldn't be calling this on the window object:

It should be fine to use _incrementPreload() and _decrementPreload() for the most part but I don't suggest using window._incrementPreload() and window._decrementPreload() as these will only work if the user is using global mode. You will probably need a wrapper that attaches to p5.prototype that calls this._incrementPreload() and this._decrementPreload() instead.

Perhaps #69 is also worth considering at this point: @ziyuan-linn found that we run into an issue when trying to load two mediapipe models simultaneously. Some synchronization mechanisms might be needed for the best composability.

@shiffman
Copy link
Member

Chiming in here to speak to this question, feedback and thoughts welcome!

Is there any desire to keep the current behavior of supporting both async and callback usage? I might be able to figure something out if there is.

The new version of ml5.js is prioritizing p5.js users and scaling down to have a simpler API with fewer models, focused on beginners to creative coding and machine learning. So our examples and documentation will follow p5.js development and focus on callbacks along with preload support. It would be "nice to have" if behind the scenes promises were also supported (I'd use them in some contexts!), but it would also be ok to wait until promises are supported more natively in p5.js 2.0 if that is not an easy thing to support.

I expect (and hope!) there will be continued use of ml5.js outside of p5.js, but in our research (shoutout to @MOQN and @QuinnHe!) we discovered that the native JS examples and documentation caused a lot of confusion for beginners to library. And given how much extra work it is to maintain and the different flavors and styles of use, I think we can focus on p5.js callbacks + preload!

@lindapaiste
Copy link
Contributor

lindapaiste commented Mar 1, 2024

It should be fine to use _incrementPreload() and _decrementPreload() for the most part but I don't suggest using window._incrementPreload() and window._decrementPreload() as these will only work if the user is using global mode. You will probably need a wrapper that attaches to p5.prototype that calls this._incrementPreload() and this._decrementPreload() instead.

@limzykenneth I've been looking into this and I'm not sure if it's even possible for use to get the right this (the right p5 instance) when using instance mode, especially if there are multiple sketches on the page. The example in the docs is able to call this._decrementPreload() because the function is bound to p5.prototype. We aren't attaching the ml5 functions to p5. prototype. The user would be calling ml5.handpose() etc.

I've been playing around with this a lot, and I am able to get preloading to work if I put the method on p5.prototype. Here's my code for trying to preload a bare-bones callbackLoadedObject method from a fake myLib library that's akin to ml5.

p5.prototype.callbackLoadedObject = function(userCallback) {
  this._incrementPreload();
  const decrement = this._decrementPreload;
  const wrappedOnDone = (result) => {
    userCallback?.(result);
    this._decrementPreload();
  }
  return myLib.callbackLoadedObject(wrappedOnDone);
}

Putting a wrapped version of the library method on the p5.prototype works correctly in instance mode if I call the function from the p5 object. That is,

let sketch = function (p) {
  var obj = {};

  p.preload = () => {
    obj = p.callbackLoadedObject();
  };
...

I cannot figure out a way to support calling obj = myLib.callbackLoadedObject(); (calling ml5.handPose() etc.). I tried assigning the p5-bound method back to the object, myLib.callbackLoadedObject = p5.prototype.callbackLoadedObject;, but that doesn't work because it loses the this and gives an error TypeError: this._incrementPreload is not a function.

I'm feeling like I've exhausted this approach and the next thing for me to play with are the registerMethod hooks.

edit: OH MY GOD I AM FINALLY GETTING SOMEWHERE!!!! You have no idea how many things I tried. An explicit .bind(this) seems to help a lot. This approach is showing promise:

const myLibOriginal = { ...myLib };

p5.prototype.myLibInit = function () {
  const increment = this._incrementPreload.bind(this);
  const decrement = this._decrementPreload.bind(this);
  myLib.callbackLoadedObject = function (...args) {
    increment();
    // TODO: handle multiple arguments
    const wrappedOnDone = (result) => {
      args[0]?.(result);
      decrement();
    };
    return myLibOriginal.callbackLoadedObject(wrappedOnDone);
  };
};

p5.prototype.registerMethod("init", p5.prototype.myLibInit);

@limzykenneth
Copy link

@lindapaiste The investigations you've done are all super helpful. I will look into this from what you have found and seeing as p5.js 2.0 is likely awhile away, I will incorporate fixes around this system if necessary to get preload working with libraries that does not attach to the p5 prototype directly.

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

Successfully merging a pull request may close this issue.

5 participants