brianzinn / react-babylonjs

React for Babylon 3D engine
https://brianzinn.github.io/react-babylonjs/
817 stars 105 forks source link

useLoader vs Model #87

Closed dennemark closed 3 months ago

dennemark commented 4 years ago

Hi,

I was wondering if there are great differences between useLoader or Model, except for Model being declarative and useLoader a nice hook?

I found one answer https://github.com/brianzinn/react-babylonjs/issues/66#issuecomment-638372895

The advantage of the above is that we can hopefully assign materials and textures. I added also in this beta support for assigning Textures to models ie: <texture assignTo={'meshes[1].material.albedoTexture'} url={url} />, so some progress in that direction as well to provide better declarative access to your 3D models.

This seems to be a good addition. I was doing through onModelLoaded via declarative Model before and going through those meshes one by one via model.rootMesh.getChildMeshes()

Model might have the advantage over useLoader to declaratively assign attributes to the loaded Mesh right?

brianzinn commented 4 years ago

hi @dennemark . I only have a rather long answer, because I'm not satisfied with the current state and see it as nearly the most useful part of the project and deserves the most attention currently.

With the recent addition of fromInstance ie:

<mesh fromInstance={mesh} />

I think with that we should be able to get the declarative aspect from the hook. Would be something like:

const MyModel = (props) => {
   const [loaded, meshes] = useLoader(props.url, ..);
   if (loaded === false) {
        return null;
   }
   return <mesh fromInstance={meshes[0]} position={props.position} />
}

To make that syntax cleaner the useLoader needs to support Suspense and then I hope the syntax would look more like:

const { meshes, animation, etc. } = useLoader(...);
return <mesh fromInstance={meshes[0]} ... >
               <standardMaterial fromInstance={meshes[0].material} diffuseColor={props.diffuse} />
           </mesh>

Then that loader could be used in the <model ../> and the model children work like it does now Model already supports syntax like this, but this doesn't work with the hook - it works with the model because of a custom Lifecycle management built into the renderer:

<model ...>
    <texture assignTo={'meshes[1].material.albedoTexture'} .... />
</model>

Adding Suspense will enable a nice fallback loader. I also am thinking about how to do progress. I think the progress will be not reported or available to the Suspense fallback only a promise is returned (I think we can use context, but then the syntax would be clunky with declaring providers, etc.), so we are unable to show a GUI with % loaded or a 3D progress loader in an easy to declare way. Also, to be mindful that reporting progress may cause undesired renders, so would be opt-in like it is now.

<Suspense fallback={<MyProgress percent={  👎  } />}>
  <MyModel />
</Suspense>

I really haven't had any time at all to work on it though unfortunately. The end result will be a major release bump, since it will be a big improvement and breaking changes. The other thing to consider is some people may want to use AssetContainer, so perhaps a way to declare that on the useLoader hook itself or a separate hook like useAssetContainer. The disadvantage currently with using the regular loader is that once it is loaded if you want to scale it or move it there is a flash of render before scaling or position changes are taken into account.

Anyway, so I don't know if you were asking more about future direction or comparing what is there right now, but hopefully the two ways can be unified. I'm still waiting for Suspense to come out of experimental and it's been taking ages. Anyway at least that puts most of my thoughts down. Cheers.

drcmda commented 3 years ago

@brianzinn you might want to take a look at use-asset: https://github.com/pmndrs/use-asset this is what i use for r3f. you dont need anything else to have suspense. just a promise or a callback and you wrap it into useLoader, which packs it into use-asset. it will cache it, too, as well as allowing your users to preload and bust cache. i would be glad if more libs are basing on use-asset, for battletesting and all.

r3f's useLoader: https://github.com/pmndrs/react-three-fiber/blob/master/src/hooks.ts#L108-L118

with suspense you don't need loading state, isLoaded, checking results, etc. next line underneath useLoader the result is guaranteed to exist, otherwise it must throw (-> error boundaries). as for percentage, you get that from the xhr events.

suspense is not experimental btw. it is a react stable feature. the experimental part is about strategies for caching (react-cache) and so on.

brianzinn commented 3 years ago

Firstly thank-you @drcmda for your suggestions - I am a big admirer of your work on r3f and react-spring. I did add some support recently for Suspense using the built-in AssetManager for babylonjs. There is a point cloud demo (https://brianzinn.github.io/react-babylonjs/?path=/story/hooks--use-asset-manager). Also, thank-you for clearing up the experimental.

It was easy to add progress support and I would easily be able to tie that into use-asset as-is (demo in use-asset-manager-v2 branch, if interested - I just used context).

I would be really keen to add use-asset and add to the battletesting (ironically I worked some time for NATO). There is one thing missing right now that may prevent me from going ahead and it's related to how assets are created and cached. I want a single promise to contain multiple cache keys. I have needed this capability also on a couple of projects I did, so maybe you want it added.

I don't know if these needs would be considered, since it would change how caching works in use-asset. As you know lots of game libraries have loaders (ie: PIXI.Loader, BabylonJS.AssetManager, etc.) - they provide a built-in way to load assets and one thing that I want to add is caching at hook level to support that use-case. I am loading up AssetManager with an array of tasks and sending them through, if 3/4 of them have previously been loaded then I would only want to send through the fourth. In another simpler example, I wrote an API wrapper that memoized request/response to reduce API calls via cache - here is a simple string (UPC)->number (quantity) example:

export type CheckUpcFn = (upcs: string[]) => Promise<Record<string, number>>;

export const checkInventoryByUPC = (): CheckUpcFn => {
    const UPC_CACHE: Record<string, number> = {};
    const mergeResponseWithCache = (upcs: string[], apiResponse: T): Record<string, number> => {
        const upcsFromResponse: Record<string, number> = {};
        if (apiResponse !== undefined) {
            apiResponse.inventoryLevels.forEach(response => {        
                const upc = response.upc;
                upcsFromResponse[upc] = response.quantity;
                UPC_CACHE[upc] = response.quantity;
            })
        }

        return upcs.reduce((previous: Record<string, number>, current: string) => {
            if (previous[current] === undefined && UPC_CACHE[current] !== undefined) {
                previous[current] = UPC_CACHE[current];
            }
            return previous;
        }, upcsFromResponse)
    }

    return async (upcs: string[]): Promise<Record<string, number>> => {
        const notCachedUPCs: string[] = upcs.filter(upc => UPC_CACHE[upc] === undefined);

        if (notCachedUPCs.length > 0) {
            // here we would reduce the promise (maybe it's a list of promises built with Promise.All(??)
            const apiResponse = await getResponse(notCachedUPCs);
            return mergeResponseWithCache(upcs, apiResponse);
        }
        return mergeResponseWithCache(upcs);
    }
}

As you can see subsequent use of this cache strategy would reduce API calls, whereas the use-asset args deep equal strategy would be unable to. I know that you support peek and I can work around that somewhat, but I want the fallback to operate on multiple cache keys via array lists. I didn't see a way around that with use-asset. If I had to write my own caching on top then it would not seem as worthwhile.

The other feedback I have is that I worked previously for a sports betting syndicate and we couldn't show any stale data after a period of time. The way you have useMemo would prevent that cache invalidation from taking effect. I would imagine 99.9% of people would want that functionality as you have it and not have the fallback triggered if a render was triggered (and it was removed from cache), so that one is just feedback. I think an opt-out on the memoization or a way to provide a unique optional key to clear the memoization would maybe be a useful feature.

drcmda commented 3 years ago

I want a single promise to contain multiple cache keys.

this you can do, it's cached according to multiple keys by default

useAsset((arg1, arg2, arg3, ...rest) => Promise, [1, 2, 3, 4, 5, 6, 7])

but perhaps it would be interesting to allow it to set a function cache validadator, similar to how you could set the equality function in redux: useAsset(cb, [1,2,3,4,5], (...args) => true/false) it then calls this validator against its cached keys.

or if you have a specific implementation in your mind, let's add it.

brianzinn commented 3 years ago

I think I didn't get my point across, but you are on the right track with the validator. I think I would need a way to provide useAsset with a way to retrieve keys from my resolved promise. In my example above would be this code:

apiResponse.inventoryLevels.forEach(response => {        
    UPC_CACHE[upc] = response.quantity;
})

That is the functionality that I believe I need to be able to load game assets using the same logic.

// for string->number and array would be:
useAsset((...) => Promise, (result: any[]) => Record<string, number>)
// for more complex keys would need to be generalized further

So, if I tried to retrieve keys "1", "2", "3" I would do so in a single promise and a single fetch request. Then when I subsequently retrieve "3", "4", "5" - I think I would need to peek and remove "3" myself - although a specialized implementation would handle that (also in previous example). The point is your cache from what I see has no way to cache except by args, whereas I want to cache by the response. I will review your library again tonight to see if I have misunderstood how it works.

drcmda commented 3 years ago

yes, do that, if you find something that might be useless i'd also be open to sharing authorship. must admit i dont get the cache by response part. suspense works by first checking if something has been processing and returning that, otherwise it must throw an exception (the promise). if it must execute something first in order to see if that meets cache criteria, wouldn't that cause an infinite loop? anyway - i dont know babylons requirements, but at least for three, the loader and the user specified args work very well - been using it for almost 2 years.

brianzinn commented 3 years ago

@drcmda you are right - I was conflating the caching concepts! I wrote a full implementation just now and ended up needing a secondary cache (for responses and building promises) vs Suspense cache :) What I will do now is refactor it and bring in use-asset! I think lots of devs want an unopinionated way to load images/textures/etc and really appreciate your guidance and suggestion. I'm busy rest of this week, but it's in the works now! Cheers.

