-
Notifications
You must be signed in to change notification settings - Fork 92
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
Enhance rotate and zoom UI #112
Conversation
Every |
Buttons are now disabled on max/min zoom. It's handled using Global singleton, maybe refactoring to different Singleton would be better. Or if you find another way to do this, I would be glad. |
@artism90 check it out if you've got time |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There are a few issues remaining, otherwise it looks fine.
Assets/UI/TabWidgets/Buttons/MainButton/MainButtons/ZoomButton.gd
Outdated
Show resolved
Hide resolved
Assets/UI/TabWidgets/Buttons/MainButton/MainButtons/ZoomButton.gd
Outdated
Show resolved
Hide resolved
I addressed the issues, thanks |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The signal connection syntax was simplified in the previous PR, so it should be here too. Please also rebase to the master branch, and when everything done, squash all commits into one containing the PR message only.
Assets/UI/TabWidgets/Buttons/MainButton/MainButtons/ZoomButton.gd
Outdated
Show resolved
Hide resolved
Assets/UI/TabWidgets/Buttons/MainButton/MainButtons/ZoomButton.gd
Outdated
Show resolved
Hide resolved
Assets/UI/TabWidgets/Buttons/MainButton/MainButtons/ZoomButton.gd
Outdated
Show resolved
Hide resolved
@CrafterSvK have you maybe time to get this pr done please? |
Global.camera_zoom_out.connect(Callable(self, "_on_camera_zoom_out")) | ||
Global.camera_rotate_left.connect(func(): rotate(-PI/2)) | ||
Global.camera_rotate_right.connect(func(): rotate(PI/2)) | ||
|
||
func _process(delta: float) -> void: | ||
_move(delta) | ||
_move_drag() | ||
|
||
func _input(event: InputEvent) -> void: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hello :) !
Please read this doc first https://docs.godotengine.org/en/stable/tutorials/inputs/inputevent.html
A thing that need to be said is checking inputs with _input, in _process or in _physics_process is really different.
You can test the code bellow by example
extends Node
var i = 0
var j = 0
func _input(_event):
i += 1
func _process(_delta):
j += 1
func _notification(what):
if what == NOTIFICATION_WM_CLOSE_REQUEST:
prints(i, j)
_input is only called when an event occur. That mean it also not respect frame rate.
_process try to respect frame rate.
_process_physic follow frame rate.
So, we need to be pretty meticulous when we change the behavior of something in _input
if event.is_action_pressed("zoom_in"): | ||
_zoom_mouse(-ZOOM_VALUE) | ||
|
||
elif event.is_action_pressed("zoom_out"): | ||
if _camera.size < ZOOM_OUT_LIMIT: | ||
_zoom(ZOOM_VALUE) | ||
_zoom_mouse(ZOOM_VALUE) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It is preferable to do that in _process func.
@@ -74,18 +71,45 @@ func _move_drag() -> void: | |||
_origin.translate(move_dir) | |||
_drag_pos = new_drag_pos | |||
|
|||
func _rotate(rotation: float) -> void: | |||
func rotate(rotation: float) -> void: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It is not recommanded to call the func rotate. Chance is a "Node" type, but "Node2D" have a rotate func.
func _zoom_mouse(zoom_value: float) -> void: | ||
if _is_zoom_within_limit(zoom_value): | ||
var m_pos := _viewport.get_mouse_position() | ||
var start_result := _raycast_from_mouse(m_pos, 1) | ||
_zoom_camera(zoom_value) | ||
var end_result := _raycast_from_mouse(m_pos, 1) | ||
if not (start_result.is_empty() and end_result.is_empty()): | ||
var move_dir: Vector3i = start_result.position - end_result.position | ||
_origin.translate(move_dir) | ||
|
||
_zoom_limit_check() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please read https://docs.godotengine.org/en/stable/tutorials/physics/ray-casting.html#d-ray-casting-from-screen
Also you can use math(Pythagore) instead physics engine. You know your plane is always on Y = 0.
Not same, but can help you https://github.com/theludovyc/Godot_DestropolisLike/blob/1081bde949fd27e9d43eb0d3e4cfa5e36faa1b8a/Script/Camera3D.gd#L15
func _on_TabWidget_button_rotate_left_pressed() -> void: | ||
emit_signal("button_rotate_left_pressed") | ||
|
||
func _on_TabWidget_button_rotate_right_pressed() -> void: | ||
emit_signal("button_rotate_right_pressed") | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If _on_TabWidget_button_rotate_left_pressed() is already called by signal, is not great to re-send a signal from it.
Signals with Godot is great powers, so come great responsability.
Try to use less as possible, they have cost on performances.
func _ready() -> void: | ||
pressed.connect(_on_ZoomButton_pressed) | ||
|
||
if zoom_direction == ZOOM_DIRECTION.IN: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You can use match here, please read https://docs.godotengine.org/fr/4.x/tutorials/scripting/gdscript/gdscript_basics.html#match
You can also use composition to avoid this case
@@ -14,6 +17,9 @@ signal button_game_menu_pressed | |||
|
|||
func _ready() -> void: | |||
if owner is PlayerHUD: | |||
connect("button_rotate_left_pressed", Callable(owner, "_on_TabWidget_button_rotate_left_pressed")) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
And if owner is not PlayerHUD what happens ? In which cases it happen ?
@CrafterSvK how it looks on this pr? |
Hello, I can take a look this evening. This probably will need a rebase first. |
Hello, I've been trying to get a hold of the codebase, so I've chosen the simplest thing I could think of.
Rotation is currently handled by multiple keyboard input handlers which do separately things (rotate camera, rotate isometric objects). Need to merge to one keyboard handler which sends signals to other handlers.
In my opinion it would be lazy to solve this by making a global emitter, for these kinds of things, maybe that's right, I do not know. Or just make button to click
rotate_left
,rotate_right
key :)