model-viewer icon indicating copy to clipboard operation
model-viewer copied to clipboard

Changing 'reveal' after a model load breaks rendering and interaction.

Open hjeldin opened this issue 3 years ago • 1 comments

Description

Switching between reveal='interaction' and reveal='auto' after a model has been loaded forces an updateSource that breaks rendering and interaction.

image

I expected that switching form interaction to auto and vice versa after a model has been loaded should not result in any side effect, and the parameter should only be considered when the model src has changed.

I tried removing changedProperties.has('reveal') in packages/model-viewer/src/features/loading.ts:391 and it seems to work fine when changing the reveal property. It also works correctly when switching between models by changing the src property.

The issue is especially visible if you zoom-in the model before the first reveal change. I tested only on my workstation but i expect the bug (and the fix) to be reproducible across platforms.

Here's the diff:

diff --git a/packages/model-viewer/src/features/loading.ts b/packages/model-viewer/src/features/loading.ts
index 94a97b1..cb5f6c2 100644
--- a/packages/model-viewer/src/features/loading.ts
+++ b/packages/model-viewer/src/features/loading.ts
@@ -388,7 +388,7 @@ export const LoadingMixin = <T extends Constructor<ModelViewerElementBase>>(
             'aria-label', this[$altDefaulted]);
       }
 
-      if (changedProperties.has('reveal') || changedProperties.has('loading')) {
+      if (changedProperties.has('loading')) {
         this[$updateSource]();
       }

Let me know if you prefer a PR, i avoided it only because it's a one-line change.

Live Demo

https://glitch.com/edit/#!/gusty-spangle-neon Version with issues: https://gusty-spangle-neon.glitch.me/ Version with my fix: https://gusty-spangle-neon.glitch.me/correct.html

Version

  • model-viewer: v1.12.0

Browser Affected

  • [X] Chrome
  • [ ] Edge
  • [X] Firefox
  • [ ] Safari

OS

  • [ ] Android
  • [ ] iOS
  • [X] Linux
  • [ ] MacOS
  • [ ] Windows

AR

  • [ ] WebXR
  • [ ] SceneViewer
  • [ ] QuickLook

hjeldin avatar May 26 '22 08:05 hjeldin

A PR would be ideal! Thanks, just got back.

elalish avatar Jun 03 '22 18:06 elalish