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

instantiateRenderEntity sets up render assets on render components #4039

Merged
merged 5 commits into from
Feb 22, 2022

Conversation

mvaligursky
Copy link
Contributor

@mvaligursky mvaligursky commented Feb 22, 2022

  • When an Entity hierarchy was created using GlbContainer.instantiateRenderEntity, the render component didn't have .asset property set up. This is now fixed, and those properties point to a render assets.

@mvaligursky mvaligursky self-assigned this Feb 22, 2022
@mvaligursky mvaligursky added area: graphics Graphics related issue bug labels Feb 22, 2022
@mvaligursky mvaligursky requested a review from a team February 22, 2022 11:41
@willeastcott
Copy link
Contributor

Wait, what's the type of the resource of a render asset? e.g. entity.render.asset?

@mvaligursky
Copy link
Contributor Author

Wait, what's the type of the resource of a render asset? e.g. entity.render.asset?

the code I added assigns Asset type to it created here:

static createAsset(assetName, type, resource, index) {
const subAsset = new Asset(assetName + '/' + type + '/' + index, type, {
url: ''
});
subAsset.resource = resource;
subAsset.loaded = true;
return subAsset;
}

But internally this is stored as an id on the component, and render.asset returns asset.id

@willeastcott
Copy link
Contributor

I meant, is the asset's resource a Render? Just wondering, because that type is not public API.

@mvaligursky
Copy link
Contributor Author

I meant, is the asset's resource a Render? Just wondering, because that type is not public API.

Fair point. Making the Render class public.

@willeastcott
Copy link
Contributor

Are developers expected to create Render objects themselves? Or do anything with them? Is the Render API in 'final' form?

@mvaligursky
Copy link
Contributor Author

I'm hiding the constructor as well .. as there's not a big use case for its public use.
Otherwise only meshes properties is exposed .. and I think that's useful to have.

@slimbuck
Copy link
Member

slimbuck commented Feb 22, 2022

Do we need to make Render class public API?

Looks like instances of Render are created by the glb-parser at load time and then used by the render component internally (via asset.resource).

Even though the Render class seems simple and innocuous enough, it's not actually possible to set a Render instance directly on the RenderComponent anyway. So seems like we can leave Render private?

@willeastcott
Copy link
Contributor

I'd probably agree with that assessment. Making Render public is not necessary to solve the original motivation for this PR.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area: graphics Graphics related issue bug
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants