Skip to content

Feat/transparent background #314

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

Merged
merged 9 commits into from
Jan 24, 2022
Merged

Feat/transparent background #314

merged 9 commits into from
Jan 24, 2022

Conversation

daenash
Copy link
Contributor

@daenash daenash commented Nov 18, 2021

Describe the change

Issue #152

The EngineView is extended with an isTransparent boolean flag which is not required to set, on default it's false.

If it's set to true this happens:

  • Android side: The SurfaceView is changed to a TextureView and the root FrameLayout is extended to implement a TextureView.SurfaceTextureListener. The listener itself is using a similar runnable implementation as the SurfaceView. Note: There is no XR related implementation.
  • iOS side: The MKTView is extended with an isTransparent @property and it sets whether the opaque flag is true or false.

Screenshots

Nope, sorry 🦄

Documentation

Updated

Testing

I need help testing on Windows
For the other platforms I tested it and it worked, so YES

@ghost
Copy link

ghost commented Nov 18, 2021

CLA assistant check
All CLA requirements met.

@ryantrem
Copy link
Member

Thanks for this contribution! There are two main questions for me:

  1. Does the Android solution really work? It seems like the transparency flag needs to be passed to the EngineView instance after it is created.
  2. Can we still enable the XR on Android even when transparency is enabled on the primary view? I don't see any technical limitations, and it seems like this behavior would match what iOS is doing.

@ryantrem
Copy link
Member

@chrisfromwork, @rgerd - do you know what needs to happen to at least stub this out on Windows (e.g. just making sure this change doesn't break on Windows even if you don't specify the isTransparent property)?

@daenash
Copy link
Contributor Author

daenash commented Nov 18, 2021

@ryantrem

Thank you for the review! 🙌 Well basically I'm not a native developer so sorry for the flaws I might have made here 🙃 I'll try to do some changes based on your suggestions, but I don't really know when.

For my project I just patched the package with the transparent solutions, so there is no chance for it being not transparent. This was the case I needed.

I'm not quite understand what that XR view is, and for my purpose I haven't needed it. I'll need to take a closer look at that.

@ryantrem
Copy link
Member

@daenash I think your code is really close, mainly just need to sort out how the transparency flag is passed to the Android EngineView! Really appreciate the contribution!

The XR view is the augmented reality view when you use Babylon.js to create and start a WebXR session (https://doc.babylonjs.com/divingDeeper/webXR/webXRSessionManagers). So the EngineView classes have a different view for regular rendering vs. XR rendering because they just work really differently. But, the XR view should be supported (and work the same way) regardless of whether transparency is enabled. From what I can tell, that's actually what your implementation does for iOS since you explicitly ignore the transparency flag on the xr view.

@daenash
Copy link
Contributor Author

daenash commented Nov 22, 2021

@ryantrem

With more code inspection I realized maybe it would be good the separate the two engine views to two separate components

One for transparent background one opaque like this

<EngineView />
<EngineViewTransparent />

And in the Android part we would implement two different classes which are extending a JAVA abstract class which contains the Babylon updating functionalities. So this way these classes would only need to implement the TextureView.SurfaceTextureListener and SurfaceHolder.Callback separately and extend a so called EngineViewBase class.

For iOS as only a flag would change on the View some other approach would be nice. I'll think about it.
Until that in Typescript maybe we could only render an EngineView with a transparency flag for iOS.

I'll try to create a structure for what I mean.

@ryantrem
Copy link
Member

That's an interesting idea. I think I'd be more inclined to keep it as one control though to keep the API simple. If we add more options like this, we could end up with an explosion of control variants, which wouldn't scale.

@CedricGuillemet
Copy link
Contributor

Hi @daenash , any update on this PR?

@daenash
Copy link
Contributor Author

daenash commented Jan 18, 2022

@CedricGuillemet Hi! No, unfortunately I had no time to follow up with this. 😿 Feel free to continue working on it if you feel so.

@CedricGuillemet
Copy link
Contributor

I changed the Android impl:

  • always using a TextureView so opacity can easily be changed at runtime
  • removed transparency boolean in engine view manager
  • added code in mandatory callbacks when view size changed

@ryantrem
Copy link
Member

I changed the Android impl:

  • always using a TextureView so opacity can easily be changed at runtime
  • removed transparency boolean in engine view manager
  • added code in mandatory callbacks when view size changed

I don't think we should switch to TextureView unconditionally as it has perf implications. My understanding is that with TextureView, since the compositor draws it, frame rate is limited to the compositor frame rate, and there is an extra layer of synchronization (e.g. Babylon's frame timing compared to the Android UI compositor's frame timing). I think it would be much safer to only use TextureView when transparency is needed. There is some discussion about this in the associated issue as well.

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.

4 participants