Viewport update mode lost when nested under ViewportContainer

[ArdaE]

Godot version:
3.0.6 (issue also present in 3.1 Alpha2)

OS/device including version:
macOS 10.13.6

Issue description:
When a Viewport is used with a ViewportContainer, its update mode (render_target_update_mode parameter) silently reverts back to "Always" whenever the project/scene is run or the scene tab is changed.

The culprit doesn't seem to be Viewport but ViewportContainer, as Viewport's update mode sticks when used in other configurations, such as when set up to render to a ViewportTexture used by a TextureRect.

Workarounds:
Replacing ViewportContainer with a simple TextureRect+ViewportTexture combo avoids the issue.

Why is this important if there's such a simple workaround?:
This is a big deal, as one wouldn't notice the issue if a scene under a Viewport is set up as a static one that is configured to render only once (e.g. to convert a 3D model/scene to a 2D static image). A scene that is set up to render once would typically not be set up to change at all, and hence provide no visual indication to the developer that it is being re-rendered every frame, wasting CPU & GPU resources. (I personally didn't realize for quite awhile that my many instances of a home-made Label3D class -- to overcome Godot's lack of a Label3D class at the moment -- were being rendered every frame even though I set up Label3D to render each instance once and only once whenever either the text changes or the scene that uses it asks the text to be rendered in a different resolution. The only reason I noticed this is because of the parameter change that I kept seeing in version control diffs.)

Considering the impact it can cause on a deployed project, ViewportContainer should either be removed, documented better, or -- in my opinion -- should be left in without any documentation changes, but it should not force Viewport's update mode back to "Always".

Steps to reproduce:
Nest a Viewport under a ViewportContainer, set its "Update Mode" to "Once", then run the scene or switch to another scene tab in the editor and then go back to the original scene. Note the change of "Update Mode" back to "Always".

Minimal reproduction project:
The attached project below has two scenes in it. Both scenes have a cube that continuously rotates, but is set up to render only once.

• ViewportTexture.tscn demonstrates the problem. When run, the expected behaviour is to see a stationary box, as the "Update Mode" was set to "Once" at the time the scene was created. But the cube rotates, and the Update Mode that is loaded is set to "Always". Set "Update Mode" to "Once" and run the scene again; note that it makes no difference, i.e. that you can still see the box rotate.

• TextureRect.tscn demonstrates a workaround to the problem where the Viewport is used to render to a ViewportTexture used by a TextureRect. In this case, the display doesn't update as the cube continues to rotate, as expected. I also added a little handler for the space bar on the keyboard to trigger a new snapshot (press space bar to take new snapshots of the cube).

ViewportUpdateModeIssue.zip

[ArdaE]

Just noticed that the Viewport's Update Mode seems to be tied to the ViewportContainer's Visibility. If the ViewportContainer is hidden, then the Viewport's update mode gets set to "Disabled", and if it's visible, then the update mode gets set to "Always". So, this implementation seems intentional, but seems like the wrong thing to do, as the Viewport already has a "When Visible" setting for that purpose. Currently neither the "Once" nor the "Always" setting will ever really work in this situation, as the Viewport always acts as if it is set to "When Visible" when it's under a ViewportContainer (although it'll never display that value).

[bojidar-bg]

Those lines are responsible for the bug/misfeature:

godot/scene/gui/viewport_container.cpp

Lines 120 to 123 in 26d33d1

	if (is_visible_in_tree())
	c->set_update_mode(Viewport::UPDATE_ALWAYS);
	else
	c->set_update_mode(Viewport::UPDATE_DISABLED);

I guess they might be removed, given that Viewport defaults to UPDATE_WHEN_VISIBLE:

godot/scene/main/viewport.cpp

Line 3061 in 26d33d1

update_mode = UPDATE_WHEN_VISIBLE;

Still, I would like to ping @reduz for that, since I'm not sure about his intention when he wrote that code as part of the GLES3 changes (cf5778e).

[ScepticDope]

Also noticed this weird behavior, still in V3.1.1 Stable Mono on Windows 8.1.

@bojidar-bg I suspect this might be the way it is because if not set correctly multiple viewports can become a performance problem real quick when MSAA is on.

I'd like to be able to actually set a different render mode when using a Viewport inside a ViewPortContainer to see what it does to the performance.

[bojidar-bg]

Asked reduz on IRC: link to logs, 16:03-44

[16:03:27] <bojidar_bg> reduz: would be nice if you could give https://github.com/godotengine/godot/issues/23729 a quick look :)
[16:39:37] <reduz> bojidar_bg: I think it's the way it's supposed to work for most cases
[16:39:45] <reduz> bojidar_bg: doesn't really make sense it works otherwise
[16:40:15] <reduz> bojidar_bg: the problem with update when visible is that it has a frame of delay, so it's not really that useful
[16:40:35] <bojidar_bg> reduz: the problem is that the ViewportContainer is changing the Viewport's update mode, without regard for what the user might want to set
[16:40:48] <bojidar_bg> esp. update once, which might be useful in certain cases
[16:41:05] <reduz> bojidar_bg: yeah but if you remove this behavior, it becomes a lot less efficient
[16:41:30] <bojidar_bg> can it be made into an option?
[16:41:32] <reduz> bojidar_bg: so I suggest adding a feature in ViewportContainer to control update mode, which defaults to the current behavior
[16:41:48] <reduz> like viewport_update_control = AUTOMATIC/USER

So, ViewportContainer should get a new property, named something like viewport_update_control, which has two values: automatic and manual. The default, "automatic", would be the current behaviour, while "manual" would not change the Viewport's update mode, in order to allow the user to pick an update mode themselves.

[ArdaE]

Instead of yet another property that'll create its own issues (see below), why not a warning about the frame-delay (and whatever other performance issue exists, if any)? Or, alternatively, why not simply disable the viewport's update mode property when the viewport is nested under a ViewportContainer, so people know to seek a different solution (e.g. the workaround I mention)?

"the problem with update when visible is that it has a frame of delay, so it's not really that useful"

This comment doesn't make any sense to me. I described why update mode is a very useful feature in some cases (otherwise why even have the feature if it won't work?), and why it is dangerous to have it automatically change behind one's back. As long as the update mode property is there and is editable, it'll still be very easy to not realize that it changes automatically by default, hence forget to change this new viewport_update_control property and end up with the same issue (the issue being not realizing the GPU is rendering an additional viewport 60 frames per second for a static scene, when you've specifically instructed it not to do so).

For anyone who knows of the issue, there's a simple workaround anyway; so the problem is for people who don't realize update mode is changing behind their back. So the new viewport_update_control property does almost nothing to address the original issue (it won't even be in close proximity to the viewport update mode in the inspector, as one is proposed to be within ViewportContainer while the other is in Viewport, making the new property more likely to be missed).

[ArdaE]

As a general design pattern, a property that is editable by the user should never change automatically.

[ScepticDope]

I would like to suggest that there is a small doc update and that this weird restriction is simply removed. For now I decided to set the desired RenderTargetUpdateMode from code on startup before ready for all my viewports. Performance skyrocketed and my game is back to 60FPS on 2160p with 16x MSAA (overkill? I think not!) for both single-player and split-screen co-op. 😅

@ArdaE Even simpler solution, snippet: GetNode<Viewport>(".../Viewport").RenderTargetUpdateMode = (Godot.Viewport.UpdateMode) 0;

The UPDATE_WHEN_VISIBLE does not work properly either I think. Also when is it considered visible? When it's not set to hidden or when it's actually visible in the root viewport? Should be in the docs I think.

[ScepticDope]

Just noticed that, UPDATE_ALWAYS creates a bug with MouseMode for some strange reason after changing while game is running the cursor can become hidden.

Also being able to actually set UpdateMode on viewports in the editor would improve my game startup time even more. I set them all at once in a _Ready now. (So I don't have to make a whole different setup where nodes get lazy loaded or I have to set it in every _EnterTree of each individual Viewport, they currently don't have scripts added and I'd like to keep it that way)

[KoBeWi]

Still valid in 4e52c75

[boruok]

sheesh, i though why node processing and emitting signals, but viewport texture not updating. Still valid in v3.3.3.stable.official [b973f99]

[bluenote10]

I was just about to open an issue "viewport.render_target_update_mode not working", but I think it just a manifestation of this issue. I'm collecting some thoughts on how to solve the issue.

To summarize the current behavior: Whenever the ViewportContainer receives a NOTIFICATION_ENTER_TREE or NOTIFICATION_VISIBILITY_CHANGED it takes control over any viewport.render_target_update_mode depending on is_visible_in_tree(). The pros/cons of this behavior are:

• pro: When the viewport container is not in the tree, the rendering of children viewports gets disabled automatically.
• con: When the viewport enters the tree, it enforces UPDATE_ALWAYS on all viewports. This makes all use cases like update once, update with reduced framerate, temporarily disable render, ... impossible.

While the disabled rendering in the not-in-tree state makes some sense, the desired update mode in the in-tree state should be user controlled.

Solution 1: Remove automatism

Simply removing the lines quoted by @bojidar-bg would allow for taking full control over viewport.render_target_update_mode. The downside would be that users have to disable the rendering manually in their viewports. In my opinion that isn't too bad. Intuitively and according to the docs, viewport.render_target_update_mode is something that I as a user have to control manually. I would have expected that it is my job to turn off render updates, no matter if the viewport container is visible or not.

Solution 2: Allow to enable/disable automatism

An easy non-breaking solution: Introduce a new setting on the viewport container to enable/disable the automatism. Downside: Adds complexity and yet another hard-to-explain flag, which isn't so great.

Solution 3: Automatism with extra memory

If we want to keep the automatism, it could remember what update mode a child viewport was using originally when switching from in-tree to not-in-tree. When re-entering the tree, it could restore the proper update mode. This solution obviously comes with additional complexity, because the child viewports and their modes have to stored and identified, if one uses a map in the viewport container remember their states. Alternatively the viewports could have two properties render_target_update_mode (the uncontrolled/desired update mode) and render_target_update_mode_controlled (the controlled/managed/internal/actual/current update mode), so that the viewport container can only control the latter, and can easily obtain the desired value for the in-tree case.

Solution 4: Decouple "tree visibilty" from "viewport update mode" / forward is_visible_in_tree into viewport children

I'm wondering if it is more appropriate to interpret "visibility in tree" and "viewport update mode" as two different things altogether. This thinking would maybe imply that it is more appropriate to forward the in-tree information into the subtree of the viewport. I.e., when the viewport container enters/exits the tree, it should only make sure that is_visible_in_tree() is mirrored into the viewport subtree. This would then (perhaps) allow to move the optimization of not rendering (when the viewport container is not in the tree) into the children of the viewport. Effectively, the viewport container would never mess with the update mode, but if it is not in-tree, the viewport would essentially also render nothing, because it has no rendering children.

[jitspoe]

I just wanted to drop in here and comment that I ran into this issue as well. I was setting the render_target_update_mode to update when visible, and every time I'd reopen the project, I'd notice it was set back to always, which was quite confusing and looked like a bug.

Turns out, it is actually behaving exactly the way I want (only updating when the viewport container is visible), but I was worried it was updating when not visible.

Perhaps something that can be done to reduce confusion would be to disable the setting when a viewport is a child of a ViewportContainer, or maybe have a notification when you change it that the ViewportContainer will override the behavior.

Things like this can sink a bit of dev time just investigating why they behave like they do and now how is expected.

[Rindbee]

Still valid in d5c1b9f.

The reason may be that there is no update mode that behaves as expected in this case.

UPDATE_WHEN_VISIBLE has no meaning for SubViewport without visible property.

And the parent in UPDATE_WHEN_PARENT_VISIBLE refers not to the parent node, but to the parent window.

[uskprod]

Coincidentally ran into this issue today as well. As a workaround, setting the update mode flag every process frame seems to work for me.

EDIT: Actually, just manually setting it each time the visibility changes is enough. Thought I tried that already, but I accidentally had an early return in the function so it wasn't reaching that call.