Prohibit inconsistent size state for SubViewport

[Sauermann]

Prohibit size changes of SubViewports with parent SubViewportContainers that have stretch mode enabled.

It is possible to bring the engine into an inconsistent state with the following tree:

• SubViewportContainer (with stretch = true and size = Vector2i(200, 200))
  • SubViewport

by using the command SubViewport.set_size(Vector2i(100, 100))

SubViewportContainer.stretch = true means, that the SubViewport is automatically resized to the size of the SubViewportContainer and this command temporarily bypasses this automatism.
A MRP for this can be found in #64893

The inconsistency is:

• The SubViewport is displayed stretched over the complete size of the SubViewportContainer
• Events are transformed based on the size of SubViewport

This means that mouse events do not align with the display, until the next size-recalculation (for example by resizing the window).

This PR prevents this kind of size change be checking the parents stretch status.

An alternative would be, that set_size implicitly sets the parents stretch status to false, but that seems to be more error prone.

This PR does not affect #34805, because that issue is based on SubViewportContainer.stretch = false.

[aaronfranke]

I was experiencing this issue recently and I can confirm that this PR fixes it. With this PR it now shows a warning which tells you exactly what part of your code is causing the issue.

[aaronfranke]

This can likely be improved. The first thing that occured to me is to only have this check on debug builds:

Suggested change

	SubViewportContainer *c = Object::cast_to<SubViewportContainer>(get_parent());
	if (c && c->is_stretch_enabled()) {
	WARN_PRINT("Can't change the size of a `SubViewport` with a `SubViewportContainer` parent that has `stretch` enabled. Set `SubViewportContainer.stretch` to `false` to allow changing the size manually.");
	return;
	}
	#ifdef DEBUG_ENABLED
	SubViewportContainer *c = Object::cast_to<SubViewportContainer>(get_parent());
	if (c && c->is_stretch_enabled()) {
	WARN_PRINT("Can't change the size of a `SubViewport` with a `SubViewportContainer` parent that has `stretch` enabled. Set `SubViewportContainer.stretch` to `false` to allow changing the size manually.");
	return;
	}
	#endif // DEBUG_ENABLED

Buut also, I see that the new force_set_size is already doing a get_parent() call and a SubViewportContainer cast, so maybe these should be combined somehow? I'd imagine an optional argument of bool p_force = false would break the property binding. Hmm... Maybe have internal_set_size(const Size2i &p_size, bool p_force = false) internally, and set_size(const Size2i &p_size) for the property binding which calls the internal one?

void SubViewport::internal_set_size(const Size2i &p_size, bool p_force) {
	SubViewportContainer *c = Object::cast_to<SubViewportContainer>(get_parent());
#ifdef DEBUG_ENABLED
	if (!p_force && c && c->is_stretch_enabled()) {
		WARN_PRINT("Can't change the size of a `SubViewport` with a `SubViewportContainer` parent that has `stretch` enabled. Set `SubViewportContainer.stretch` to `false` to allow changing the size manually.");
		return;
	}
#endif // DEBUG_ENABLED
	_set_size(p_size, _get_size_2d_override(), Rect2i(), _stretch_transform(), true);
	if (c) {
		c->update_minimum_size();
	}
}

[Sauermann]

My first try was to add an optional argument bool p_force = false, but that did break the GDScript binding.

SubViewport::_set_size would still need to be public, so that it can be called from SubViewportContainer.
A different approach would be to make SubViewport::_set_size private, but then SubViewportContainer would need to be a friend class of SubViewport.

@aaronfranke Do you have suggestions, which of the two approaches is better?

[Sauermann]

The first thing that occured to me is to only have this check on debug builds

Would this message not also be helpful for people, who don't use a debug build, since this can be achieved via GDScript?
Also without this check, the inconsistent state can be reached, which I would like to prevent.

[aaronfranke]

Sorry, I updated the message, the _set_size method is already taken by the parent class Viewport, so we would have to call the other one something else, like internal_set_size or I guess we could do Viewport::_set_size to refer to the base class but you're right that it might be confusing to have a public method that starts with an underscore.

Keep in mind that there are two levels of debug. DEBUG_ENABLED will show up in the editor and in debug templates (the old "release_debug"), but not in release templates (the old "release"). DEV_ENABLED is the one that will only show up when you compile yourself with full debugging for engine debugging (the old "debug"). If we have DEBUG_ENABLED it will still have the check for all editor builds.

What do you think about this? https://github.com/aaronfranke/godot/tree/fix-inconsistent-subviewport-size

[Sauermann]

Thank you for the explanation of the Debug-Levels.

Your suggestions looks fine to me with one exception:
Even in a release template, the test should be executed in order to prevent the inconsistent state.

I have pushed a new version incorporating your changes but only put the warning message within DEV_ENABLED.

[aaronfranke]

The DEBUG_ENABLED thing can be decided by @akien-mga. I would wrap the whole if in DEBUG_ENABLED, but aside from that I approve.

[aaronfranke]

Maybe docs could be improved though:

[Sauermann]

PR-review-meeting:
Add public function set_size_force.
Use _internal_set_size as name and make it private.

[Sauermann]

I have updated the PR with the requested changes from the PR-review-meeting.

[akien-mga]

Approved in PR review meeting.

[akien-mga]

Thanks!