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

Impact Attribute Damage #109

Merged
merged 7 commits into from
Mar 18, 2024
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
35 changes: 35 additions & 0 deletions COGITO/Components/ImpactAttributeDamage.gd
Original file line number Diff line number Diff line change
@@ -0,0 +1,35 @@
extends Node3D
class_name ImpactAttributeDamage
# Attached to a RigidBody, this components will will damage a CogitoAttribute when the rigidbody collides with something.
@onready var parent_node = get_parent()

## This is what will take damage
@export var attribute : CogitoAttribute
## The minimum velocity at time of impact to take damage. This prevents light hits from damageing the attribute
@export var minimum_velocity : float = 1.0
## How much damage to do to the attribute
@export var damage : int
## Forced delay between impact times (in seconds).
@export var next_impact_time : float = 0.3

var time_passed : float
Copy link
Contributor

@FailSpy FailSpy Mar 12, 2024

Choose a reason for hiding this comment

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

Hmm, maybe you can replace time_passed with Godot's Timer functionality so this class doesn't need to track time. Can create and store a oneshot Timer dynamically in code on the _ready, and you can then reuse the single reference to the timer.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I basically copy pasted that from the impact sounds component. Is there an advantage to creating a whole new node for such a simple timer?

Copy link
Contributor

Choose a reason for hiding this comment

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

If you use SceneTreeTimer
It does not create a new node.

The advantage is really that there's not a duplication of time tracking in the backend. Godot pools timers together and ticks them together, as well it also standardizes timer tracking to make it easier to hook with other scripts. So it saves a cycle or so on _physics_process

Copy link
Collaborator Author

@ac-arcana ac-arcana Mar 14, 2024

Choose a reason for hiding this comment

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

I wasn't aware of SceneTreeTimer and I am just taking a look at it. I see that it does not create a node. However, it does still create a timer which is automatically freed once the time is elapsed. On top of this, I would probably need to store a boolean to track whether the timer has completed or not.
Isn't the timer still going to result in GC when it is freed? I'm used to thinking in C# terms, so this might not be the case for gdscript?
Maybe I'm missing a way to keep a reference to it once it has timed out?

Copy link
Contributor

Choose a reason for hiding this comment

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

It is, I don't think you're thinking of it wrong. I originally had a Timer node/resource in mind, so if you're using SceneTreeTimer, scratch the reusing reference thing.
The main thing that strikes me as making it worthwhile to add a SceneTreeTimer is that currently every ImpactAttribute component in a scene will be running cycles on _physics_process at all times to check if it should decrease its timer.

With using SceneTreeTimer, only ImpactAttribute components that have recently made an impact (and thusly, a timer is actually relevant) will be tracking time in an already existing section of Godot's backend that tracks time.

Copy link
Contributor

Choose a reason for hiding this comment

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

This is an extremely minor performance optimization BTW, I don't think that this is a necessary change at all, nor will really be felt by anyone. I was just throwing in this tidbit as a thing to be aware of overall.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I think I'm starting to get what you're suggesting now. If there is no physics process function at all checking a timer it will save a little bit of performance on these objects every frame.
So do you think it would be better to use SceneTreeTimer? Or would making and keeping a Timer node be better as it would prevent allocating a new timer and resulting GC for each collision?

Copy link
Contributor

Choose a reason for hiding this comment

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

I think a SceneTreeTimer is just neater if it's purely code based, given that the Timer node requires some configuration to make it one shot etc.


# Called when the node enters the scene tree for the first time.
func _ready() -> void:
if !parent_node.is_class("RigidBody3D"):
print("ImpactDamage: ", parent_node, " needs to be RigidBody3D.")
else:
parent_node.body_entered.connect(_on_parent_node_collision)


func _on_parent_node_collision(_collided_node):
if parent_node.linear_velocity.length() > minimum_velocity and time_passed == 0:
attribute.subtract(damage)
time_passed = next_impact_time


func _physics_process(delta: float) -> void:
if time_passed > 0:
time_passed -= delta
else:
time_passed = 0
7 changes: 7 additions & 0 deletions COGITO/Components/ImpactAttributeDamage.tscn
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
[gd_scene load_steps=2 format=3 uid="uid://b5cjuw2emh3wq"]

[ext_resource type="Script" path="res://COGITO/Components/ImpactAttributeDamage.gd" id="1_lfg44"]

[node name="ImpactAttributeDamage" type="Node3D"]
script = ExtResource("1_lfg44")
damage = 1