dennemark commented 3 years ago

@brianzinn sorry, I forgot to reply. Actually your first post clarifies a lot the concept of both versions Model and useLoader. The useLoader concept is a great idea!

Concerning the discussion about use-asset, I guess it would be a great addition for images/textures/json/.., but concerning typescript support I guess your loaders should work, too?

brianzinn commented 3 years ago

@dennemark no worries. the new version I am working on does support typescript fully - here is a screenshot from a reference project in flight: image the NPM is here (does not work with react-babylonjs yet). https://github.com/brianzinn/react-babylonjs-loaders

I will need to backport those hooks to work with react-babylonjs as they are designed to work with what will be a context for Scene that will work with or without the renderer (react-babylonjs). Everything with model will break with adding support for Suspense . if you use something like use-asset then you need to be careful if you are re-using any cached responses - even a mesh cannot be re-used in a new Scene (ie: if you navigate away and return). There are ways to serialize and hydrate a Mesh object, but have not seen a way to do so with textures. I will probably ask in the main forum if it can be done, though. I will try to bring in use-asset into that new library once it is working - will still need our own cache invalidation due to constraints mentioned above.

dennemark commented 3 years ago

@brianzinn I am resurrecting this topic. I am currently thinking about ways to cache/reuse assets, such as texture, json or meshes and I think it fits the discussion. (my precise use case: I have tree meshes, their node materials and texture, but load them individually once, to create the final asset and want to reuse it if I change the scene content)

I found multiple options to achieve reusable resources. 1. AssetContainers in BabylonJS https://doc.babylonjs.com/divingDeeper/importers/assetContainers 2. useAssetManager by react-babylonjs and 3. use-asset by @drcmda (pmndrs)

It seems the first two options need a persistent scene (probably since shaders will be build). Therefore assets cannot be moved to another scene, when the old scene is disposed before. I am not sure if use-asset implementation in r3f needs a scene, but for i.e. an unprocessed json file it will keep the cached file even between scenes.

To my understanding useAssetManager is doing caching only within its call? So if I have multiple assets of the same name, it would only load them once? AssetContainer on the other hand can take multiple assets and be extended in runtime, however instantiating assets into the scene from it is only possible for all assets within the container (instantiation is pretty useful in combination with cached meshes). So one would need multiple asset container for multiple assets. use-asset would probably face similar issues, if it wants to store assets that are compiled for scene usage (i.e. a texture), since babylon needs the scene. however it could store the raw data (i.e. a .jpg) and supply it, when a new scene is created.

Please correct me if I am wrong with my assumptions. To store assets inbetween scenes, I should probably use either browser-cache or use-asset and afterwards build my assets from it. Alternatively: Just keep one scene and engine alive and just dispose/recreate assets in it. This would be possible with AssetContainer (maybe in combination with use-asset) and maybe useAssetManager?

Best, Martin

brianzinn commented 3 years ago

Actually you have found the main reason that I didn't use use-asset. I needed the cache to clear when the scene changed and actually would prefer to have the option to avoid that as well. It would be nice to support cross-scene caching, but also some people may have multiple scenes loaded at once and switch them for rendering. It is not only caching during the call - the response is cached forever, because otherwise on every render even after the asset was loaded a promise would throw and it would try to reload. Check out this part of the code: https://github.com/brianzinn/react-babylonjs/blob/master/src/hooks/loaders/useAssetManager.tsx#L114

