Fix viewport scaling at intermediate resolutions

[KerekesDavid]

Fixes #102656: viewport scale is wrong at resolutions that are not integer multiples of content_scale_factor.

This is done by allowing size_2d_override to be Size2 instead of Size2i. This change is propagated to keep the exposed SubViewport consistent with the underlying implementation not propagated to SubViewport to keep compatibility for now.

Note:
I see the output of Viewport::get_visible_rect() get treated as a Rect2i in some places, even though it returns a Rect2. Before this change, this might have been accidentally correct, since it was generated from either size or size_2d_override, which were both Size2i. It might need a second look to make sure it still works correctly, because I'm not exactly sure what all these code fragments do.

[KerekesDavid]

Pushed a change that does not expose this to SubViewport to avoid the compatibility warnings. (As a side note, these do not pop up in my local tests for some reason, how would I fix this?)

If someone could point me towards some resources on what kind of compatibility are we talking about here, and what is the usual philosophy that the dev team follows, that would be super appreciated. (Is it compatibility for users between versions? Is it API/ABI compatibility?)

[AThousandShips]

The compatibility notes doesn't occur locally because it's a dedicated check, but this breaks compatibility because the type changes, and that can't be done trivially

You would probably need to register compatibility methods, see here for instructions

If this can be done in a way that doesn't break compatibility it should, if unavoidable it can be evaluated

[KerekesDavid]

One last try, forgot to revert the property registration itself to Vector2i.

[AThousandShips]

The methods will still be registered with the changed type I think, so that will still break compatibility unless the bound methods also don't use Vector2

[KerekesDavid]

Bound methods should all be Vector2i, only the internals are working with Vector2.

[Calinou]

Tested locally on macOS, it works as expected. There's no more jitter in the MRP when resizing the window from any of its corners.

I think the rationale makes sense, and keeping the exposed property as-is to keep compatibility.

[KerekesDavid]

I think this one is only waiting for a last review from the core team and a merge now :)

[akien-mga]

We're in release freeze for 4.4 and only merging regression fixes now, so this will first be merged for 4.5 after 4.4-stable is released. It's already queued for merge in 4.5-dev1.

[OhiraKyou]

I set up a dev environment and tested the changes locally. I found that only two of the changes (plus a function signature header update) needed to be included to fix the MRP. So, my reversion suggestions are based on the assumption that this would be merged as a fix for the jitter, with any additional work being pushed to an optional future PR.

Minimum necessary changes:

• In Window::_update_viewport_size, final_size_override must be a Size2
  • This is because it is divided by the content_scale_factor on lines 1155 and 1236, resulting in float values that need to be preserved.
• In Viewport::_set_size, p_size_2d_override must be a Size2 (with the redundant Size2 conversion removed).
  • This is because Window passes in final_size_override on line 1257.

In other words, the math on Window's final_size_override is what results in float values. Everywhere else, 2D size overrides are only given integer values. So, changing them to floats doesn't do anything without additional work.

Edit: Actually, as discussed later, because p_size_2d_override is saved to the Viewport's size_2d_override in Viewport::_set_size, the value will be out of sync with the calculated stretch_transform if size_2d_override is not also a Size2.

[Repiteo]

Thanks! Congratulations on your first merged contribution! 🎉

[KerekesDavid]

Thanks! Congratulations on your first merged contribution! 🎉

Thanks, happy to help! :)