Add a few buttons in Remote Scene Tree

[Mickeon]

Implements... closes godotengine/godot-proposals#5267.

This PR adds two buttons to the remote SceneTree displayed in the Editor:

A Scene button to any scene root Node instantiated from file. When clicked, it opens the original PackedScene it comes from and switches off the remote Tree display.

A Visibility Toggle button. Enough said. Clicking on it toggles the remote Node's visibility.

I would like to ask a few questions regarding this PR's implementation:

• I feel like this should be an editor setting. It can potentially be quite annoying to see.
  • Especially the "eyes". The "eyes" are particularly distracting. I only included it because it's part of the proposal. Should they be just outright removed? Or should it all be an editor setting with a few modes to choose from?
• Regarding technical implementation (and possible jank):
  • A new signal open (akin to SceneTreeEditor) has been added to the EditorDebuggerTree , emitted when the Scene button is pressed. However, the all of the "pressed" answering logic is present in EditorDebuggerNode. Because of this, the enum storing the button ids is public, and EditorDebuggerNode emits the signal for the remote tree. This all feels odd to me, but I only did it because other "remote tree manipulating" methods were there. I wouldn't want these classes to be more cryptic than they are already, should the logic be moved to to EditorDebuggerTree?

[akien-mga]

Looks good to me, but I wonder if the Visible icon should actually be functional.

[KoBeWi]

I think making the eye icon functional shouldn't be difficult to do. It could just toggle visible property, the same way as you can do via the remote inspector.

btw why is the instance button gray if it's clickable?

[Mickeon]

From the proposal itself:

Having a grayed-out PackedScene icon (similar to the editor).

The idea probably was to make it less eye-catching, and to further distinguish it from the Local Tree. When I was working on the PR I believed it suited it nicely, too. Perhaps a different colour could suit more, since this symbol is typically associated with them being disabled in the UX, so long as it would stand out less.

[KoBeWi]

From the proposal itself:

The proposal is weird on that one, because it also says (about the eye icon)

It doesn't event to be editable (can be grayed-out)

which implies that the scene button shouldn't be clickable either 🤷‍♂️
I think active icons shouldn't be grayed. Can always be changed later if people don't like it.

When the Scene is changed, the remote tree is toggled off. Is this behaviour expected? Or is it a nuisance?

It's expected. When you inspect scene, you want to see it.

[Mickeon]

I'm trying hard to figure out what to send to the Debugger to have the visibility toggle, to absolutely no avail. Something about the RemoteDebugger, peers, messages... For now I could rip the eye icon out of this PR to have something complete, but I don't know if I should...

[KoBeWi]

Grayed-out unclickable icon is ok for now.

[Mickeon]

Updated the PR to no longer use the tooltip workaround.
The icons now look like this:

The scene icon is slightly more transparent still but not to the point where it looks deactivated, just like the eye icon is going to be, until I may figure it out someday.

[Mickeon]

I managed to make the Visibility Toggle work. Very happy about this.

[Faless]

The PR looks good overall, but the serialization is really messy.
Please do not use a Dictionary. I suggest the following:

struct RemoteNode {
		int child_count = 0;
		String name;
		String type_name;
		ObjectID id;
		String scene_file_path; // Empty when not present
		int visible = 0; // 0 = no "is_visibile", 1 = "is_visible", -1 = NOT "is_visible"
		bool visible_in_tree = false;
};

[Mickeon]

That's alright! The less code smells the merrier. However, I feel like I cannot use anything that isn't a Variant for this, because it needs to be passed around, serialised and deserialized in SceneDebuggerTree.
https://github.com/godotengine/godot/blob/1bd96c050325c6f545af8e6e22c7dd0fa1d06a49/scene/debugger/scene_debugger.cpp#L589-L599
At worst, it may be its own Object. Could that work?

[Calinou]

At worst, it may be its own Object. Could that work?

Object types come with significant overhead; see #61273.

[Mickeon]

Ok, no, Yeah... So what is the idea? How to improve the code quality, without having to reconsider how data is passed between running project and editor just for this PR?

Perhaps I could still serialize it as Dictionary? Then when passed to the editor it could be converted to the struct?

[KoBeWi]

Looks ok on the editor side.
Not sure about the serialization problem.

[Faless]

Perhaps I could still serialize it as Dictionary? Then when passed to the editor it could be converted to the struct?

No, I was suggesting you use a string, an int, and a boolean.

 	int idx = 0; 
 	while (p_arr.size() > idx) { 
 		ERR_FAIL_COND(p_arr.size() < 7); 
 		CHECK_TYPE(p_arr[idx], INT); // "child_count" 
 		CHECK_TYPE(p_arr[idx + 1], STRING); // "name". 
 		CHECK_TYPE(p_arr[idx + 2], STRING); // "type_name". 
 		CHECK_TYPE(p_arr[idx + 3], INT); // "id". 
 		CHECK_TYPE(p_arr[idx + 4], STRING); // "scene_file_path". 
 		CHECK_TYPE(p_arr[idx + 5], INT); // "visible". 
 		CHECK_TYPE(p_arr[idx + 6], BOOL); // "is_visible_in_tree". 
 		nodes.push_back(RemoteNode(p_arr[idx], p_arr[idx + 1], p_arr[idx + 2], p_arr[idx + 3], p_arr[idx + 4], p_arr[idx + 5], p_arr[idx + 6])); 
 		idx += 7; 
  	}

[KoBeWi]

CHECK_TYPE(p_arr[idx + 5], INT); // "visible".

Why int?

[Faless]

Why int?

My understanding was that we needed a tri-state for it (i.e. has visibility method, visible = true, and visible = false), so like mentioned above: int visible = 0; // 0 = no "is_visibile" method, 1 = "is_visible = true", -1 = NOT "is_visible = false" (or any combination of the above).
But if that's not the case (i.e. we don't need to know if the node has the is_visible method at all), then a bool is enough.

[KoBeWi]

Or we could go further and reuse the same int for is_visible_in_tree() 🤔
-1 = no method
0 = not visible
1 = visible
2 = visible in tree
3 = visible and visible in tree

[Faless]

Or we could go further and reuse the same int for is_visible_in_tree()

I'd prefer the 3-state solution, so you could do

if (visibility) {
  bool is_visible = visibility > 0;
  // logic to add visible/visible_in_tree icon.
}

I'd rather use 3 bools in place of a 5-state int without a proper enum (or use an enum, but then you need casting and it gets a bit more complicated than it needs to be I think).

[Faless]

I'd rather use 3 bools in place of a 5-state int without a proper enum (or use an enum, but then you need casting and it gets a bit more complicated than it needs to be I think).

Though I guess using an enum and a couple of casts is also okay actually.

[Mickeon]

So be it, then. If it's no bother to add all of those parameters, it can be done similarly to that way. Hopefully it doesn't have much of any memory drawbacks, other than having to expand onto it if more properties are going to be necessary. I can get to it in these upcoming days.

[Mickeon]

Updated this PR based on the small discussion above. Another review is greatly appreciated! @Faless

[Faless]

@Mickeon Please apply the following patch: fixes.txt (diff file) then it should be good to go.

You can apply the patch with: patch -p1 < fixes.txt (or via your favorite diff viewer).

[Mickeon]

Thank you for the kind patching! Applied successfully, and verified that it didn't cause any behavior unintended by me.

[akien-mga]

Thanks!