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

Migrate billboard to bit-ecs #5901

Merged
merged 9 commits into from
Jan 23, 2023
Merged

Migrate billboard to bit-ecs #5901

merged 9 commits into from
Jan 23, 2023

Conversation

keianhzo
Copy link
Contributor

@keianhzo keianhzo commented Jan 19, 2023

This PR migrates the billboard component and also makes some choices that I'd like comments on:

  • Adds a inflatorWrapper function to wrap a component inflator. I assume we will need to do this quite a lot for some components so it might be useful to have a convenience function for it
  • I found myself using the default inflator in a few places so I thought that it might make sense to reuse the same one. As we add more it might make sense to expose them in an object
  • Exposes the main camera in App. I think we are accessing to this through the scene in a few places so I thought it might make sense to expose it here

src/aframe-to-bit-components.js Show resolved Hide resolved
src/bit-systems/billboard.ts Outdated Show resolved Hide resolved
src/bit-systems/billboard.ts Outdated Show resolved Hide resolved
src/gltf-component-mappings.js Outdated Show resolved Hide resolved
src/bit-systems/billboard.ts Outdated Show resolved Hide resolved
src/bit-systems/billboard.ts Outdated Show resolved Hide resolved
Copy link
Contributor

@netpro2k netpro2k left a comment

Choose a reason for hiding this comment

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

Looking good. Made a few small comments.

Biggest one would be getting rid of use of frame scheduler, I suspect we won't need that anymore going forward but maybe we will want some utility functions for doing something similar if we find ourselves writing the same code in many systems.

Also lets make sure we preserver that TODO, billboard as it is kind of stopped making sense since we now have many potential cameras in the scene. (Maybe it just needs to be renamed face-player. I would definitely like to also explore something more akin to how other engines handle billboarding and have it be more of a rendering concept, sitting at the same level as something like Mesh. That allows us to do the billboarding as part of the vertex shader.

src/utils/jsx-entity.ts Outdated Show resolved Hide resolved
src/aframe-to-bit-components.js Show resolved Hide resolved
src/bit-systems/billboard.ts Outdated Show resolved Hide resolved
src/bit-systems/billboard.ts Outdated Show resolved Hide resolved
src/bit-systems/billboard.ts Outdated Show resolved Hide resolved
src/app.ts Outdated Show resolved Hide resolved
src/gltf-component-mappings.js Outdated Show resolved Hide resolved
src/bit-systems/billboard.ts Outdated Show resolved Hide resolved
@keianhzo
Copy link
Contributor Author

Thanks for the feedback, it helped a lot. Regarding the vertex shader approach, definitely worth exploring specially if lookAt is as expensive as you mentioned. We still would need it for non-mesh objects but it would save some CPU for geometries.

I have addressed most of the feedback except from:

  • I have kept the inflatorWrapper. I think the amount of code that we are adding < amount of code that we are removing and could be handy for other simple components.
  • As inView ended up being just a function call after the refactor, I have left onlyY as a boolean.

src/bit-systems/billboard.ts Outdated Show resolved Hide resolved
src/utils/jsx-entity.ts Outdated Show resolved Hide resolved
@keianhzo keianhzo merged commit 554f519 into master Jan 23, 2023
@keianhzo keianhzo deleted the bitecs-billboard branch January 23, 2023 12:24
@keianhzo keianhzo mentioned this pull request Jan 31, 2023
50 tasks
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.

3 participants