852353 - Allow Tilt to support positioning elements at arbitrary depths and with arbitrary depths

Status: RESOLVED FIXED

(Firefox Graveyard :: Developer Tools: 3D View, defect)

Description

[Dave Townsend [:mossop]]

Currently Tilt is made to work with elements appearing only at fixed offsets of STACK_THICKNESS and with elements that are always the same depth. This patch will also open the way for extensions to extend Tilt though we may want to do something nicer if its something we want to support properly.

Comment 1

[Dave Townsend [:mossop]]

Attachment: WIP

Comment 2

[Dave Townsend [:mossop]]

Comment on attachment 726455 [details] [diff] [review]
WIP

This passes try runs. The main difference that these changes makes is changing traverse to be a recursive function, the code is simpler this way but it could be switched back to an iterative function if needed. It also returns the nodes in a different order to previously. I didn't see any need to maintain that order but if I'm mistaken I think it's possible too.

Comment 3

[Victor Porof [:vporof][:vp]]

Comment on attachment 726455 [details] [diff] [review]
WIP

Review of attachment 726455 [details] [diff] [review]:
-----------------------------------------------------------------

(In reply to Dave Townsend (:Mossop) from comment #2)
> This passes try runs. The main difference that these changes makes is
> changing traverse to be a recursive function, the code is simpler this way
> but it could be switched back to an iterative function if needed. It also

This used to be recursive extravaganza at some point, but I found that on heavy pages it slowed down things a bit too much, and I'd like to keep things as performant as possible, even though these improvements aren't obvious in the majority of pages.

> returns the nodes in a different order to previously. I didn't see any need
> to maintain that order but if I'm mistaken I think it's possible too.

Heh, the idea behind the previous ordering was subtle, but reversing it makes the rendering noticeably slower in some cases. It relied on a thing called depth testing, which makes sure that "if a pixel was drawn, then anything underneath it won't really mater and will be skipped". The previous way the visualization mesh was built made sure that boxes were drawn front-to-back (when looking at the thing from the front) to minimize pixel repaint. it'd be great if this was maintained.

R+ with the above things addressed.

I guess the way an addon would extend Tilt would be to override TUD_getNodePosition? (correct me if I'm wrong). If so, then It'd be a good idea to document this in there.

::: browser/devtools/tilt/TiltUtils.jsm
@@ +402,5 @@
> +   *                  width of the node
> +   *         {Number} height
> +   *                  height of the node
> +   *         {Number} depth
> +   *                  depth of the node

Maybe rename this to "thickness" and "base" to "depth"? Sounds more in line with what we have so far.

@@ +414,5 @@
> +    }
> +
> +    coord.base = aParentPosition ?
> +                 (aParentPosition.base + aParentPosition.depth) :
> +                 0;

Uber nit: I prefer having such things on the same like, even if they exceed 80 chars.

@@ +463,5 @@
>            continue;
>          }
>  
> +        let coord = TiltUtils.DOM.getNodePosition(aContentWindow, node,
> +                                                  aParentPosition);

If you ditch the recursive function you can do this.getNodePosition

::: browser/devtools/tilt/TiltVisualizer.jsm
@@ +872,5 @@
>  
> +    vec3.set([x,     y,     z], highlight.v0);
> +    vec3.set([x + w, y,     z], highlight.v1);
> +    vec3.set([x + w, y + h, z], highlight.v2);
> +    vec3.set([x,     y + h, z], highlight.v3);

Love it!

::: browser/devtools/tilt/TiltWorkerPicker.js
@@ +42,5 @@
>      // the back quad
> +    let v0b = [vertices[i + 24], vertices[i + 25], vertices[i + 26]];
> +    let v1b = [vertices[i + 27], vertices[i + 28], vertices[i + 29]];
> +    let v2b = [vertices[i + 30], vertices[i + 31], vertices[i + 32]];
> +    let v3b = [vertices[i + 33], vertices[i + 34], vertices[i + 35]];

It took me a while to remember how all of this worked.. :) Nicely done.

Comment 4

[Victor Porof [:vporof][:vp]]

s/like/line/ on the uber nit above.

Comment 5

[Dave Townsend [:mossop]]

Attachment: patch

(In reply to Victor Porof [:vp] from comment #3)
> R+ with the above things addressed.
> 
> I guess the way an addon would extend Tilt would be to override
> TUD_getNodePosition? (correct me if I'm wrong). If so, then It'd be a good
> idea to document this in there.

That's the way I used in my extension, it's a little hacky and I think with a little more work I could make something nicer but that will be a separate bug. I added a comment to that method anyway. Also went back to iterative and the old ordering. I'd like a quick final pass just to make sure it looks ok though.

Comment 6

[Victor Porof [:vporof][:vp]]

https://hg.mozilla.org/integration/fx-team/rev/a4742ca5ca32

Comment 7

[Tim Taubert [:ttaubert] (inactive)]

https://hg.mozilla.org/mozilla-central/rev/a4742ca5ca32