Bug 174176 – Web Inspector: [META] Add 3D layer visualization

• Component: Web Inspector
• Status: RESOLVED FIXED

[Ross Kirsling]

Add 3D visualization for compositing layers to Web Inspector.

(Sony demonstrated this functionality as a standalone webapp at the 2016 WebKit Contributors' Meeting and it was well received.)

[Ross Kirsling]

Created attachment 314656 [details]
Visualizer presented at WKCM 2016

Here's a screenshot of the tool that was demonstrated.

Current implementation uses THREE.js -- I assume we'll need to eliminate this dependency.

UI/UX will require discussion but presumably this will be launched from LayerTreeDetailsSidebarPanel.

[Joseph Pecoraro]

> Current implementation uses THREE.js -- I assume we'll need to eliminate
> this dependency.

We use libraries where applicable! See Source/WebInspectorUI/UserInterface/External.

   - CodeMirror - Text Editor
     - used heavily for all our text content views, even the console
   - Esprima - JavaScript Parser
     - used for pretty printing, type profiler, and more
   - ESLint - JavaScript Static Analysis
     - currently unused

Deciding whether or not we include a (compatibly licensed) 3rd party library is mainly based on its usefulness to the project. If using THREE.js is a big win versus not using it, then sure lets include it. If it provides non-trivial features or would significantly improve readability / performance than it seems naturally useful. Likewise if we think we might grow to use it in more places, or would naturally want to use it later on for other purposes that helps.

As an example. With the Timeline graphs I started using d3.js but switched to just generating manually generating a SVG. Given the simplicity of the UI (just a few polygons / paths) using a full library felt like overkill. It also would mean anytime someone wanted to make changes they would have to learn d3's idioms which even I didn't get fully acquainted with.

[Don Olmstead]

(In reply to Joseph Pecoraro from comment #2)
> > Current implementation uses THREE.js -- I assume we'll need to eliminate
> > this dependency.
> 
> We use libraries where applicable! See
> Source/WebInspectorUI/UserInterface/External.
> 
>    - CodeMirror - Text Editor
>      - used heavily for all our text content views, even the console
>    - Esprima - JavaScript Parser
>      - used for pretty printing, type profiler, and more
>    - ESLint - JavaScript Static Analysis
>      - currently unused
> 
> Deciding whether or not we include a (compatibly licensed) 3rd party library
> is mainly based on its usefulness to the project. If using THREE.js is a big
> win versus not using it, then sure lets include it. If it provides
> non-trivial features or would significantly improve readability /
> performance than it seems naturally useful. Likewise if we think we might
> grow to use it in more places, or would naturally want to use it later on
> for other purposes that helps.
> 
> As an example. With the Timeline graphs I started using d3.js but switched
> to just generating manually generating a SVG. Given the simplicity of the UI
> (just a few polygons / paths) using a full library felt like overkill. It
> also would mean anytime someone wanted to make changes they would have to
> learn d3's idioms which even I didn't get fully acquainted with.

At this point its just drawing quads so to me there's not a reason to use Three.js. It just made the initial implementation easier. I would assume it would be like your experience with d3.js.

[Joseph Pecoraro]

> At this point its just drawing quads so to me there's not a reason to use
> Three.js. It just made the initial implementation easier. I would assume it
> would be like your experience with d3.js.

Okay, yeah it sounds like we could avoid it in this case then.

[Nikita Vasilyev]

I think a 3D visualization for compositing layers is something worth doing.

(In reply to Ross Kirsling from comment #1)
> UI/UX will require discussion but presumably this will be launched from
> LayerTreeDetailsSidebarPanel.

I'd suggest making a new Layers tab, closed by default, and linking to it from Element -> Layers sidebar.

[Ross Kirsling]

(In reply to Joseph Pecoraro from comment #2)
> We use libraries where applicable!

Glad to hear that they're not out of the question. :D


(In reply to Don Olmstead from comment #3)
> At this point its just drawing quads so to me there's not a reason to use Three.js.

I think we also need the rotate/zoom/pan controls, but hopefully the conclusion is the same.

(In reply to Nikita Vasilyev from comment #5)
> I'd suggest making a new Layers tab, closed by default, and linking to it
> from Element -> Layers sidebar.

Sounds good to me!

[Ross Kirsling]

Created attachment 315868 [details]
WIP Patch (three.js omitted)

WIP Patch

Current state:
- Introduces a LayerViewer tab into Web Inspector.
- Simply displays a fixed-size canvas visualizing the current layer tree.
- Uses three.js (omitted from the patch to avoid noise).
- Camera is centered on the visible part of the page, zoom and pan are disabled, rotation is constrained so that it doesn't go behind the scene.

Things to do:
- Sufficiently improve UX for initial landing.
- Attempt to remove three.js.

Some questions:
- What should this tab be called? "LayerViewer" isn't ideal, but then the sidebar is already called Layers...
- Should the visualization have fixed dimensions or fill its parent element? The former would be consistent with the memory graphs and could be more behaviorally stable; the latter would avoid having a truncated visualization in a large Inspector window.
- This tab should be launchable from the Layers sidebar, but should it retain the Layers sidebar itself? If so, is "supplementalRepresentedObjects" the right way to feed the body element from the ContentView to the sidebar?

[Ross Kirsling]

Created attachment 315869 [details]
WIP Screenshot #1

[Ross Kirsling]

Created attachment 316485 [details]
WIP Patch #2

Updated the WIP patch based on IRC feedback.

Changes:
- Call tab "Layers" (on the understanding that this tab could become the new home for the layer tree data grid).
- Put Layers tab behind an experimental feature flag.
- Make the visualization fill its parent element and resize along with the Web Inspector window.

Next steps:
- Try adding layer tree data grid below the visualization, such that interacting with either part of the split view updates the other.

Let me know if it would be appropriate to submit a piece of this as a first patch to land, now that it's behind an experimental feature flag (though I suppose everything hinges on the legal approval for three.js).

[Devin Rousso]

Comment on attachment 316485 [details]
WIP Patch #2

View in context: https://bugs.webkit.org/attachment.cgi?id=316485&action=review

> Source/WebInspectorUI/UserInterface/Views/Layers3DContentView.js:37
> +    static _createLayerMesh(rect, index, isOutline = false)

This function doesn't look like it needs to be static.  You can just make it a regular private function.

> Source/WebInspectorUI/UserInterface/Views/Layers3DContentView.js:39
> +        const Z_INTERVAL = 25;

Style: lowercase

> Source/WebInspectorUI/UserInterface/Views/Layers3DContentView.js:45
> +        const geometry = new THREE.Geometry();
> +        geometry.vertices.push(new THREE.Vector3(rect.x,              -rect.y,               index * Z_INTERVAL));
> +        geometry.vertices.push(new THREE.Vector3(rect.x + rect.width, -rect.y,               index * Z_INTERVAL));
> +        geometry.vertices.push(new THREE.Vector3(rect.x + rect.width, -rect.y - rect.height, index * Z_INTERVAL));
> +        geometry.vertices.push(new THREE.Vector3(rect.x,              -rect.y - rect.height, index * Z_INTERVAL));

We typically only use `const` for variables that won't be different for any given invocation of a function (such as zInterval).  Also, we don't usually add spacing like this, although I'm not against this personally :P

> Source/WebInspectorUI/UserInterface/Views/Layers3DContentView.js:54
> +        const material = new THREE.MeshBasicMaterial({color: "hsl(76, 49%, 75%)", transparent: true, opacity: 0.4, side: THREE.DoubleSide});

Style: this object has enough properties that it might be worth separating on new lines.  Up to you though.

> Source/WebInspectorUI/UserInterface/Views/Layers3DContentView.js:60
> +    shown()

This should be under Protected.

> Source/WebInspectorUI/UserInterface/Views/Layers3DContentView.js:67
> +    hidden()

Ditto.

> Source/WebInspectorUI/UserInterface/Views/Layers3DContentView.js:69
> +        super.hidden();

NIT: I tend to put `super.hidden()` last just in case a parent class does something weird to the element.  It looks like it's fine here.

> Source/WebInspectorUI/UserInterface/Views/Layers3DContentView.js:81
> +        this._setup();
> +        this._animate();

NIT: I think you can just move the contents of both of these functions to initialLayout, but I don't feel too strongly so it's up to you.

> Source/WebInspectorUI/UserInterface/Views/Layers3DContentView.js:93
> +            WebInspector.layerTreeManager.layersForNode(node, (layerForNode, childLayers) => {
> +                this._clearLayers();
> +                for (let i = 0; i < childLayers.length; i++)
> +                    this._addLayer(childLayers[i], i);
> +            });

Adding this to layout() means that anytime the view changes (such as a resize) you are fetching the layer information from the backend.  I'm not sure we want to do that.  It might be better to put this in its own function, which is called in shown() and _layerTreeDidChange.

> Source/WebInspectorUI/UserInterface/Views/Layers3DContentView.js:116
> +        this._renderer = new THREE.WebGLRenderer({antialias: true});

Please add all of these member variables as null in the constructor.  It helps with readability.

    this._renderer = null;
    this._camera = null;
    this._controls = null;
    this._boundsGroup = null;
    this._compositedBoundsGroup = null;
    this._scene = null;

> Source/WebInspectorUI/UserInterface/Views/Layers3DContentView.js:129
> +        this._controls.maxAzimuthAngle =  Math.PI / 2;

Style: extra space.

> Source/WebInspectorUI/UserInterface/Views/Layers3DContentView.js:145
> +        requestAnimationFrame(() => { this._animate(); });

We should cancelAnimationFrame whenever hidden() is called, as we don't need to be rendering when the view is not visible.  Consequently, this should first start animating in shown(), not initialLayout().

> Source/WebInspectorUI/UserInterface/Views/Layers3DContentView.js:162
> +        const compositedBounds = {x: layer.bounds.x, y: layer.bounds.y, width: layer.compositedBounds.width, height: layer.compositedBounds.height};

Style: put each of the keys on a new line:

    let compositedBounds = {
        x: layer.bounds.x,
        y: layer.bounds.y,
        width: layer.compositedBounds.width, 
        height: layer.compositedBounds.height,
    };

> Source/WebInspectorUI/UserInterface/Views/LayersTabContentView.js:26
> +WebInspector.LayersTabContentView = class LayersTabContentView extends WebInspector.ContentBrowserTabContentView

Extending ContentBrowserTabContentView means that you get a bunch of extra UI (sidebars, back/forward, navigation bar, etc.) that it doesn't look like you need here.  I think you should just extend from TabContentView (see NewTabContentView for an example).

> Source/WebInspectorUI/UserInterface/Views/LayersTabContentView.js:28
> +    constructor(identifier)

This is an old style of constructing.  You can drop the `identifier`.

> Source/WebInspectorUI/UserInterface/Views/LayersTabContentView.js:35
> +

// Static.

> Source/WebInspectorUI/UserInterface/Views/LayersTabContentView.js:52
> +    static shouldSaveTab()
> +    {
> +        return false;
> +    }

Is there a reason that we don't want to save this tab?  In that case, do we also want to hide it in the New Tab view (isEphemeral)?

[Ross Kirsling]

(In reply to Devin Rousso from comment #10)

> > Source/WebInspectorUI/UserInterface/Views/Layers3DContentView.js:60
> > +    shown()
> 
> This should be under Protected.

I can't tell whether this is true or not... 🤔 ContentView itself doesn't specify anything as Protected and it looks like shown/hidden are only called from other classes. Admittedly, there are a few derived classes calling it Protected though. I'm happy to do whichever.

> > Source/WebInspectorUI/UserInterface/Views/LayersTabContentView.js:26
> > +WebInspector.LayersTabContentView = class LayersTabContentView extends WebInspector.ContentBrowserTabContentView
> 
> Extending ContentBrowserTabContentView means that you get a bunch of extra
> UI (sidebars, back/forward, navigation bar, etc.) that it doesn't look like
> you need here.  I think you should just extend from TabContentView (see
> NewTabContentView for an example).

To be sure, this is only used for the this.contentBrowser.showContentView call at the moment, but I figure we'll need the extra functionality soon, if we intend to have the visualization end up as one part of a split view? I suppose we could remove it until it becomes necessary though.

> > Source/WebInspectorUI/UserInterface/Views/LayersTabContentView.js:28
> > +    constructor(identifier)
> 
> This is an old style of constructing.  You can drop the `identifier`.

Everything that extends from ContentBrowserTabContentView or TabContentView seems to write those lines in the same way, so I followed suit...and yet, these are all being instantiated without arguments, so you must be right!

> > Source/WebInspectorUI/UserInterface/Views/LayersTabContentView.js:52
> > +    static shouldSaveTab()
> > +    {
> > +        return false;
> > +    }
> 
> Is there a reason that we don't want to save this tab?  In that case, do we
> also want to hide it in the New Tab view (isEphemeral)?

I did this to achieve the "closed by default" behavior that Nikita suggested, but given that this tab is feature-flagged and the idea for it has changed a little, I think this can probably go away.

[Ross Kirsling]

> > Source/WebInspectorUI/UserInterface/Views/Layers3DContentView.js:93
> > +            WebInspector.layerTreeManager.layersForNode(node, (layerForNode, childLayers) => {
> > +                this._clearLayers();
> > +                for (let i = 0; i < childLayers.length; i++)
> > +                    this._addLayer(childLayers[i], i);
> > +            });
> 
> 
> Adding this to layout() means that anytime the view changes (such as a resize) you are fetching the layer information from the backend.  I'm not sure we want to do that.  It might be better to put this in its own function, which is called in shown() and _layerTreeDidChange.

Most of my "where should I put things" decisions here were a direct mimicry of LayerTreeDetailsSidebarPanel, though admittedly that view is making DOM updates while this one's just redrawing on the canvas.

Upon experimenting, it appears that fetching layer data on every _layerTreeDidChange is far more dangerous, since that fires MANY times when, e.g., scrolling down a page. By having _layerTreeDidChange simply report needsLayout, most of those fetches are avoided.

If nothing else though, we can and should early out when our layoutReason is Resize.

[Ross Kirsling]

Created attachment 316677 [details]
WIP Patch #3

Addressed review feedback.

Note: I did attempt to extend from TabContentView instead of ContentBrowserTabContentView, but this ends up being rather laborious due to having to call the ContentView hooks for the visualization manually, copy-paste some assumed CSS, etc. Not sure if this is worth it if we expect to ultimately extend from ContentBrowserTabContentView in the end anyway.

[Ross Kirsling]

Created attachment 316989 [details]
WIP Patch #4

Updated patch to account for "WI" namespace and new Experimental settings panel.

[Ross Kirsling]

I think we've now reached the feature set that I was envisioning for a "Day 1 MVP", so to speak. Perhaps we could consider/discuss enabling this for real?

[Ross Kirsling]

Going to finally close this, now that the blog post has been published.
Further development need not depend on this meta bug.

[Don Olmstead]

🎊🎊🎊🎊