1023462 - Option to toggle visibility on alive/dead audionodes

Status: RESOLVED INVALID

(DevTools Graveyard :: Web Audio Editor, defect)

Description

[Jordan Santell [:jsantell] [@jsantell]]

Option to show/hide nodes based off of whether or not the node has been GC'd.

This means we would still have to store representations of dead nodes on the front end, just not show them, which could grow in size overtime, as opposed to just removing them once GC'd. It can be beneficial to see all the nodes created over time, regardless of whether or not the node was GC'd.

Maybe instead of a visibility toggle, it'd be a checkbox, "destroy GC'd nodes", which would prevent a large stack growing, but less useful perhaps.

Comment 1

[Jordan Santell [:jsantell] [@jsantell]]

This was +1'd during the web audio hack day in berlin's jsfest -- sometimes the dead nodes hide too quickly to be able to see if the node was ever created or not

Comment 2

[Jordan Santell [:jsantell] [@jsantell]]

Got this working rather easily, but wanted to pick some brains on functionality... ni? :D

Case where you'd want to leave GC'd nodes visible -- relatively simple scene, want to know if a buffer node was created, but was quickly GC'd, hard to tell in fast moving context.

Case where you'd want to leave GC'd nodes invisible -- complex scene with many many nodes created constantly. In fact, I'd wager that the graph is pretty useless due to the size and speed.

Question: Keeping around dead nodes in the tools' memory (just for visualization purposes, not for interacting) can be costly on a busy context. Does it make sense, if toggling this, to just save all FUTURE dead nodes for that context? Could seem weird for someone turning that on and not noticing any immediate changes. But it also seems better than displaying nodes from ages (seconds) past.

Comment 3

[Soledad Penades [:sole] [:spenades]]

Ohh nice!

I'd love to see this. Definitely sometimes it's hard to say if you're doing things right because GC happens too quickly.

Re your question: could you put a limit on how many you show? Of course not hardcoded but in a pref, of course? :P
And at some point start disposing older nodes.

Or just let it be. If they're creating too many nodes and the machine is too slow for all this... well, they called for it! It's like trying to put a limit on how many frames you benchmark for the timeline etc.

Comment 4

[Paul Adenot (:padenot)]

(In reply to Jordan Santell [:jsantell] [@jsantell] from comment #2)
> Question: Keeping around dead nodes in the tools' memory (just for
> visualization purposes, not for interacting) can be costly on a busy
> context. Does it make sense, if toggling this, to just save all FUTURE dead
> nodes for that context? Could seem weird for someone turning that on and not
> noticing any immediate changes. But it also seems better than displaying
> nodes from ages (seconds) past.

Maybe we could have a "Clean" button or something ? Also, I agree that this would only apply to future dead nodes.

Comment 5

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

I think the dead nodes should be faded out.

+1 to a "remove all dead nodes" button.

+1 to only applying to future nodes.

Comment 6

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

Clearly needs more "bring out your dead" buttons.

Comment 7

[Jordan Santell [:jsantell] [@jsantell]]

Attachment: gc-node.gif

Cool -- thanks all for the feedback. If the option to display the dead nodes are on, then going forward, all dead nodes are saved and displayed. When disabling, all dead nodes are destroyed in the view.

Here's a gif of it.

Comment 8

[Jordan Santell [:jsantell] [@jsantell]]

Attachment: 1023462-toggle-display-gc-nodes.patch

this was much larger than anticipated, victor D:

Comment 9

[Jordan Santell [:jsantell] [@jsantell]]

try: https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=7174c59c56f2

Comment 10

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

Comment on attachment 8515342 [details] [diff] [review]
1023462-toggle-display-gc-nodes.patch

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

Sorry for the lag.

::: browser/devtools/webaudioeditor/controller.js
@@ +254,5 @@
> +  observe: function(subject, topic, pref) {
> +    Prefs.refresh();
> +
> +    // Emit the general preference change event.
> +    window.emit(EVENTS.PREF_CHANGE, pref);

Wouldn't it be a good idea to emit the specific pref change in here, instead of as a separate event?

::: browser/devtools/webaudioeditor/test/head.js
@@ +35,5 @@
>  waitForExplicitFinish();
>  
>  let gToolEnabled = Services.prefs.getBoolPref("devtools.webaudioeditor.enabled");
>  
> +Services.prefs.setBoolPref("devtools.webaudioeditor.show-dead-nodes", false);

let gShowDeadNodes = ...

@@ +44,5 @@
>    gDevTools.testing = false;
>    info("finish() was called, cleaning up...");
>    Services.prefs.setBoolPref("devtools.debugger.log", gEnableLogging);
>    Services.prefs.setBoolPref("devtools.webaudioeditor.enabled", gToolEnabled);
> +  Services.prefs.setBoolPref("devtools.webaudioeditor.show-dead-nodes", false);

setBoolPref(..., gShowDeadNodes)

@@ +318,5 @@
>  function click (win, element) {
>    EventUtils.sendMouseEvent({ type: "click" }, element, win);
>  }
>  
> +// Use this click method for XUL buttons

wat

Comment 11

[Jordan Santell [:jsantell] [@jsantell]]

Waiting on this, as Sole found some discrepancies with when the nodes become hidden -- want to work through it in PDX

Comment 12

[Jordan Santell [:jsantell] [@jsantell]]

If this is to land, will have to rethink a whole state of "dead" nodes, as when interacting with a node, it'll make calls on the actual underlying audio node (which no longer exists), so some sort of snapshot state that would have to permeate over everything. Does not sound fun. Will have to think some more.

Comment 13

[Sebastian Zartner [:sebo]]

Bulk changing the status, as there is no assignee anymore.

Sebastian

Comment 14

[BMO Automation]

The code behind DevTools:Web Audio Editor has gone away. Closing this bug as INVALID