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

Calculate window input event transform only on window change #59310

Merged
merged 1 commit into from
Jan 31, 2023

Conversation

Sauermann
Copy link
Contributor

@Sauermann Sauermann commented Mar 19, 2022

Currently during every event processing, Viewport::_get_input_pre_xform() calculates the Transform2D, that is based on a few attributes of Windows like CONTENT_SCALE_ASPECT, CONTENT_SCALE_MODE and others.
However this Transform2D only changes, when the window changes.

With this patch, the Transform2D gets only recalculated after a window change.
This change also makes several transform-calculations in Viewport simpler.

Transforms drawio

My MRP for testing these changes consists of multiple subwindows, that can be configured with all available options:
WindowsTestbed.zip (updated 2022-05-07 since Input-API changed, updated 2022-09-18 since Transform2D-API changed)

image

Update 2022-10-01: split nonessential parts into other PRs

@Sauermann Sauermann requested review from a team as code owners March 19, 2022 10:25
@Calinou Calinou added this to the 4.0 milestone Mar 19, 2022
@Sauermann Sauermann force-pushed the proposal-event-transform branch 4 times, most recently from e7dcb5a to 3927653 Compare March 20, 2022 19:33
@Sauermann
Copy link
Contributor Author

Sauermann commented Mar 20, 2022

I noticed, that it is a bad idea to merge "Stretch Transform" into "Window transform", because one could imagine, that "Stretch Transform" could be changed without knowledge of Window, which would lead to unexpected behavior. My latest update solves this problem.

Extended MRP: WindowsTestbed.zip (no longer works because of Input-API changes)

Transforms drawio

@Sauermann
Copy link
Contributor Author

Originally, this enhancement PR contained a bugfix. I have split this bugfix into its own PR: #59709

@akien-mga
Copy link
Member

This has been updated / simplified, would be good to review further.

I'm not 100% clear on what are the actual benefits of this PR though. Is this a performance optimization, or does it solve issues?

@Sauermann
Copy link
Contributor Author

Since I don't intend to change any functionality with this PR, it does not solve any issues.
My reasons for this PR are:

  1. Structure: This transform originates in the Window. Viewport should be able to function, without knowing anything about it. Even SubViewports have to go through _get_input_pre_xform, even though it is always the identity transform for them.
  2. Clarity: _get_input_pre_xform is not very descriptive and it took me a long time to understand, where it comes from and why it is necessary. I would like to make the functionality more accessible by moving the calculation to Window::_update_viewport_size, where the transform originates in.
  3. Code simplification:

In a followup I want to remove Viewport::to_screen_rect. The only other place, where it is used, can be replaced by a check, if this is a Window or a SubViewport. It seems like to_screen_rect was only passed to Viewport in order to calculate the transform there.
In a followup after #66692, I want to replace Viewport::_get_input_pre_xform by a lookup in the Window class.

  1. Performance: but I am not sure if this is for events necessary

Copy link
Member

@groud groud left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Approved in PR review meeting.

@akien-mga akien-mga merged commit 1c42e14 into godotengine:master Jan 31, 2023
@akien-mga
Copy link
Member

Thanks!

@Sauermann Sauermann deleted the proposal-event-transform branch January 31, 2023 21:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants