Variants editor #2929

ghost · 2021-10-27T21:42:59Z

Adds Variant selection to Material editor

Display of material list based on variant working Fix issue where selecting a material occurs after dragging.

Adds variant tests.

elalish · 2021-10-28T00:30:15Z

packages/space-opera/src/components/materials_panel/materials_panel.ts

-import '@material/mwc-icon-button';

+import {$gltfIndex, Material} from '@google/model-viewer/lib/features/scene-graph/material';
+import {$primitivesList} from '@google/model-viewer/lib/features/scene-graph/model';


I'd like to consider our editor as built on top of model-viewer, meaning on its public API. If the feature we want in the editor can't be implemented with MV's public API, we should use that as feedback for changing its public API. Can we do this without accessing our private symbols?

This is something that I keep wishing I had when working on other material features, I finally added it here because it makes the whole thing so much cleaner, but I agree in general, I do think this should have already been a thing which is why I felt ok adding it; we have this info just not in as useful a form.

elalish · 2021-10-28T00:34:27Z

packages/space-opera/src/components/materials_panel/materials_panel.ts

-      </me-dropdown>
+      <div slot="content">
+        ${this.renderVariantsSelector()}
+        <me-section-row label="Material">


This all seems to be nicely functional; let's tweak the CSS to make it a little nicer to look at. We can discuss at our next meeting.

No problem, just keep in mind the 'create variant' functionality will be coming in next so that UI should be taken into account as well

elalish · 2021-10-28T00:36:20Z

packages/space-opera/src/components/materials_panel/materials_panel.ts

+    getModelViewer().addEventListener('pointermove', () => drag = true);
+    getModelViewer().addEventListener('pointerup', (event) => {
+      if (!drag) {
+        this.onClick(event);


This looks interesting; care to describe?

This simply prevents selecting a material while dragging the object.

elalish · 2021-10-28T00:37:32Z

packages/space-opera/src/components/materials_panel/materials_panel.ts

+      this.selectableMaterials = Array.from(selectableSet);
+      this.selectableMaterials.sort((a, b) => {
+        return a[$gltfIndex] - b[$gltfIndex];
+      });


Are these not sorted to begin with? I thought we get things back in insertion order?

because the list is based on the order a material is encountered in the list primitives the materials will not be in order. Per our conversation isActive is being added to Material in order to provide the same information publicly without exposing primitive-node

Uses the new API in material_panel to implement variant selection, and to filter materials by active material.

elalish

Looking pretty close, just a few things.

elalish · 2021-10-28T20:58:12Z

packages/model-viewer/src/features/scene-graph/material.ts

    return (this[$sourceObject] as DefaultedMaterial).emissiveFactor;
  }

+  get gltfIndex(): number {


I might just call this index, since our materials index is the same. Also looks like this wasn't added to the API?

Sounds good.
Added.

elalish · 2021-10-28T21:03:04Z

packages/space-opera/src/components/materials_panel/materials_panel.ts



+
+export interface VariantToMaterialMapping {


elalish · 2021-10-28T23:20:54Z

packages/space-opera/src/components/materials_panel/materials_panel.ts

  }

+  get selectedVariant(): string|null {
+    if (!getModelViewer()) {


I think for consistency you can remove this. In fact I think the getModelViewer() return type doesn't even include null/undefined now.

elalish · 2021-10-28T23:22:33Z

packages/space-opera/src/components/materials_panel/materials_panel.ts

  }

+  updateSelectableMaterials() {
+    if (getModelViewer() != null && getModelViewer()!.model != null) {


Elsewhere we just assert these are not null. I think that's fine here since I believe the whole panel gets hidden if there's no model loaded.

elalish · 2021-10-28T23:24:52Z

packages/space-opera/src/components/materials_panel/materials_panel.ts

+      getModelViewer().addEventListener('variant-applied', () => {
+        onVariantApplied();
+        getModelViewer().removeEventListener(
+            'variant-applied', onVariantApplied);


A simpler version of this is getModelViewer().addEventListener('variant-applied', onVariantApplied, { once: true });

elalish · 2021-10-28T23:27:28Z

packages/space-opera/src/test/materials_panel/materials_panel_test.ts

+    await panel.updateComplete;
+
+    const selector = panel.shadowRoot?.getElementById('variant-selector');
+    expect(selector!.id).toEqual('variant-selector');


This is a bit tautological; maybe check that the selector is visible?

elalish · 2021-10-28T23:30:26Z

packages/space-opera/src/components/materials_panel/materials_panel.ts

+      </me-section-row label>
+    `;
+    }
+    return html``;


I've been trying to get away from this style we had a lot of where we would conditionally remove html. It makes a lot of things difficult (you never know if an element exists, etc). Let's instead change the visibility of the element when availableVariants changes; like set display: "none" or add a class that does that. There should be plenty of examples around the code base to follow.

Setting display: none if you could verify that this code looks ok, everything works fine but...

elalish

Almost there, let's just fix the last of the CSS (since this will be public as soon as we click merge).

elalish · 2021-10-29T20:57:14Z

packages/space-opera/src/components/materials_panel/materials_panel.ts

  metallicFactorSlider!: SliderWithInputElement;
  @query('me-dropdown#material-selector') materialSelector!: Dropdown;
+  @query('me-dropdown#variant-selector') variantSelector!: Dropdown;
+  @query('me-dropdown#create-variant-selector')


elalish · 2021-10-29T21:01:47Z

packages/space-opera/src/components/materials_panel/materials_panel.ts

+      <me-section-row
+      label="Variant"
+      style='display: ${
+        getModelViewer().availableVariants.length > 0 ? 'block' : 'none'}'>


This is fine until you add create variant functionality. Then this length will change without updating any @property, which means it won't cause render to run. You can cross that bridge when you come to it. Generally I do CSS changes like this outside of the render function to avoid this, since it's cheaper than calling render more often.

I see, so you can just update CSS and it will respond without having to 're-render' the html?

Yeah gonna leave this here until I have the button, which will provide the hooks to update the CSS display state

elalish · 2021-10-29T21:25:31Z

packages/space-opera/src/components/materials_panel/materials_panel.ts

-                                'Unnamed Material'}</paper-item>`)}
-      </me-dropdown>
+      <div slot="content">
+        <me-section-row>


I think this can just remain unchanged from master, right? Are these divs and rows doing anything useful? Seems to be causing unwanted centering too.

elalish · 2021-10-29T21:26:36Z

packages/space-opera/src/components/materials_panel/materials_panel.ts

+
  renderSelectMaterialTab() {
    return html`
    <me-expandable-tab tabName="Selected Material" .open=${true} .sticky=${


If you open enough tabs under this so it scrolls, you'll see that having a sticky one below a not sticky one is very weird. Let's go with neither is sticky. Hopefully that will make their styles match better too.

Done.
Looks pretty good IMO.

elalish

LGTM, thanks!

SrirachaSource added 8 commits October 12, 2021 15:03

ensure materials are loaded before creating thumbnails

e0d05a2

Add variant selector to material panel

95f7d1d

Variants selector is nested in variants tab

64e0e3c

interim variant editor

b9de9c5

Merge branch 'master' into variant-editor

08e1b09

Variant selection working

d214e7e

Display of material list based on variant working Fix issue where selecting a material occurs after dragging.

rearranged layout of variant and material selectors

ab73d92

Merge branch 'master' into variant-editor

85bb629

ghost requested review from elalish and samaneh-kazemi October 27, 2021 21:43

SrirachaSource added 3 commits October 27, 2021 14:49

remove unused css

ecfb05d

Removed unused dialog

0b3f142

Fixes test failure

e1dc431

Adds variant tests.

elalish suggested changes Oct 28, 2021

View reviewed changes

Adds gltfIndex and isActive to Material API

34d5e43

Uses the new API in material_panel to implement variant selection, and to filter materials by active material.

ghost requested a review from elalish October 28, 2021 19:53

elalish suggested changes Oct 28, 2021

View reviewed changes

Updates per feedback

9fcff63

ghost requested a review from elalish October 29, 2021 20:36

Moves variants into it's own tab

158d28d

elalish suggested changes Oct 29, 2021

View reviewed changes

updates per feedback

83bef33

ghost requested a review from elalish October 29, 2021 22:25

elalish previously approved these changes Oct 29, 2021

View reviewed changes

fix test

469a38e

ghost dismissed elalish’s stale review via 469a38e October 29, 2021 22:39

elalish approved these changes Oct 29, 2021

View reviewed changes

ghost merged commit b1ad269 into master Oct 29, 2021

ghost deleted the variants-editor branch October 29, 2021 22:54

This pull request was closed.

Variants editor #2929

Variants editor #2929

Uh oh!

Conversation

ghost commented Oct 27, 2021

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!

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!

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!

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!

Choose a reason for hiding this comment

Uh oh!

elalish left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone