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

Merged
merged 1 commit into from
Mar 12, 2025

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_enabled (true by default), disabling which will allow the camera focus to move anywhere.
  • Added a plugin button Snap Limit to Viewport, on which you click will snap the top-left limit to the global position of the camera (0, 0) 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.

kleonc
kleonc previously requested changes Jan 11, 2025
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).

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.

AFAICT no longer breaks compat. 👍

  • Added limit_disabled, which allows the camera focus to move anywhere. Set to false by default.

As already mentioned in #101427 (comment), please rename it to limit_enabled and swap its default value/checks (even for the Camera2D itself there are already properties like enabled, drag_{horizontal|vertical}_enabled, {position|rotation}_smoothing_enabled).

@kleonc kleonc dismissed their stale review January 13, 2025 21:37

No longer compat-breaking.

@Lazy-Rabbit-2001
Copy link
Contributor Author

Making the plugin of it seems a bit time-costing so I don't plan to add it in this pr. However, once i have available time for it, I'll try to add it in a continuation of this.
So I think it's time for reviewing the codes and the documentation.

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.

Tested a little in action, AFAICT works as expected. Overall LGTM, not sure about the exposed editor_draw_limits_color, see the comment I've added.

@@ -937,6 +1014,7 @@ void Camera2D::_bind_methods() {
ADD_GROUP("Editor", "editor_");
ADD_PROPERTY(PropertyInfo(Variant::BOOL, "editor_draw_screen"), "set_screen_drawing_enabled", "is_screen_drawing_enabled");
ADD_PROPERTY(PropertyInfo(Variant::BOOL, "editor_draw_limits"), "set_limit_drawing_enabled", "is_limit_drawing_enabled");
ADD_PROPERTY(PropertyInfo(Variant::COLOR, "editor_draw_limits_color"), "set_limit_drawing_color", "get_limit_drawing_color");
Copy link
Member

Choose a reason for hiding this comment

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

For other reviewers: note there are other debug drawing colors which are still hardcoded, so this one being customizable/exposed is inconsistent in that regard. I'm not sure whether this is fine as is or if maybe something needs to be done about it.

Color area_axis_color(1, 0.4, 1, 0.63);

Color margin_drawing_color(0.25, 1, 1, 0.63);

Copy link
Member

@Calinou Calinou Jan 22, 2025

Choose a reason for hiding this comment

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

If it's to be made configurable, the limits color should be configurable in the editor settings rather than a per-node property, unless there's a demonstrable need for it to be configured on a per-node basis (like collision shapes).

That said, I would prefer we don't expose a setting at all if no need has been expressed by users beforehand.

@arkology
Copy link
Contributor

arkology commented Feb 7, 2025

Maybe @KoBeWi would help with this if he'd like to help with the new dragging system for Camera2D.

I think it may be enough to open separate issue for rotated camera dragging and leave this out of scope of current PR.

@KoBeWi KoBeWi modified the milestones: 4.x, 4.5 Feb 7, 2025
@Lazy-Rabbit-2001
Copy link
Contributor Author

Lazy-Rabbit-2001 commented Feb 9, 2025

UPD: Also I get error ERROR: Nonexistent signal: '_camera_limit_enabled_updated'.

@arkology How did you trigger this error thrown at that time? Maybe I need details about it so that I can locate where the problem happened.

@arkology
Copy link
Contributor

arkology commented Feb 9, 2025

@Lazy-Rabbit-2001 I didn't pay attention what exactly caused error. And I can not reproduce it now.

@arkology
Copy link
Contributor

arkology commented Feb 9, 2025

I'm not sure how to check if the current mode is "Move Mode", and I'd appreciate it if someone could tell me where these modes are stored.

CanvasItemEditor::get_singleton()->get_current_tool(). But I don't know how to track its state properly.

@Lazy-Rabbit-2001
Copy link
Contributor Author

@Lazy-Rabbit-2001 I didn't pay attention what exactly caused error. And I can not reproduce it now.

Perhaps you triggered an edge case that is extremely difficult to track....
nvm let this pr get merged and see if that case can be caught within some conditions

@KoBeWi
Copy link
Member

KoBeWi commented Feb 10, 2025

You still didn't remove TOOLS_ENABLED checks from the plugin.

@Lazy-Rabbit-2001
Copy link
Contributor Author

You still didn't remove TOOLS_ENABLED checks from the plugin.

Sorry but I was so busy that I had no time available for it. I'll remove the rest one tonight

@Repiteo Repiteo dismissed their stale review March 12, 2025 02:48

Style inconsistency addressed

@Repiteo Repiteo merged commit 7e6c468 into godotengine:master Mar 12, 2025
20 checks passed
@Repiteo
Copy link
Contributor

Repiteo commented Mar 12, 2025

Thanks!

@Lazy-Rabbit-2001 Lazy-Rabbit-2001 deleted the camera_2d_draggable branch March 12, 2025 16:47
@arkology
Copy link
Contributor

arkology commented Mar 16, 2025

image

Bug - both yellow and orange rectangles are camera limit. The real camera limit is yellow one.
I did not test, but I suppose pull requests by @KoBeWi referenced above will fix it.

UPD 2025/03/18
Also it does not respect ignore_rotation which is annoying.

@Lazy-Rabbit-2001
Copy link
Contributor Author

Bug - both yellow and orange rectangles are camera limit. The real camera limit is yellow one. I did not test, but I suppose pull requests by @KoBeWi referenced above will fix it.

Hope he will fix this, as I've checked a bit he has established a new and unique dragging system for Camera2D

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
8 participants