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

Fix type issues for simple-water and particle-emitter #5952

Merged
merged 1 commit into from
Feb 28, 2023

Conversation

keianhzo
Copy link
Contributor

This fixes a bug that I've seen in earlier code of mine. I think the reason the bug was introduced was that after adding support for the glTF inflator I wanted the component to also work for JSX entities and I changed the component parameters data to array obviously breaking the glTF support. I've fixed that.

I've also limited the simple-water and particle-emitter inflators support only to glTF as I've seen that for this type of components we are not really adding support for JSX (see UVScroll component) and adding support for JSX would require adding another inflator which I'm not sure if it's worth i. Is this assumption correct or we want parity?

Copy link
Contributor

@johnshaughnessy johnshaughnessy left a comment

Choose a reason for hiding this comment

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

I've also limited the simple-water and particle-emitter inflators support only to glTF as I've seen that for this type of components we are not really adding support for JSX (see UVScroll component) and adding support for JSX would require adding another inflator which I'm not sure if it's worth i. Is this assumption correct or we want parity?

If we don't have an EntityDef written in JSX that needs to create a simpleWater or particleEmitter, then it's fine to only support these components in gltfInflators.

adding support for JSX would require adding another inflator

This is not necessarily true. It depends whether the data passed in via JSX would be the same shape as what is passed in via gltf components. If the shape of the data is the same for both, then you can re-use the sample inflator. If they are different, then you would need separate inflators.

@keianhzo keianhzo marked this pull request as ready for review February 28, 2023 09:04
@keianhzo keianhzo merged commit ce8e732 into master Feb 28, 2023
@keianhzo keianhzo deleted the water-particles-type-fixes branch February 28, 2023 09:05
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 this pull request may close these issues.

2 participants