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

Fix the CanvasItemEditor jiggling around when zooming. #73223

Closed

Conversation

Macksaur
Copy link
Contributor

I like having the editor view constrained to the scene but it behaves kinda funny when you zoom out.

This PR tackles two issues:

  • The view centring code isn't stable and vibrates when zooming
  • The scrollable area calculation often ends up negative and wont centre the scene when zoomed out

This PR fixes these issues by adding a dynamically sized border around the canvas item rect that's the larger of either

  1. the size of the configured project's window size (same as before this PR) or
  2. half of the editor's visible area.

This allows the canvas item editor to always centre exactly on one of the four corners of the scene when zoomed out. When zoomed in this PR behaves as before and allows panning the editor view up to one full configured project window size away.

Without this PR (jiggly + doesn't centre):
godot_constrain_zoom

With this PR (smooth + centres on any of the four corners):
godot_constrain_zoom2

@Macksaur Macksaur changed the title Updated the CanvasItemEditor view offset calculation to include a dynamically sized border. Fix the CanvasItemEditor jiggling around when zooming. Feb 13, 2023
@YuriSizov YuriSizov added this to the 4.1 milestone Feb 13, 2023
@YuriSizov YuriSizov requested a review from a team February 13, 2023 18:41
@akien-mga akien-mga requested review from Calinou, KoBeWi and groud June 19, 2023 12:46
@groud
Copy link
Member

groud commented Jun 19, 2023

The constrained view was removed in the end, so I guess this PR could be closed ?

@KoBeWi
Copy link
Member

KoBeWi commented Jun 19, 2023

Yep, thanks for the contribution nonetheless.

I think this might be still relevant for 3.x.

@KoBeWi KoBeWi closed this Jun 19, 2023
@Macksaur
Copy link
Contributor Author

The constrained view was removed in the end

I think this PR would have prevented its removal but I don't know if it was ever brought up in meetings. I would very much rather this issue not dismissed so quickly.

@Macksaur
Copy link
Contributor Author

Macksaur commented Jun 19, 2023

I've tried to to link issues that people were having that this PR would directly solve. From what I can understand from the frustrations involved (mine included) removing the constrained view only feels like the correct solution when you give up on fixing it. It should have been a last resort. People are turning off the constrained review because it is broken. I don't think this PR was considered before the constrained view was removed.

I believe this PR makes the constrained view smooth and enjoyable to use. People can still turn it off if they must. But at least having it behave in a more regular fashion would be more appealing to me than its outright removal.

@KoBeWi
Copy link
Member

KoBeWi commented Jun 19, 2023

Most of the issues weren't about scrolling experience, but about viewport getting locked at an arbitrary position. (e.g. #47007, #32163)

@Macksaur
Copy link
Contributor Author

If you look at those issues, I've commented at the bottom that I believe this PR fixes those complaints...

@KoBeWi
Copy link
Member

KoBeWi commented Jun 19, 2023

🤔
Ok sorry, I missed that you also adjusted the scrollable area.

Well, the feature can always be re-added if there is demand. Though there was little interest in fixing it; most people were fine once they learned that constrains can be disabled. You could maybe open a proposal and see if there is any support (although it would be probably nice if people could test the fixed version...).

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