Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Make Viewports Resources #3915

Open
madmiraal opened this issue Feb 4, 2022 · 20 comments
Open

Make Viewports Resources #3915

madmiraal opened this issue Feb 4, 2022 · 20 comments

Comments

@madmiraal
Copy link

Describe the project you are working on

Godot engine

Describe the problem or limitation you are having in your project

There are a number of issues with the current class hierarchy:

  1. Window inherits Viewport, but a Window is not a Viewport. It contains a Viewport, but it's not a Viewport. So, Window should not inherit Viewport.
  2. Similarly, Viewport inherits Node, but a Viewport is not a Node. As described in the documentation, a Viewport is "a surface on which the game is drawn". As detailed in Make viewports/containers usable inside Godot, improve viewport documentation and make it simpler to actually use viewports #2139, adding a Viewport to a scene only makes sense if it's a child of a ViewportContainer or linked to a ViewportTexture. Viewport should not inherit Node. Likewise, adding a Window to a scene makes even less sense. A Window should not inherit Node either.
  3. SceneTree has a hidden root node. In 4.0 it's a Window. In 3.x it's a Viewport. However, there is no apparent good reason for this. In fact, when we design our scenes, we can use any Node as the visible root Node. On the other hand, a Viewport has a SceneTree that it displays. It's the Viewport that has a SceneTree, not a SceneTree that has a Viewport.

There are also a number of issues with separation of responsibility:

  1. A Window provides the interface to the DisplayServer (which was part of OS in 3.x). A Viewport provides the interface to the RenderingServer (VisualServer in 3.x). However, Window code contains a lot of Viewport specific code including accessing the RenderingServer. Likewise Viewport code contains a lot of Window specific code including accessing the DisplayServer.
  2. Similarly, Viewport has a lot of Control specific code. This was done a long time ago in 72fcb8a because of "bugs when controls don't have a common parent".

These issues have led to a lot of spaghetti code that makes it very difficult to troubleshoot, never mind fix, problems like godotengine/godot#20619, godotengine/godot#28665, godotengine/godot#30215, godotengine/godot#30950, godotengine/godot#32222, godotengine/godot#34805, godotengine/godot#35965, godotengine/godot#48368, etc.

Finally, the Godot Best Practices documentation has a section on When and how to avoid using nodes for everything that would suggest that a Viewport should be a Resource and a Window probably only an Object.

Describe the feature / enhancement and how it helps to overcome the problem or limitation

At a very high level:

  • Move Window code into Window.
  • Move Viewport code into Viewport.
  • Move Control code into Control
  • Make Viewport a member of Window.
  • Make SceneTree a member of Viewport.
  • Make Viewport inherit from Resource.
  • Make Window inherit from Object

This will make the class hierarchy follow general C++ as well as Godot best practices, and generally make it easier to troubleshoot and fix issues associated with Viewports CanvasLayers and Controls when the problem currently lies elsewhere.

Describe how your proposal will work, with code, pseudo-code, mock-ups, and/or diagrams

This is more of a overall C++ design philosophy. The actual code changes will be whatever it takes to make it compile and run again.

If this enhancement will not be used often, can it be worked around with a few lines of script?

No.

Is there a reason why this should be core and not an add-on in the asset library?

It's core code.

@YuriSizov
Copy link
Contributor

I don’t see any explanation how one would use it if it was no longer a node.

@madmiraal
Copy link
Author

I don’t see any explanation how one would use it if it was no longer a node.

Viewport would be a resource; so the same way you'd use any resource.

@YuriSizov
Copy link
Contributor

YuriSizov commented Feb 4, 2022

Viewport would be a resource; so the same way you'd use any resource.

That explains nothing. Currently it's a node. You use node hierarchy to put a scene into a dedicated viewport. How would you put nodes inside of a viewport if it's a resource? Where would you put a viewport resources in your node tree? Can you give examples of before and after? With ViewportContainer, with render target and ViewportTexture?

@madmiraal
Copy link
Author

How would you put nodes inside of a viewport if it's a resource? Where would you put a viewport resources in your node tree? Can you give examples of before and after? With ViewportContainer, with render target and ViewportTexture?

A SceneTree does not need a Viewport. A Viewport needs a SceneTree. Therefore, the Viewport would have a SceneTree member and displays that SceneTree. Changing scenes simply requires changing the Viewport's SceneTree.

Likewise, a Window would have Viewport member. Similarly, a ViewportContainer would also have a Viewport member instead of a child node, and a ViewportTexture would have a Viewport member instead of a NodePath.

@TheDuriel
Copy link

You've got this very backwards. Godot has One, Singular, SceneTree. The root node of this tree, is a Viewport.

@madmiraal
Copy link
Author

You've got this very backwards. Godot has One, Singular, SceneTree. The root node of this tree, is a Viewport.

I understand how it works currently. What I'm saying is that the logic is backwards and it should be the Viewport that has a SceneTree not the SceneTree that has a Viewport.

