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

Simplify UI scaler #1157

Merged
merged 21 commits into from
Sep 8, 2021
Merged

Simplify UI scaler #1157

merged 21 commits into from
Sep 8, 2021

Conversation

kianzarrin
Copy link
Collaborator

No description provided.

@kianzarrin kianzarrin self-assigned this Sep 4, 2021
@kianzarrin kianzarrin changed the base branch from master to scale-Unity-GUI September 4, 2021 08:53
// always 1080f. But we keep this code for the sake of future proofing
return resolution.y;
}
private static float BaseResolutionX => UIView.GetAView().GetScreenResolution().x;
Copy link
Contributor

Choose a reason for hiding this comment

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

I'd reduce GetAView() calls to a minimum.
Call it once and cache it.
The method searches every time for the first UIView even though the value never changes.
Also the method is implemented via LINQ and will cause heap allocations each time.

Copy link
Collaborator

@kvakvs kvakvs left a comment

Choose a reason for hiding this comment

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

Renaming config option will reset scale for everyone.

@DaEgi01
Copy link
Contributor

DaEgi01 commented Sep 4, 2021

Renaming config option will reset scale for everyone.

I see, how do we handle that?

  1. we don't care about the user?
  2. we don't care about the code?
    :D

@kvakvs
Copy link
Collaborator

kvakvs commented Sep 4, 2021

I see, how do we handle that?
1. we don't care about the user?
2. we don't care about the code?

Reasonable default for example 100% zoom

or keep old variable, give new variable default -1 for example, and if it is -1, we migrate value to new on the first load. This migration code can be removed in the next release.

@DaEgi01
Copy link
Contributor

DaEgi01 commented Sep 4, 2021

I see, how do we handle that?

  1. we don't care about the user?
  2. we don't care about the code?

Reasonable default for example 100% zoom

or keep old variable, give new variable default -1 for example, and if it is -1, we migrate value to new on the first load. This migration code can be removed in the next release.

To be honest, in that case I'd probably leave the variable name as is since its too much hastle for too little gain. I did not consider that when I proposed to rename the variable. But your solution also sounds fine. I don't mind either solution in this case .. since they all have some little drawback.

Copy link
Member

@krzychu124 krzychu124 left a comment

Choose a reason for hiding this comment

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

👍

Base automatically changed from scale-Unity-GUI to master September 7, 2021 18:21
@kianzarrin kianzarrin merged commit f44bd75 into master Sep 8, 2021
@kianzarrin kianzarrin deleted the simplify-UIScaler branch September 8, 2021 05:20
@originalfoo originalfoo added this to the 11.6.0 milestone Dec 10, 2021
@originalfoo originalfoo added TRAFFIC LIGHTS Feature: Traffic lights - toggle, timed, etc UI User interface updates Toolbar The main TMPE toolbar labels Dec 10, 2021
@originalfoo originalfoo added the code cleanup Refactor code, remove old code, improve maintainability label Dec 23, 2021
@originalfoo originalfoo modified the milestones: 11.6.0, 11.6.1.2 Jan 18, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
code cleanup Refactor code, remove old code, improve maintainability Toolbar The main TMPE toolbar TRAFFIC LIGHTS Feature: Traffic lights - toggle, timed, etc UI User interface updates
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants