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

Function to add element and all its children to AssetContainer #14457

Merged
merged 6 commits into from Oct 25, 2023

Conversation

carolhmj
Copy link
Contributor

Fixes https://github.com/BabylonJS/ThePirateCove/issues/663

The function addAllAssetsToContainer receives a Node as argument and adds that node and all its children and derived elements (such as materials and skeletons) to an asset container. It is intended to work as a utility function used after loading a mesh with AssetsManager, for example. Usage example on: #TLFUIB#1

@bjsplat
Copy link
Collaborator

bjsplat commented Oct 24, 2023

Please make sure to label your PR with "bug", "new feature" or "breaking change" label(s).
To prevent this PR from going to the changelog marked it with the "skip changelog" label.

@bjsplat
Copy link
Collaborator

bjsplat commented Oct 24, 2023

packages/dev/core/src/assetContainer.ts Outdated Show resolved Hide resolved
packages/dev/core/src/assetContainer.ts Outdated Show resolved Hide resolved
Copy link
Member

@PatrickRyanMS PatrickRyanMS left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In looking through this, I think this is the right path, but I still have a question. I'm assuming this mirrors what we do with import to an asset container with a glTF. However, since this can be called after import, do we care about other material classes that could be attached to a node? For example, do we need to worry about node material or multi-material if those are assigned to the node? That would mean needing to search the node material for texture nodes. I don't know if this is a good path or not. I think getting opinions from @RaananW, @sebavan, or @Popov72 would be good here.

Copy link
Member

@RaananW RaananW left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Feel free to ignore my single comment (in this case, of course ;-) )

packages/dev/core/src/assetContainer.ts Outdated Show resolved Hide resolved
@sebavan sebavan merged commit 841412b into BabylonJS:master Oct 25, 2023
10 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants