Skip to content

Commit

Permalink
Fix render timing (google#2331)
Browse files Browse the repository at this point in the history 
* added failing tests

* fixed rendering timing
  • Loading branch information
@elalish
elalish authored May 4, 2021
1 parent 28549c0 commit f485800
Show file tree
Hide file tree
Showing 4 changed files with 76 additions and 43 deletions.
6 changes: 2 additions & 4 deletions packages/model-viewer/src/features/loading.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -360,10 +360,8 @@ export const LoadingMixin = <T extends Constructor<ModelViewerElementBase>>(
`${this[$ariaLabel]}. ${this[$ariaLabelCallToAction]}`);
}

if (changedProperties.has('reveal') || changedProperties.has('loaded')) {
if (!this[$sceneIsReady]()) {
this[$updateSource]();
}
if (changedProperties.has('reveal') || changedProperties.has('loading')) {
this[$updateSource]();
}
}

Expand Down
29 changes: 15 additions & 14 deletions packages/model-viewer/src/model-viewer-base.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -39,12 +39,12 @@ const $template = Symbol('template');
const $fallbackResizeHandler = Symbol('fallbackResizeHandler');
const $defaultAriaLabel = Symbol('defaultAriaLabel');
const $resizeObserver = Symbol('resizeObserver');
const $intersectionObserver = Symbol('intersectionObserver');
const $clearModelTimeout = Symbol('clearModelTimeout');
const $onContextLost = Symbol('onContextLost');
const $loaded = Symbol('loaded');

export const $updateSize = Symbol('updateSize');
export const $intersectionObserver = Symbol('intersectionObserver');
export const $isElementInViewport = Symbol('isElementInViewport');
export const $announceModelVisibility = Symbol('announceModelVisibility');
export const $ariaLabel = Symbol('ariaLabel');
Expand Down Expand Up @@ -249,20 +249,21 @@ export default class ModelViewerElementBase extends UpdatingElement {
if (HAS_RESIZE_OBSERVER) {
// Set up a resize observer so we can scale our canvas
// if our <model-viewer> changes
this[$resizeObserver] = new ResizeObserver((entries) => {
// Don't resize anything if in AR mode; otherwise the canvas
// scaling to fullscreen on entering AR will clobber the flat/2d
// dimensions of the element.
if (this[$renderer].isPresenting) {
return;
}
this[$resizeObserver] =
new ResizeObserver((entries: Array<ResizeObserverEntry>) => {
// Don't resize anything if in AR mode; otherwise the canvas
// scaling to fullscreen on entering AR will clobber the flat/2d
// dimensions of the element.
if (this[$renderer].isPresenting) {
return;
}

for (let entry of entries) {
if (entry.target === this) {
this[$updateSize](entry.contentRect);
}
}
});
for (let entry of entries) {
if (entry.target === this) {
this[$updateSize](entry.contentRect);
}
}
});
}

if (HAS_INTERSECTION_OBSERVER) {
Expand Down
73 changes: 50 additions & 23 deletions packages/model-viewer/src/test/three-components/Renderer-spec.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -14,77 +14,104 @@
*/

import {USE_OFFSCREEN_CANVAS} from '../../constants.js';
import ModelViewerElementBase, {$canvas, $getModelIsVisible, $onResize, $renderer, $scene, $userInputElement} from '../../model-viewer-base.js';
import {LoadingMixin} from '../../features/loading.js';
import ModelViewerElementBase, {$canvas, $intersectionObserver, $isElementInViewport, $onResize, $renderer, $scene, $userInputElement} from '../../model-viewer-base.js';
import {ModelScene} from '../../three-components/ModelScene.js';
import {Renderer} from '../../three-components/Renderer.js';
import {waitForEvent} from '../../utilities.js';
import {assetPath} from '../helpers.js';

const expect = chai.expect;

const ModelViewerElement = class extends ModelViewerElementBase {
const ModelViewerElement = class extends LoadingMixin
(ModelViewerElementBase) {
static get is() {
return 'model-viewer-renderer';
}
};

customElements.define('model-viewer-renderer', ModelViewerElement);

async function createScene(): Promise<ModelScene> {
function createScene(): ModelScene {
const element = new ModelViewerElement();
document.body.insertBefore(element, document.body.firstChild);
const sourceLoads = waitForEvent(element, 'load');
element[$intersectionObserver]!.unobserve(element);
element[$isElementInViewport] = false;

element.src = assetPath('models/Astronaut.glb');
element[$getModelIsVisible] = () => {
return true;
};

// manual render loop
element[$renderer].threeRenderer.setAnimationLoop(null);
await sourceLoads;

return element[$scene];
}

function disposeScene(scene: ModelScene) {
const {element} = scene;
if (element.parentNode != null) {
element.parentNode.removeChild(element);
}
}

suite('Renderer', () => {
let scene: ModelScene;
let otherScene: ModelScene;
let renderer: Renderer;

setup(async () => {
setup(() => {
renderer = Renderer.singleton;
// Ensure tests are not rescaling
ModelViewerElement.minimumRenderScale = 1;
scene = await createScene();
scene = createScene();
otherScene = createScene();
});

teardown(() => {
const {element} = scene;
if (element.parentNode != null) {
element.parentNode.removeChild(element);
}
disposeScene(scene);
disposeScene(otherScene);
renderer.render(performance.now());
});

suite('render', () => {
let otherScene: ModelScene;
test('pre-renders eager, invisible scenes', async () => {
const sourceLoads = waitForEvent(scene.element, 'load');
(scene.element as any).loading = 'eager';
await sourceLoads;

renderer.render(performance.now());
expect(scene.renderCount).to.be.equal(1, 'scene first render');
expect(otherScene.renderCount).to.be.equal(0, 'otherScene first render');
});

suite('with two loaded scenes', () => {
setup(async () => {
otherScene = await createScene();
const sceneVisible = waitForEvent(scene.element, 'poster-dismissed');
const otherSceneVisible =
waitForEvent(otherScene.element, 'poster-dismissed');
scene.element[$isElementInViewport] = true;
otherScene.element[$isElementInViewport] = true;
await Promise.all([sceneVisible, otherSceneVisible]);
});

teardown(() => {
const {element} = otherScene;
if (element.parentNode != null) {
element.parentNode.removeChild(element);
}
test('renders only dirty scenes', () => {
renderer.render(performance.now());
expect(scene.renderCount).to.be.equal(1, 'scene first render');
expect(otherScene.renderCount).to.be.equal(1, 'otherScene first render');

scene.isDirty = true;
renderer.render(performance.now());
expect(scene.renderCount).to.be.equal(2, 'scene second render');
expect(otherScene.renderCount).to.be.equal(1, 'otherScene second render');
});

test('renders only dirty scenes', () => {
test('renders only visible scenes', () => {
renderer.render(performance.now());
expect(scene.renderCount).to.be.equal(1, 'scene first render');
expect(otherScene.renderCount).to.be.equal(1, 'otherScene first render');

scene.isDirty = true;
otherScene.isDirty = true;
otherScene.element[$isElementInViewport] = false;

renderer.render(performance.now());
expect(scene.renderCount).to.be.equal(2, 'scene second render');
expect(otherScene.renderCount).to.be.equal(1, 'otherScene second render');
Expand Down
11 changes: 9 additions & 2 deletions packages/model-viewer/src/three-components/Renderer.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -397,14 +397,18 @@ export class Renderer extends EventDispatcher {
const {dpr, scaleFactor} = this;

for (const scene of this.orderedScenes()) {
const {element} = scene;
if (!element.modelIsVisible && scene.renderCount > 0) {
continue;
}

this.preRender(scene, t, delta);

if (!scene.isDirty) {
continue;
}
++scene.renderCount;

if (!scene.element.modelIsVisible && !this.multipleScenesVisible) {
if (!element.modelIsVisible && !this.multipleScenesVisible) {
// Here we are pre-rendering on the visible canvas, so we must mark the
// visible scene dirty to ensure it overwrites us.
for (const visibleScene of this.scenes) {
Expand Down Expand Up @@ -448,6 +452,9 @@ export class Renderer extends EventDispatcher {
}

scene.isDirty = false;
if (element.loaded) {
++scene.renderCount;
}
}
}

Expand Down

0 comments on commit f485800

Please sign in to comment.