I think this is where babylonjs is lacking in that a loaded Mesh cannot be used across scenes without an AssetContainer. There is a post on the old html5gamedevs from deltakosh about how it's not possible, but I think that was before AssetContainer. I think the Three way of having a reusable geometry is a plus - that can be cached all day. I put in a comment in the hook on the referenced link above - essentially to cache across scenes would need to add an option to maintain a cache and that will increase memory usage - an easy out is to use one asset container for each asset. As you can see the useAssetManager hook itself isn't fully thought out I left a comment that the API will change and was going to revisit this at some point :) Keep in mind also that some model formats load their own assets separately, so even just to store the original HTTP response as data isn't enough. Changes are probably needed in upstream library was my first thought when I originally came across this.

drcmda commented 3 years ago

as for use-asset, cached assets should never be cleared or disposed of, because they are held outside of react. clearing them would be against the point. in r3f you can re-use cached assets in any scene, even when the previous one unmounts. this is one the critical things about suspense and cache control, it's outside the mount phases.

for instance, this model can mount and unmount, but the buffers and materials will always be there:

function Model({ url }) {
  const { scene } = useLoader(GLTFLoader, url)
  return <primitive object={scene} />
}

but in gltfjsx'es case it could even mount multiple times, because it's immutable.

gltfjsx especially is one of the most important assets we have in the r3f eco system, this is light years from how people have dealt with gltf before. it could work in babylon as well if more parts between these libs are shared.

brianzinn commented 3 years ago

Clearing the cache when the scene changes was intended as an interim shortcut/hack and not intended to be permanent. Something could be written to cache the geometries and materials and then rehydrate meshes/bones/skeletons/animations into another scene. The AssetContainer is just a scene object holding those assets (https://github.com/BabylonJS/Babylon.js/blob/master/src/assetContainer.ts) and is an option. I'm definitely open to working on an ecosystem to share and battle test those libraries to target different renderers and 3D engines.

dennemark commented 3 years ago

Actually it seems the scene is the bottleneck for proper usage in use-asset. AssetContainer needs a scene and it cannot pass the objects to another scene. I asked in the babylon forum, concerning this issue. Lets see where it leads: https://forum.babylonjs.com/t/keeping-scene-objects-in-memory-after-scene-dispose/18538

brianzinn commented 2 years ago

a similar library to use-asset is out https://github.com/pmndrs/suspend-react. it's closer to the syntax that I was imagining. I am going to propose some changes to upstream library for how models are loaded, so global caching would work - compared to how models are loaded directly to a scene. will post back.

drcmda commented 2 years ago

@brianzinn soon cache will also live in react, it won't be global: https://github.com/pmndrs/suspend-react#react-18

each render-root will create a cache root by default, but users can also make their own. the following for instance will result in two fetches:

import { Cache } from 'react'

<App>
  <Cache>
    <Model url="foo.glb" />
  </Cache>
  <Cache>
    <Model url="foo.glb" />
  </Cache>
</App>

this is great for multiple canvases, routes and many other things. if you see potential in suspend-react, would be open to sharing repo/team privileges.

dennemark commented 2 years ago

suspend-react seems really promising! i have been using use-asset for assets like custom node materials, was nice to work with. But the synchronous-like approach of suspend-react looks great!

I am curious of how you would implement it @brianzinn . Are there two options? 1. just load the glb file and return it. 2. load glb file and load it into babylon scene. Thinking about it, I guess the second approach makes sense, although it would not work, if another scene is created. But since suspend-react exists within a render-root its cache-root will be unmounted as soon as the engine and with it the scene are unmounted. So in this case option 1. might be better suited, since the glb files are cached even if the engine is unmounted. Would be annoying though, if react-babylonjs creates its own cache boundary and cannot take the cache boundary of parent react-dom renderer...

brianzinn commented 2 years ago

I am properly investigating now. Another use case that I am trying to solve for is this (Cache part is optional):

<App>
  <Cache>
    <Model url="foo.glb" position={...} />
    <Model url="foo.glb" position={...} />
  </Cache>
</App>

I realize that I can put in a key or something to force double fetching, but to do a single fetch and have 2 models. It can be solved with instancing and I have a storybook for that, but it would be nicer if it was a bit transparent. The thing about babylon.js is that it loads the model onto the Scene directly, so there is no way to declaratively duplicate. I am going to propose a new loading stage, which is a cacheable object that can be rehydrated as a model.

brianzinn commented 3 months ago

maybe available in babylon 8, but I am closing this outstanding issue from not having a workable solution due to downstream limitations. with the intermediate stage proposal would be a good opportunity to introduce pmndrs cache/state libraries https://github.com/BabylonJS/Babylon.js/issues/14567