Add second instance of Renderer to manage transparent background. by nicolas-daures · Pull Request #949 · google/model-viewer

nicolas-daures · 2020-01-03T13:09:52Z

Reference Issue

Fixes #874

Management of the "transparent" value for the background-color property.

To allow the property value to change (transparent -> opaque, opaque -> transparent), we create two instances of the Renderer class. The first to manage a background color, the second to manage transparency.

cdata · 2020-01-09T17:48:38Z

Thanks for the change suggestion @nicolas-daures !

Would it be possible to structure this so that the transparent renderer is created on-demand? This way folks only pay a cost in the case that they actually want to use transparency.

Also for the sake of cross-referencing things, we have had issues with transparent rendering in the past (see #381 ).

elalish

Could you add an example of using transparency, perhaps here?

elalish · 2020-01-09T17:31:38Z

src/three-components/Renderer.ts

      const widthDPR = width * dpr;
      const heightDPR = height * dpr;
+      if (this.isTransparent) {
+        context.clearRect(0, 0, widthDPR, heightDPR);


Why is it necessary to clear both the context and three's renderer color?

If I don't clear the context, the transparent pixels of the image to draw in the canvas will not overwrite the pixels of the previous image.

If I don't clear the color buffer of the three renderer, the transparent pixels of the new rendering will not overwrite those of the previous rendering.

…-viewer

nicolas-daures · 2020-01-14T13:41:35Z

@cdata The renderer is now created on-demand.

About transparency issues, I have the same rendering as on this comment: #381 (comment)

@elalish I added an example in customize-ui with a linear gradient background to see the transparency.

elalish

Thanks for bringing the rendering issue to my attention. I don't see a reason to hold this back due to a Three.js bug, especially since it works for most cases. I want to test this a bit more before merging, but I really like it! Thanks for taking the initiative!

elalish · 2020-01-14T16:57:52Z

packages/model-viewer/src/three-components/Renderer.ts

+
+  static get singleton(): Renderer {
+    let singleton = this[$singleton];
+    if (!singleton) {


nit: in this project's style we prefer to say explicitly x == null rather than !x. Also, is the temporary variable necessary here? Same comment for the changes below.

I modified the code to adapt to the project's style. I deleted the temporary variable. It just used to avoid the cast of Renderer.

elalish · 2020-01-14T17:02:47Z

packages/model-viewer/examples/customizing-ui.html

+        </div>
+        <example-snippet stamp-to="demo-container-2" highlight-as="html">
+          <template>
+            <model-viewer ar camera-controls ios-src="assets/Astronaut.usdz" src="assets/Astronaut.glb" background-color="transparent" alt="A 3D model of an astronaut">


nit: Everything inside the <template> should be indented starting over from the left. If you look at this page in your browser you'll see why. Also, can we add an image or some text or something behind this so that it's clear the transparency is working? These dev pages serve as manual regression tests for this project.

Ah, I just saw the linear gradient; the trouble is that it's not inside the <template>, so it's not a visible part of the demo code.

I modified the example to integrate the gradient and a text in the template. Thanks for all the feedback.

valdrinkoshi · 2020-01-15T16:51:58Z

packages/model-viewer/src/three-components/Renderer.ts

+    if (this[$singleton] != null) {
+      (this[$singleton] as Renderer).dispose();
+    }
    this[$singleton] = new Renderer({debug: isDebugMode()});


Should this be created only if the previous value was defined (aka inside the if statement)? Same for the singleton with transparency below.

Good remark, thank you.

cdata · 2020-01-16T18:16:53Z

@nicolas-daures thanks so much for your diligent work on this change. I want to give you a friendly heads up that we are trying to start the year off on the right foot by landing a major source restructuring change (see #962 ). Your changes are still good, but I will likely ask you to reconcile before this lands and everything will have been moved two directories deep.

Anyway, I am happy to help you reconcile the changes when that time comes. Just wanted to let you know what is going on.

elalish · 2020-01-16T18:21:20Z

@nicolas-daures This seems to work, except when switching between the renderers. You can repro by scrolling up and down on the customize-ui page. It seems to switch back and forth between rendering transparently and non each time it comes into view. Can you repro?

…-viewer

nicolas-daures · 2020-01-17T06:42:33Z

@cdata I merged master in my pull request to get the packages and lerna. It works. I would update the pull request with your changes regularly.

@elalish I can reproduce the bug and I fixed it. Thanks for the feedback.

elalish · 2020-01-17T22:15:08Z

@nicolas-daures Thanks! That big change @cdata was talking about just landed, so once you merge or rebase I'll take another look. Note the build instructions have changed too, so check out the updated READMEs.

…-viewer # Conflicts: # packages/modelviewer.dev/examples/customizing-ui.html

nicolas-daures · 2020-01-21T06:29:37Z

@elalish It's merged.

cdata

Thanks for keeping this in sync with master, I know there has been a lot of change happening on the side 🍻

Ideally we would also have at least one test that confirms that setting backgroundColor to "transparent" causes the canvas to render with alpha transparency.

cdata · 2020-01-21T15:26:52Z

packages/model-viewer/src/features/environment.ts


+      // Update background color before to get texture utils because renderer singleton depends on background transparency
+      if (skyboxImage == null)
+      {


clang-format needs to be run on this change

cdata · 2020-01-21T15:27:59Z

packages/model-viewer/src/features/environment.ts

+        if (backgroundColor === 'transparent') {
+          if (this[$scene].background != null) {
+            this.disconnectedCallback();
+            delete this[$scene].background;


If this isn't significantly different from assigning null, prefer to assign null instead

ok, that's fixed.

cdata · 2020-01-21T15:30:51Z

packages/model-viewer/src/features/environment.ts

+      {
+        if (backgroundColor === 'transparent') {
+          if (this[$scene].background != null) {
+            this.disconnectedCallback();


These (connectedCallback/disconnectedCallback) are custom element lifecycle methods, and are meant to be invoked by the browser based on the semantics defined in the custom element spec. If we are calling them manually, then it's a sign that we may need to factor out part of their implementation to discrete methods.

Why are we invoking these methods manually?

These callbacks are called to unregister the scene from the renderer and to unsubscribe from the contextlost event. I change the code to call only what is necessary.

cdata · 2020-01-21T15:33:47Z

packages/model-viewer/src/three-components/Renderer.ts


-  static get singleton() {
-    return this[$singleton];
+  static get singleton(): Renderer {


It seems weird to call this a singleton now that we are creating up to two of them. Maybe we should just call them opaqueRenderer and transparentRenderer?

ok, that's renamed.

cdata · 2020-01-21T15:37:17Z

packages/model-viewer/src/features/environment.ts

+        }
+        else {
+          const mustUpdateRenderer = this[$scene].background == null;
+          mustUpdateRenderer && this.disconnectedCallback();


If these lifecycle methods are being invoked in order to get the renderer to update, then we could use this[$needsRender]() to provoke a re-render and skip calling the lifecycle methods.

The objective is not really to update the rendering but to unsubscribe listeners and unregister the scene from the old renderer before switching to the new renderer. So the accessor $renderer must return the old renderer to unsubscribe the listener, then the same accessor must return the new renderer to subscribe the listener. This is why there is the test to null on the background performed in the accessor.

cdata · 2020-01-21T15:38:14Z

packages/model-viewer/src/model-viewer-base.ts


  get[$renderer]() {
-    return Renderer.singleton;
+    return this[$scene].background ? Renderer.singleton : Renderer.singletonWithTransparency;


It seems like we should only use the transparent renderer if the background color is, in fact, "transparent". One way to accomplish this would be to specialize the [$renderer] accessor for the EnvironmentMixin implementation, so that it overrides the default ("always use opaque renderer") with a version that returns a renderer based on the value of the backgroundColor property.

koch-crsd · 2020-01-21T16:19:12Z

hi guys, great work! :)

I have a small question: I've noticed that if the background-color attribute is set as "transparent" it works. But if for example additionally also the shadow-intensity attribute is set, then the transparency isn't applied and it gets the default white background.?

it would be really awesome if the transparency would still work even when having the shadow-intensity attribute set...

… Assign null to background instead of using delete.

elalish · 2020-01-31T22:05:52Z

@nicolas-daures @valdrinkoshi @koch-crsd I have an alternative PR that I'd love feedback on: #975. It draws heavily from this one, but also simplifies our API.

elalish · 2020-02-03T17:01:34Z

Closing in favor of #975. Thank you @nicolas-daures for pushing us on this and contributing to the landed change!

nicolas-daures added 2 commits January 3, 2020 14:07

Add second instance of Renderer to manage transparent background.

d8a3165

Revert the deletion of the default background.

de7510c

elalish reviewed Jan 9, 2020

View reviewed changes

nicolas-daures added 3 commits January 14, 2020 08:59

Create Renderer singleton on-demand.

948cb22

Add example of transparent background.

701b3c7

Merge branch 'master' of https://github.com/GoogleWebComponents/model…

4bcf2f1

…-viewer

elalish mentioned this pull request Jan 14, 2020

GLTFLoader: Texture alpha should be ignored on opaque materials mrdoob/three.js#15483

Closed

elalish reviewed Jan 14, 2020

View reviewed changes

nicolas-daures added 2 commits January 15, 2020 08:34

Remove useless variable, fix code style.

f3fb39c

Add gradient and text in background of transparent background example.

d964870

valdrinkoshi reviewed Jan 15, 2020

View reviewed changes

Reset instance of renderer only if previous instance exists.

e67acca

nicolas-daures added 2 commits January 17, 2020 07:23

Merge branch 'master' of https://github.com/GoogleWebComponents/model…

05cc4de

…-viewer

Fix background transparency while switching between the renderers.

e73b670

Merge branch 'master' of https://github.com/GoogleWebComponents/model…

055a4df

…-viewer # Conflicts: # packages/modelviewer.dev/examples/customizing-ui.html

cdata suggested changes Jan 21, 2020

View reviewed changes

Rename renderer singletons to opaqueRenderer and transparentRenderer.…

a4aa0b5

… Assign null to background instead of using delete.

elalish mentioned this pull request Jan 29, 2020

Transparent rendering #975

Merged

elalish closed this Feb 3, 2020

Conversation

nicolas-daures commented Jan 3, 2020

Reference Issue

Uh oh!

cdata commented Jan 9, 2020

Uh oh!

elalish left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nicolas-daures commented Jan 14, 2020

Uh oh!

elalish left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cdata commented Jan 16, 2020

Uh oh!

elalish commented Jan 16, 2020

Uh oh!

nicolas-daures commented Jan 17, 2020

Uh oh!

elalish commented Jan 17, 2020

Uh oh!

nicolas-daures commented Jan 21, 2020

Uh oh!

cdata left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

koch-crsd commented Jan 21, 2020

Uh oh!

elalish commented Jan 31, 2020

Uh oh!

elalish commented Feb 3, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants