initial commit on including babylonjs in renderers' page #1247 #1298

sun765 · 2020-06-18T16:17:12Z

Reference Issue

…ear model when switching scene 2. model's transform 3. loading model's reject call back

sun765 · 2020-06-18T16:20:54Z

packages/render-fidelity-tools/src/components/renderers/babylon-viewer.ts

+
+import {ScenarioConfig} from '../../common.js';
+
+// const IS_BINARY_RE = /\.glb$/;


i don't know what does this variable do, i guess it's checking whether the model is glb or gltf? If that's the case I can just delete it because in Babylonjs you don't have to distinguish these two format

Yes, that's right.

I will delete this then.

sun765 · 2020-06-18T16:22:18Z

packages/render-fidelity-tools/test/renderers/babylon/index.html

+   </babylon-viewer>
+ </renderer-harness>
+
+ <!-- <script src="../../../node_modules/babylon/babylon.js"></script> -->


I probably need to use this line in next PR, so i just comment it here.

elalish

Looking good! Also, go ahead and run update-screenshots and commit those here as well so we can see how the goldens are comparing.

elalish · 2020-06-18T16:29:15Z

packages/render-fidelity-tools/src/components/renderers/babylon-viewer.ts

@@ -0,0 +1,151 @@
+/* @license
+ * Copyright 2019 Google LLC. All Rights Reserved.


This should be updated to 2020 since it's a new file.

Ok, i'll fix it .

elalish · 2020-06-18T16:33:40Z

packages/render-fidelity-tools/src/components/renderers/babylon-viewer.ts

+const $engine = Symbol('engine');
+const $scene = Symbol('scene');
+const $camera = Symbol('camera');
+const $hdrTexture = Symbol('hdrTexture');


I'm over these symbols. We need them in our top-level model-viewer API (the mixins) to keep private stuff mostly private since JS doesn't really have that concept. However, that style got applied somewhat slapdash through the rest of our TS codebase, which I think is unnecessary and ugly. I plan to rip these out of our files at some point, but if you'd like to start here you're welcome to. We can also leave it to a later date, but I want to make sure you're aware.

Ok thanks. Actually I'm wondering about the reason of doing this as well. I will get rid of this later.

elalish · 2020-06-18T16:35:40Z

packages/render-fidelity-tools/src/components/renderers/babylon-viewer.ts

+
+    // load hdr directly
+    this[$hdrTexture] = new HDRCubeTexture(
+        scenario.lighting, this[$scene], 128, false, false, false);


What is 128?

i think this is related to the issue i just posted. #1297 I just replied you there, can you see it there?

elalish · 2020-06-18T16:37:06Z

packages/render-fidelity-tools/src/components/renderers/babylon-viewer.ts

+    // load hdr directly
+    this[$hdrTexture] = new HDRCubeTexture(
+        scenario.lighting, this[$scene], 128, false, false, false);
+    this[$scene].environmentTexture = this[$hdrTexture];


If you don't use this hdrTexture anywhere else, you might skip the member and just put it directly here.

That make sense. I will fix it.

elalish · 2020-06-18T16:39:53Z

packages/render-fidelity-tools/src/components/renderers/babylon-viewer.ts

+
+  private async[$updateScenario](scenario: ScenarioConfig) {
+    if (this[$scene] != null) {
+      this[$scene].dispose();


I'm not familiar with Babylon, but does the scene require disposal while the engine does not? And how about the environment texture?

Scene require disposal because if not, when the scenario changes, the former scenario's model is still in the scene. In the babylonjs's forum I found this is the recommended way to clear all models and meshes.
Engine seems to be independent of scene. it doesn't need to be disposal. I think in babylonjs everything is represented as tree node, so the engine is the root node and the scene is a child node of engine.
I think environment texture should be disposal because some scenario has different environment texture.

elalish · 2020-06-18T16:40:40Z

packages/render-fidelity-tools/src/components/renderers/babylon-viewer.ts

+  }
+
+  private[$render]() {
+  }


Is this necessary?

Yeah, i need to write an event listener here in next PR . Basically it's to tell the index page that the rendering is done and you can go ahead.

bejado · 2020-06-18T18:57:59Z

packages/render-fidelity-tools/src/components/renderers/babylon-viewer.ts

+  constructor() {
+    super();
+  }


Not sure what the code style is for this repo, but this constructor is unnecessary.

bejado · 2020-06-18T19:02:10Z

packages/render-fidelity-tools/src/components/renderers/babylon-viewer.ts

+    const alpha = orbit.phi * Math.PI / 180;
+    const beta = orbit.phi * Math.PI / 180;


Did you mean to use orbit.phi here twice?

Oh it's a mistake, thank you for pointing out that for me!

bejado · 2020-06-18T19:07:35Z

packages/render-fidelity-tools/src/components/renderers/babylon-viewer.ts

+        scenario.model.substring(lastSlashIndex + 1, scenario.model.length);
+
+    await new Promise((resolve) => {
+      SceneLoader.Append(modelRootPath, modelFileName, this[$scene], () => {


SceneLoader.AppendAsync returns a promise, so I think instead you could do:

await SceneLoader.AppendAsync(...)

That being said, it's unclear to me what the difference is between Append and AppendAsync.. maybe Append is already synchronous?

i'm think you are right. I will replace that with the AppendAsync function. Thanks for the comment!

prideout · 2020-06-18T19:14:54Z

LGTM.

…include-babylon-renderer-page

elalish · 2020-06-24T22:40:49Z

packages/render-fidelity-tools/src/components/renderers/babylon-viewer.ts

+  private[$scene]: Scene;
+
+
+  connectedCallback() {


These empty callbacks can also be removed.

elalish · 2020-06-24T22:42:33Z

packages/render-fidelity-tools/src/components/renderers/babylon-viewer.ts

+
+    const {orbit, target} = scenario;
+    const alpha = (orbit.theta + 90) * Math.PI / 180;
+    let beta = orbit.phi * Math.PI / 180;


Can this be const instead?

oh i forgot to change that. At first I saw in babylonjs's document that there'll be error if beta is set to 0 or PI, so I added a condition to added 0.1 offset. But i found it's actually correct without this condition, so i guess babylon has did this conditional offset themselves and I just delete it. Thank you for finding that !

elalish · 2020-06-24T22:44:27Z

packages/render-fidelity-tools/src/components/renderers/babylon-viewer.ts

+    const hdrTexture = new HDRCubeTexture(
+        scenario.lighting, this[$scene], 256, false, false, false);
+    this[$scene].environmentTexture = hdrTexture;
+    // rotate both skybox and hdr texture for 180 deg to match other renderers


I might say skybox and environment, to match model-viewer terminology.

elalish · 2020-06-24T22:45:25Z

packages/render-fidelity-tools/src/components/renderers/babylon-viewer.ts

+
+    this[$scene].executeWhenReady(() => {
+      requestAnimationFrame(() => {
+        requestAnimationFrame(() => {


Was one raf not enough?

i just copied this code from filament-viewer.ts, here is the origin code. I actually don't understand why need two rafs
// Wait two rAFs to ensure we rendered at least once:
requestAnimationFrame(() => {
requestAnimationFrame(() => {
this.dispatchEvent(
new CustomEvent('model-visibility', {detail: {visible: true}}));
});
});

i tried one raf and both update screenshots and fidelity test are successful. So i will just change it to one raf, is that ok?

Sounds good.

elalish · 2020-06-24T22:46:19Z

packages/render-fidelity-tools/test/renderers/babylon/index.html

+<head>
+ <meta charset="UTF-8">
+ <meta name="viewport" content="width=device-width, minimum-scale=1.0, initial-scale=1.0, user-scalable=yes">
+ <title>Filament Fidelity Tests</title>


That's silly, thank you for finding that!

…into include-babylon-renderer-page

elalish

Thanks, looks great! Next thing will be to use Babylon's environment prefiltering so these look halfway decent.

sun765 added 14 commits June 16, 2020 19:02

installed babylon and updated package-lock

b699429

add babylon-viewer into rollup build options

3da3e2c

create babylon's index file and ts file, nothing looks wrong

bc6bdc8

set up camera, scene and engine

5b8d1fb

babylon demo scene working!

31925df

update canvas size

ab126ee

installed babylon/loader, nothing looks wrong

a92e50d

working on importing models

9778cfc

model seems import correctly, but it's not shown

fcd8c03

model loads correctly from different scene, two things to fix: 1. cl…

6ee4083

…ear model when switching scene 2. model's transform 3. loading model's reject call back

added hdr to environment testure

f9f0983

set up config coordinate system

447515c

ready to make pr

04bd357

pull master, upadte package-lock

1a39e48

sun765 commented Jun 18, 2020

View reviewed changes

sun765 requested a review from elalish June 18, 2020 16:22

elalish reviewed Jun 18, 2020

View reviewed changes

bejado reviewed Jun 18, 2020

View reviewed changes

sun765 added 10 commits June 18, 2020 17:14

config hdr texture and skybox

2bedd57

correct the coordinate mistake

1402823

update screenshots fail at filament

82815e1

finished render babylon's goldens

64707d2

fixed screen shot delay problem

776e4f9

Merge branch 'master' of https://github.com/google/model-viewer into …

7c1e33f

…include-babylon-renderer-page

excluded failed scenarios

ff4fa5d

fixed clip distance problem, but the skybox is gone

4b9900b

fixed wrong x direction problem

e062181

find a workaround for confliction between skybox and near far clip

07ee0bd

sun765 added 4 commits June 23, 2020 13:05

refractored hdr texture

afc62fa

refractored code

02b6847

removed all commited goldens, removed far clip

d3303d9

Merge branch 'master' of https://github.com/google/model-viewer into …

d09d168

…include-babylon-renderer-page

sun765 force-pushed the include-babylon-renderer-page branch from 66744e6 to d09d168 Compare June 23, 2020 19:55

sun765 added 3 commits June 23, 2020 16:06

reset goldens to initial state

5241725

correct licenses

b4a3cac

add comments to why not setting far clip

d2424ff

sun765 requested a review from elalish June 23, 2020 20:22

prideout previously approved these changes Jun 24, 2020

View reviewed changes

elalish dismissed prideout’s stale review via 7719d38 June 24, 2020 16:21

elalish force-pushed the include-babylon-renderer-page branch from 7719d38 to 5cd6fd5 Compare June 24, 2020 17:13

elalish requested changes Jun 24, 2020

View reviewed changes

sun765 added 3 commits June 24, 2020 19:51

ready to pr

ac49de7

removed double raf

cf7f771

add updated goldens

2c8be0f

sun765 force-pushed the include-babylon-renderer-page branch from 5cd6fd5 to 2c8be0f Compare June 25, 2020 16:32

sun765 and others added 4 commits June 25, 2020 13:09

removed all unused callbacks

5ec450c

update goldens

c1dc759

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

13ad5e9

…into include-babylon-renderer-page

reverting accidental reversion

7c09e51

elalish approved these changes Jun 25, 2020

View reviewed changes

sun765 merged commit 7a5f745 into master Jun 25, 2020

elalish deleted the include-babylon-renderer-page branch September 15, 2020 23:05

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

initial commit on including babylonjs in renderers' page #1247 #1298

initial commit on including babylonjs in renderers' page #1247 #1298

sun765 commented Jun 18, 2020 •

edited

sun765 Jun 18, 2020

elalish Jun 18, 2020

sun765 Jun 18, 2020

sun765 Jun 18, 2020

elalish left a comment

elalish Jun 18, 2020

sun765 Jun 18, 2020

elalish Jun 18, 2020

sun765 Jun 18, 2020

elalish Jun 18, 2020

sun765 Jun 18, 2020

elalish Jun 18, 2020

sun765 Jun 18, 2020

elalish Jun 18, 2020

sun765 Jun 18, 2020

elalish Jun 18, 2020

sun765 Jun 18, 2020

bejado Jun 18, 2020

bejado Jun 18, 2020

sun765 Jun 18, 2020

bejado Jun 18, 2020

sun765 Jun 18, 2020

prideout commented Jun 18, 2020

elalish Jun 24, 2020

elalish Jun 24, 2020

sun765 Jun 24, 2020

elalish Jun 24, 2020

elalish Jun 24, 2020

sun765 Jun 24, 2020

sun765 Jun 25, 2020

elalish Jun 25, 2020

elalish Jun 24, 2020

sun765 Jun 24, 2020

elalish left a comment


		import {ScenarioConfig} from '../../common.js';

		// const IS_BINARY_RE = /\.glb$/;

		@@ -0,0 +1,151 @@
		/* @license
		* Copyright 2019 Google LLC. All Rights Reserved.

		const alpha = orbit.phi * Math.PI / 180;
		const beta = orbit.phi * Math.PI / 180;

initial commit on including babylonjs in renderers' page #1247 #1298

initial commit on including babylonjs in renderers' page #1247 #1298

Conversation

sun765 commented Jun 18, 2020 • edited

Reference Issue

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

elalish left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

prideout commented Jun 18, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

elalish left a comment

Choose a reason for hiding this comment

sun765 commented Jun 18, 2020 •

edited