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

bitECS object list support #6067

Merged
merged 1 commit into from
Sep 27, 2023
Merged

bitECS object list support #6067

merged 1 commit into from
Sep 27, 2023

Conversation

keianhzo
Copy link
Contributor

@keianhzo keianhzo commented May 10, 2023

This commit adds bitECS object list support.

Basic Strategy

Reuse the existing A-Frame based code as much as possible for now.

In the related functions that take A-Frame element, take Object3D instead. Object3D has a reference to A-Frame element in the A-Frame based implementation and has a reference to Entity ID in the bitECS. The functions process with A-Frame element or Entity ID depending on whether new loader enabled.

Explicitly use shouldUseNewLoader() and/or make two functions, one for A-Frame based and another one for bitECS based implementation, if different logics are needed between A-Frame based and bitECS based implementations. Duplicated codes may not be perfectly removed but it would be simpler to follow the code rather than the new loader pretends the old one. And it would be easier to edit
the code when we will get rid of A-Frame.

Changes

  • Introduce MediaInfo component and save url and content type into it when loading media in media-loader. The info is used in the object list
  • Fire listed_media_changed event when media is loaded via media-loader and when the loaded media entity is removed
  • Take Object3D instead of A-Frame element in the related functions
  • Use shouldUseNewLoader() and/or make two separated functions
    for A-Frame and bitECS where the different logics are needed

Future TODOs

  • Support avatars

Authors @keianhzo and @takahirox

@@ -38,7 +38,7 @@ import {
Billboard,
MaterialTag,
VideoTextureSource,
Mirror
Inspectable
Copy link
Contributor

Choose a reason for hiding this comment

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

Is removing Mirror intentional?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, It wasn't being used.

@keianhzo
Copy link
Contributor Author

keianhzo commented Jun 6, 2023

Related #6092

@keianhzo keianhzo added this to the New Loader milestone Jun 29, 2023
@takahirox
Copy link
Contributor

Sorry for the late review. Would you mind resolving the conflicts?

@takahirox
Copy link
Contributor

Resolved the conflicts so far.

I read through the changes and found that elOrEid that holds Entity ID for bitECS or Element for A-Frame is used a lot but I don't think it is good design because

  • It may be a bit less readable
  • Variable that can store different types doesn't really fit to TypeScript (when we rewrite the JavaScript code to TypeScript at some point)
  • Dynamically checking type is needed and statical type check can't work

Perhaps explicitly separating the codes between bitECS and A-Frame with shouldUseNewLoader() would be simpler? And it would be easier to edit when we will get rid of A-Frame code. I will try to update the code.

@takahirox
Copy link
Contributor

takahirox commented Sep 8, 2023

Ready for test and review.

I have fixed some bugs, have cleaned up the code, have written the commit log and PR comment.

@takahirox takahirox changed the title BitECS object list support bitECS object list support Sep 8, 2023
@keianhzo
Copy link
Contributor Author

@takahirox Thanks for updating this while I was away. I've added the missing support for pinning. I think that's the last missing part for this PR.

@takahirox
Copy link
Contributor

This PR affects not only the new loader but also the old one so thinking we should test more carefully than other new loader specific PRs before merging.

@takahirox
Copy link
Contributor

Would you mind separating the change for pinned objects stuff from this PR for quicker review? The review of this PR would be easier because we both already looked through the changes without the pinned objects stuff.

@keianhzo
Copy link
Contributor Author

You mean moving the last two commits to another PR? I can do that if that helps reviewing.

@takahirox
Copy link
Contributor

Yes. If you do it, I will approve this PR immediately and can review another one later.

This commit adds bitECS object list support.

**Basic Strategy**

Reuse the existing A-Frame based code as much as possible for now.

In the related functions that take A-Frame element, take Object3D
instead. Object3D has a reference to A-Frame element in the A-Frame
based implementation and has a reference to Entity ID in the bitECS.
The functions process with A-Frame element or Entity ID depending on
whether new loader enabled.

Explicitly use shouldUseNewLoader() and/or make two functions, one
for A-Frame based and another one for bitECS based implementation,
if different logics are needed between A-Frame based and bitECS
based implementations. Duplicated codes may not be perfectly
removed but it would be simpler to follow the code rather than
the new loader pretends the old one. And it would be easier to edit
the code when we will get rid of A-Frame.

**Changes**

- Introduce MediaInfo component and save url and content type
  into it when loading media in media-loader. The info is used
  in the object list
- Fire listed_media_changed event when media is loaded via
  media-loader and when the loaded media entity is removed
- Take Object3D instead of A-Frame element in the related functions
- Use shouldUseNewLoader() and/or make two separated functions
  for A-Frame and bitECS where the different logics are needed

**Future TODOs**

- Support avatars
- Support Pinning
@keianhzo
Copy link
Contributor Author

@takahirox I've removed the last two commits

@keianhzo keianhzo merged commit ff9b049 into master Sep 27, 2023
9 of 11 checks passed
@keianhzo keianhzo deleted the bitecs-object-list branch September 27, 2023 13:11
@keianhzo keianhzo mentioned this pull request Oct 3, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
new-loader P1 Address as quickly as possible
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants