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: Support media with link component #6272

Merged
merged 2 commits into from
Oct 12, 2023
Merged

Conversation

keianhzo
Copy link
Contributor

@keianhzo keianhzo commented Sep 19, 2023

Fixes #6263

In the AFrame loader when an object had a link and a image components (also works with video and loader) the media would override the link media

In the new loader we don't support a media loader loading multiple media. The second media would override the first one's parameters.

Using image/video/model with the link component seems to be a pretty common use case. This PRs adds a hack to support that specific use case.

Another alternative would be to add support for multiple loads to the media loader but I'm no sure if that's something that we want...

@keianhzo keianhzo requested a review from takahirox September 19, 2023 10:07
@keianhzo keianhzo changed the title Support media with link component bitECS: Support media with link component Sep 19, 2023
@keianhzo keianhzo added new-loader P1 Address as quickly as possible labels Sep 19, 2023
@takahirox
Copy link
Contributor

takahirox commented Sep 21, 2023

As commented inline this solution is very hacky for sure. Give me some time to think whether there is clearer solution...

I haven't deeply thought yet but making a new component (that holds link url) for this special case rather than adding link url to MediaLoader component would be simpler? Anyways give me more time...

@takahirox
Copy link
Contributor

Let me share my thoughts.

With the new loader enabled, we prevent a glTF node that has multiple Hubs components that generates Three.js object. We preprocess such nodes to make multiple Hubs Component glTF nodes under the node. Under this policy, Link + Media is not doable as is.

Probably we don't spend long time for this problem. So probably preprocessing Link + Media glTF Hubs Components and reusing the existing code as much as possible would be good. Also, adding a special path after loading a media as this PR does would be reasonable.

But I don't prefer adding linkSrc property to MediaLoader because MediaLoader component will have strong dependency with Link. Maybe introducing a new component (eg: MediaLink) would be simpler.

This code in the media loading system

  const linkSrc = APP.getString(MediaLoader.linkSrc[eid]);
  if (linkSrc) {
    ...
  }

would be replaced with like

  if (hasComponent(world, MediaLink, eid)) {
    const linkSrc = APP.getString(MediaLink.src[eid]);
    ...
  }

@keianhzo keianhzo force-pushed the bitecs-media-with-link branch from aa39222 to c619017 Compare October 2, 2023 15:11
@keianhzo
Copy link
Contributor Author

keianhzo commented Oct 2, 2023

Yes, that makes sense and I like that is much more clear, updated.

@keianhzo keianhzo force-pushed the bitecs-media-with-link branch from c619017 to 91ec811 Compare October 12, 2023 12:34
@keianhzo keianhzo merged commit d9c7904 into master Oct 12, 2023
9 of 11 checks passed
@keianhzo keianhzo deleted the bitecs-media-with-link branch October 12, 2023 12:45
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.

bitECS: No link button when Link and Image components are defined
2 participants