depthai-core icon indicating copy to clipboard operation
depthai-core copied to clipboard

Refactor asset support for Keembay

Open moratom opened this issue 3 years ago • 3 comments

Refactoring done as discussed with @themarpe over Slack.

moratom avatar Aug 03 '22 23:08 moratom

On first glance the following would error out:

  • Create NN1, set blob (blob A)
  • Create NN2, set blob (blob B)
  • loadResource from NN2 -> returns NN1's blob A.

Yes, it would.

That said, the current mechanism might not be best suited for such refactor. Check the following means:

  • each Node's asset manager gets equipped with its own "root path" (eg on node creation, etc...)
  • assetManager.set("__blob") then saves the path as eg: NN2: /node/2/__blob

That can be done, yes.

I just want to clarify what the desired behavior of loadResource is? It would make most sense to me, that loadResource only checks for assets in the assetManager of the node where it is called from. So if you expect something to be in the pipeline, you can explicitly call it like parent.lock()->loadResource("resource"). Otherwise, if you silently "fall back" on checking the pipeline assetManager, you might get some hard to debug bugs later. Thoughts?

moratom avatar Aug 05 '22 08:08 moratom

@moratom The desired behavior of loadResource is that it find an asset in the "unified" space. Eg Pipeline asset manager has an asset A, and node 1 has asset B.

Those should be accessible from anywhere by: /pipeline/A or /node/1/B, but also accessible from "current working directory" of Pipeline as such: A and from node 1 as: B (where the cwd in former is /pipeline and in latter /node/1)

Otherwise, if you silently "fall back" on checking the pipeline assetManager, you might get some hard to debug bugs later.

Thats why the managers are prefixed - all are under unique paths. There shouldn't be no clashes.

Another option is having just as single Pipeline asset manager, but I wouldn't change it now (was reworked in 3d735e0647bda393602c3f24418fd982fad5968f, but not from the exact idea mentioned before)

themarpe avatar Aug 05 '22 13:08 themarpe

The new implementation is in faf09b3 commit.

The loadResource function now works as you described the desired behavior in the comment above, while the assetManager only extends the previous interface so the current examples should still work fine. Let me know what you think @themarpe.

moratom avatar Aug 07 '22 23:08 moratom