@SilencedPerson
Copy link

SilencedPerson commented Feb 6, 2022

How would this work inside the editor?
I am having trouble imagining the process. I understand why you want to change things.
But how would the multiple SceneTrees be handled with GUI? How would it affect current workflow. What would you pass to function such as change_scene(). What would be the new root?

@madmiraal
Copy link
Author

@SilencedPerson The only changes needed to SceneTree would be the root node, which would now be your scene's root node.

@TheDuriel
Copy link

How would anything get displayed? Where would Autoloads go? As children of your Scene? What if you don't want to use a default scene? What if I want multiple viewports for the same World space?

@madmiraal
Copy link
Author

How would anything get displayed? Where would Autoloads go? As children of your Scene? What if you don't want to use a default scene? What if I want multiple viewports for the same World space?

None of this would change.

@Ommina
Copy link

Ommina commented Feb 8, 2022

I'm left feeling that the meat of @TheDuriel 's question remains unanswered. At least, the "what if I want multiple viewports..." portion. You say nothing would change, but given the current method of using multiple viewports is to add additional Viewport Nodes, were Viewport to be a Resource, that's going to be a bit of a challenge. Something is going to have to change.

If SceneTree is a member of Viewport, how do I have multiple ViewPorts and where do I put them?

@TheDuriel
Copy link

A Viewport is a View into a World. Multiple Viewports may view the same World. With your proposal, a Viewport defines both a MainLoop and a World. Which invalidates many uses of Viewports.

@Zireael07
Copy link

Seconding @TheDuriel: I use two or three viewports into the same 2D world for some minimap trickery...

@Sauermann
Copy link

I believe this question was addressed here:

A SceneTree does not need a Viewport. A Viewport needs a SceneTree. Therefore, the Viewport would have a SceneTree member and displays that SceneTree. Changing scenes simply requires changing the Viewport's SceneTree.

The SceneTree defines the world and one or many Viewports display it.

@TheDuriel
Copy link

The SceneTree defines the world and one or many Viewports display it.

This is false. Worlds are resources created by Viewports.

@SilencedPerson
Copy link

So what I gathered so far is that concept of Worlds would be included in the SceneTree but then how does it distinguish between World2D and World? Actually, how does it work now? Root would become the root of a user defined scene. Does that then mean Autoloads would lie outside of a the main SceneTree or inside? What would happen to the Autoloads if I change the scene if they are inside, else, how would Autoloads reach into the SceneTrees if they are outside?

This is how the one SceneTree looks now, get_node() takes the Viewport, and finds a child of it. How would that work now?

Viewport
 ┖╴Autoload1
 ┖╴Autoload2
 ┖╴MainScene
    ┠╴Node1
    ┖╴Node2
       ┖ Node3

How does it look after the change?

@Sauermann
Copy link

Sauermann commented Feb 8, 2022

Autoload does two things:

  1. Add a new node as a child to root
  2. Add a global constant to all script languages as syntactic sugar for accessing the added node

The handling of the constants would be a problem to solve (This would require the ScriptServer to store different constants for different roots, so that they do not interfere with each other)

But otherwise, keeping the autoloaded Nodes as children of the Viewport would work just as before.
If the root node would be displayed in the editors Scene-Tree, we could add children even without going to "Project Settings" (without the constants as syntactic sugar).

@TheDuriel
Copy link

Having multiple instances unique to each root, of what are supposed single instant globals, singletons, is... just not an acceptable tradeoff for no gain?

Also still no answer about how one is supposed to have multiple cameras in the same world.

@madmiraal
Copy link
Author

The changes being proposed are about how Godot is designed under the hood. It's about following OOP design best practices. Specifically, it's about paying attention to the "is_a" vs "has_a" relationships as well as proper separation of responsibilities.

To this I've added the need to follow Godot's own best practices. Specifically, not making everything a Node. Viewport does not need to be a Node. Viewport only needs to be a Resource that is assigned to a Window, ViewportContainer or a VieportTexture. Having Viewport as a Node only causes confusion (see #2139).

Beyond that, this proposal is not suggesting anything else changes; other than making everything work as it did before (or hopefully better). Therefore, all other questions about "how one is supposed to do ..." are moot at this high-level design point.

@TheDuriel
Copy link

Beyond that, this proposal is not suggesting anything else changes; other than making everything work as it did before (or hopefully better). Therefore, all other questions about "how one is supposed to do ..." are moot at this high-level design point.

We are simply naming a plethora of reasons for why it CAN NOT work the way you propose. Part of making a proposal, is figuring out how to implement it. Spouting rhetoric about OOP design doesn't achieve that.

Yes, a Viewport doesn't need to be a node. If it had to be, it would not extend Node directly, but one of the three main types. But it being a node is the only viable option, and conveys many advantages that you are dismissing as "it'll be figured out".

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

7 participants