Repository navigation
Add a visual clue to indicate the projectiles are about to dissapear - #2801
Salonso928 wants to merge 9 commits into
Conversation
|
Test build no longer available. |
manuq
left a comment
There was a problem hiding this comment.
The work you did looks good for the NO_EDIT_combat.tscn, which uses the NO_EDIT_projectile.tscn. But not with other projectiles, like ink_blob_projectile.tscn or any other projectile in StoryQuests.
I have more to comment about the visual clue (which is a great start, by the way), but let's fix this first!
| time_passed+=_delta | ||
| if time_passed>= time_to_dissapear && not is_dissapearing: | ||
| is_dissapearing=true | ||
| animation_player.play("dissapear") |
There was a problem hiding this comment.
This crashes the game if you play res://scenes/quests/lore_quests/quest_000/4_ink_combat/tutorial_ink_combat.tscn with:
E 0:00:08:038 projectile.gd:71 @ @implicit_ready(): Node not found: "%AnimationPlayer" (relative to "/root/TutorialInkCombat/InkBlobProjectile").
E 0:00:12:055 Projectile._process: Cannot call method 'play' on a null value.
…effect is only controlled by the projectile.gd script
|
Hi @manuq I made some changes. I removed the AnimationPlayer node, now the blink effect is only controlled by the projectile.gd script, therefore all the scenes that use this script should display the effect. Videocaptura.de.pantalla_20260828_120009.mp4 |
manuq
left a comment
There was a problem hiding this comment.
This looks good, although I think that the projectile should blink a predictable amount of times, let's say 3 blinks by default before dissapearing. Sorry that it wasn't part of the specification!
I understand that you switched from AnimationPlayer to make the blink programmatically because, as you saw, your first attempt changed only one of the 18 projectile scenes currently in the game.
There are pros and cons of doing it programmatically: the main benefit is that changing the script changes all 18 at the same time, great! But it leaves all the projectiles with the same look and feel. Someone wanting a different blink effect will need to duplicate the script. And artists wanting to improve the effect will be more comfortable editing it in an AnimationPlayer. Also we may want to add a sound effect in addition to the blink as audio clue.
Is fine if you want to keep doing it programmatically. I may add a followup ticket to move to AnimationPlayer. If so, check my suggestion to switch to Tween.
Alternatively, if you want to go back to AnimationPlayer you can make it optional, and add AnimationPlayer nodes to the NO_EDIT_projectile.tscn and the ink_blob_projectile.tscn, leaving the others (all StoryQuests) as before (without blink effect). For making it optional, you should not set a unique name (as I suggested before):
@onready var animation_player: AnimationPlayer = %AnimationPlayer
Because that will fail with Node not found: "%AnimationPlayer" in the scenes that don't have it. Instead you can export a new property:
@export var blink_animation_player: AnimationPlayer
Under @export_group("FXs"). And assign it in the 2 scenes mentioned above.
| ##the effect will start when the projectile has spent 70% of its lifetime | ||
| time_to_dissapear = duration * 0.7 | ||
|
|
||
| blink_timer = Timer.new() |
There was a problem hiding this comment.
Instead of using a Timer node to do the blinking, it would be better to use a Tween.
But this leads to my main feedback, whether or not to animate the blink effect programmatically. I'll comment about it separately.
| time_passed += _delta | ||
| if time_passed >= time_to_dissapear && not is_dissapearing: |
There was a problem hiding this comment.
The DurationTimer node is already tracking the time, you could check its time_left property. Or even better, create a separate Timer for this, and call it StartBlinkingTimer.
The _ready() method already does:
duration_timer.wait_time = duration
duration_timer.start()
So that's the place to setup the new timer:
duration_timer.wait_time = duration
duration_timer.start()
start_blinking_timer.wait_time = duration - blink_times * blink_duration
|
The lint and format checks are failing too for boring reasons (new lines and spacing). Please consider installing pre-commit as explained here. |
manuq
left a comment
There was a problem hiding this comment.
Excellent, this works very well now. Thank you so much for taking my advice on using AnimationPlayer. See my new feedback.
| if blink_animation_player: | ||
| if duration_timer.time_left <= time_to_dissapear && not is_dissapearing: | ||
| is_dissapearing = true | ||
| var animation_duration = blink_animation_player.get_animation("dissapear").length | ||
| blink_animation_player.play("dissapear", -1, animation_duration / blink_effect_time) |
There was a problem hiding this comment.
This is inside the _process function so it's being checked every frame. It would be better to add a timer, similar to the existing duration_timer.
| # Logic for the dissapear animation | ||
| # When the proyectile is dissapearing, the duration_timer will not reset | ||
| if not is_dissapearing: | ||
| duration_timer.start() |
There was a problem hiding this comment.
This changes the behavior. Now projectiles that have a blink_animation_player will behave differently and don't do what the documentation comment of duration_timer says:
## This timer is restarted each time the projectile collides with anything.
## So the life of the projectile is extended in each collision.
@onready var duration_timer: Timer = %DurationTimer
We could discuss this behavior separately, but that's what it is right now. If you add a timer to trigger the animation, as suggested above, you should reset it here too.
| # When the proyectile is dissapearing, the duration_timer will not reset | ||
| if not is_dissapearing: | ||
| duration_timer.start() |
There was a problem hiding this comment.
Same here, changes the existing behavior.
There was a problem hiding this comment.
I understand that you changed this level by accident, while testing. Please revert it.
…he projectile collides or gets reppelled
|
Hi! I made some changes. I followed your feedback, now the blink effect will reset if the projectiles collides or gets repelled (as seen in the following video) and also I use a timer to control the effect. The effect iis and works the same as before but now the code is better. godot7.mp4this also works for the blob projectile because de gd script is the same |
manuq
left a comment
There was a problem hiding this comment.
Great work! Thank you for following my recommendation and using a timer, it's much better now. This is almost ready, check my new comments below.
| ## The percentage of the projectile's lifespan before it starts its disappearing animation. | ||
| @export_range(0.0, 1.0, 0.1) var dissapearing_point: float = 0.7 |
There was a problem hiding this comment.
I played 2 levels that has this default, at 70% the projectile starts blinking:
- res://scenes/quests/lore_quests/quest_000/4_ink_combat/tutorial_ink_combat.tscn
- res://scenes/quests/lore_quests/quest_001/2_ink_combat/ink_combat_round_3.tscn
And because the enemies have different projectile duration, 5 seconds in first one and 10 seconds in the other, the clue is much quicker in the first one. If the projectile duration is too short, the dissapearing clue is too short too, giving the player no time to react. Would you consider changing it so that the clue duration is be the same regardless of the projectile duration? If so you can replace the dissapearing_point with a blink_duration export variable, in seconds. Basically replace this calculation:
var time_to_dissapear = duration * dissapearing_point
With:
var time_to_dissapear = min(0, duration - blink_duration)
| #If the projectile collides with another object or is repelled the animation will reset | ||
| if blink_animation_player: | ||
| if is_dissapearing: | ||
| reset_animation_and_timer() | ||
| else: | ||
| blink_timer.start() |
There was a problem hiding this comment.
This is repeated below. Can you add a new function _reset_timers() and factor out this?
| apply_impulse(impulse) | ||
|
|
||
| if blink_animation_player: | ||
| var time_to_dissapear = duration * dissapearing_point |
There was a problem hiding this comment.
|
|
||
| func _on_blink_timer_timeout() -> void: | ||
| is_dissapearing = true | ||
| var animation_duration = blink_animation_player.get_animation("dissapear").length |
There was a problem hiding this comment.
This is the second one having a warning in the editor:
| var animation_duration = blink_animation_player.get_animation("dissapear").length | |
| var animation_duration := blink_animation_player.get_animation("dissapear").length |
| #If the projectile collides with another object or is repelled the animation and timer will reset | ||
| if blink_animation_player: | ||
| if is_dissapearing: | ||
| reset_animation_and_timer() |
There was a problem hiding this comment.
Nice, you have factored out repeated code into reset_animation_and_timer(). There is more repeated code at the beginning of got_repelled() and _on_body_entered(). So what about doing more refactoring?
func _on_projectile_hit() -> void:
add_small_fx()
# Reset timers and blink animation:
duration_timer.start()
if blink_animation_player:
if is_dissapearing:
blink_animation_player.play("RESET")
blink_timer.start()
is_dissapearing = false
else:
blink_timer.start()
| ## The number of times the projectile blinks before disappearing. | ||
| @export_range(1, 10, 1) var blink_times: int = 3 | ||
|
|
||
| var is_dissapearing: bool = false |
There was a problem hiding this comment.
Now that there is a timer, is_dissapearing is not needed. You can check blink_timer.is_stopped() to know if the timer is not running (in other words the projectile is dissapearing). Or blink_animation_player.is_playing().
|
Hi, I followed all your advices, refactor some of the code, delete the is_dissapearing variable and now the time the animation will spend is fixed, the animation effect is still the same and now the minimun it will be shown is 3 seconds |
| ## root node. When the projectile gets hit, the [member GPUParticles2D.amount_ratio] is set to 1. | ||
| @export var trail_fx_scene: PackedScene | ||
|
|
||
| @export var blink_animation_player: AnimationPlayer |
There was a problem hiding this comment.
Please add a documentation comment explaining that this AnimationPlayer needs to have an animation called "dissapear". By the way, I think that the animation can be called "blink". And even better, we can add a warning in the editor when the AnimationPlayer that was set in the Inspector doesn't have one. But I can open a separate ticket for it.
Also please move the @export_group("Dissapear Clue") line above this one, so this export belongs to that group. Would you agree with that?
| ## The time in seconds to show the disappear animation. | ||
| @export_range(0.0, 10.0, 0.5) var blink_duration: float = 3.0 |
There was a problem hiding this comment.
"blink duration" reads to me as the duration of one blink. Sorry if I caused this trouble in previous reviews, but this should be named dissapear_duration to match the documentation comment.
| ## The number of times the projectile blinks before disappearing. | ||
| @export_range(1, 10, 1) var blink_times: int = 3 | ||
|
|
||
| var blink_effect_time: float |
There was a problem hiding this comment.
This variable is not needed, it can be calculated when needeed with duration, dissapear_duration and blink_times.
| ## How long the projectile lives in the scene. | ||
| ## [br][br] | ||
| ## This timer is restarted each time the projectile collides with anything. | ||
| ## This timer is restarted each time the projectile collies with anything. |
There was a problem hiding this comment.
| ## This timer is restarted each time the projectile collies with anything. | |
| ## This timer is restarted each time the projectile collides with anything. |
| if blink_animation_player: | ||
| #If the blink duration is greater than the duration | ||
| #The minimun time the animation will spend is 3 seconds. | ||
| var time_to_dissapear: float = max(3.0, duration - blink_duration) |
There was a problem hiding this comment.
Where does this 3 seconds come from? I think that if the dissapear duration is greater than the duration, it should start blinking immediately. You had this right before.
| big_fx_scene = ExtResource("4_41322") | ||
| trail_fx_scene = ExtResource("5_3lot6") | ||
| blink_animation_player = NodePath("AnimationPlayer") | ||
| blink_times = 5 |
There was a problem hiding this comment.
As requested before, let's reset this to the default.
| blink_timer.start() | ||
| else: | ||
| blink_timer.start() |
There was a problem hiding this comment.
If and else have the same content, is better to do:
| blink_timer.start() | |
| else: | |
| blink_timer.start() | |
| blink_timer.start() |



Affected scene: res://scenes/quests/template_quests/NO_EDIT/2_NO_EDIT_combat/NO_EDIT_combat_components/NO_EDIT_projectile.tscn
Affected script: res://scenes/game_elements/props/projectile/components/projectile.gd
When a projectile has spent the 80% of its duration, a dissapear animation will start. While this animation is playing the proyectile can be interacted with the repel comand and all Area2D nodes if posible, but this will no longer reset its duration.
An AnimationPlayer was added to the projectile scene and imported using the @onready annotation in the projectile.gd
Videocaptura.de.pantalla_20260827_193240.mp4
Fixes #2442