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

Improve usability of Camera2D #101427

Open
wants to merge 9 commits into
base: master
Choose a base branch
from

Conversation

Lazy-Rabbit-2001
Copy link
Contributor

@Lazy-Rabbit-2001 Lazy-Rabbit-2001 commented Jan 11, 2025

Implements and closes: godotengine/godot-proposals#11527

Changes:

* Now the default limit will not be +/-1000000, instead, the top-left will be (0, 0) while the bottom-right will become (viewport_w, viewport_h)

  • Added limit_disabled, which allows the camera focus to move anywhere. Set to false by default.
  • Added a button Snap Limit to Viewport, on which you click will snap the top-left limit to the global position of the camera and the bottom-right limit to the global position plus the size of the viewport.
  • Added a editor_draw_limits_color, which allows you to change the color of the limit rect.
  • Now you can drag the dragger to resize and offset the limit rectangle, and thus to change the limit_* variables.

Issues

  • Currently, scaling the camera will also scale the rect, which is easy to be fixed theoretically but hard to fix technically (?)

@timoschwarzer
Copy link
Contributor

timoschwarzer commented Jan 11, 2025

While I like the idea of making editing the camera limits a more interactive experience, I very much dislike the change of the default limit. I didn't try it out, but wouldn't this effectively make a camera that gets added to a node non-functional (as in, it doesn't move in any direction) unless you enlarge the limit rectangle? This would be a bad user experience, and lead to lots of confusion.

Edit: This is also why the unit tests fail.

Instead, I propose a Boolean property on the camera to enable/disable the limits, and have it disabled by default.

@Lazy-Rabbit-2001
Copy link
Contributor Author

Lazy-Rabbit-2001 commented Jan 11, 2025

Instead, I propose a Boolean property on the camera to enable/disable the limits, and have it disabled by default.

Good idea, but the reason why I changed the default is that users couldn't see and drag the rectangle as it was too large to find the dragger if we still use default 1000000 rectangle, unless they change the limit_* manually in the inspector, which is worse than making it visible in a reasonable size so that the editor viewport can contain the full rectangle of the camera limits.
Yep, it is necessary to add a property that controls if the camera movement is limited, as by default the oversized limits are still "limited", so it will be more flexible to make this have the ability to be unlimited.
As for the unit test failure, I'll fix that soon.

Copy link
Member

@kleonc kleonc left a comment

Choose a reason for hiding this comment

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

  • Now the default limit will not be +/-1000000, instead, the top-left will be (0, 0) while the bottom-right will become (viewport_w, viewport_h)

This is compatibility breaking change. Properties set to their default values are not being saved to the resource (scene) file, meaning if someone was using Camera2D with its default limits unchanged, then such limits are not saved within the scene file containing the Camera2D. After this change loading such scene would create the Camera2D with the new defaults, resulting in changed behavior (likely breaking user's project).

@Lazy-Rabbit-2001 Lazy-Rabbit-2001 changed the title Allow changing limit_* of Camera2D by dragging the rectangle Improve usability of Camera2D Jan 12, 2025
@Lazy-Rabbit-2001 Lazy-Rabbit-2001 requested a review from a team as a code owner January 12, 2025 12:05
@Lazy-Rabbit-2001
Copy link
Contributor Author

I reverted the change of default value. What takes the place of the previous change has been added in the top post.

@Lazy-Rabbit-2001 Lazy-Rabbit-2001 requested a review from a team as a code owner January 12, 2025 13:06
@kleonc
Copy link
Member

kleonc commented Jan 12, 2025

  • Added limit_unlimited, which allows the camera focus to move anywhere. Set to true by default.

Oh, haven't thought about that new bool suggestion previously, but limit_unlimited = true by default is compat breaking.

  • For the case when the user uses default limits it indeed shouldn't be problematic because the current limit defaults (+/- 10_000_000) are practically speaking "infinite", meaning the behavior should be almost for sure unaffected.
  • However, if the user uses custom limits then now such custom limits won't take any effect, because of limit_unlimited being enabled by default. So this is compat breaking, it requires the user to manually set limit_unlimited = false to restore the current behavior.

So the limits should be enabled by default to preserve the current behavior.

Regarding the limit_unlimited property name, I suggest renaming it to something simpler like limit_enabled (there are already many existing x_enabled properties across the engine, like CanvasItem.y_sort_enabled or Sprite2D.region_enabled), and of course swapping the meaning (AFAICT non-negated versions are preffered, and currently it's equivalent to limit_disabled).

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.

Allow changing limit_* of Camera2D by dragging the rectangle
4 participants