Multiplayer preparation #160
Conversation
|
WalkthroughThis pull request involves significant refactoring and feature additions across the Changes
Uplevel your code reviews with CodeRabbit ProCodeRabbit ProIf you like this project, please support us by purchasing the Pro version. The Pro version has advanced context, superior noise reduction and several proprietary improvements compared to the open source version. Moreover, CodeRabbit Pro is free for open source projects. |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| static func get_first_parent_of_type(node: Node, type: Variant): | ||
| var current_node = node | ||
| while current_node != null: | ||
| if is_instance_of(current_node, type): | ||
| return current_node | ||
| current_node = current_node.get_parent() | ||
| return null |
There was a problem hiding this comment.
The function get_first_parent_of_type could potentially lead to an infinite loop if the node hierarchy has a cycle. Although cycles in the node hierarchy are not common or recommended, it's still good practice to protect against this possibility. You can add a set to keep track of visited nodes and break the loop if a node is visited twice.
static func get_first_parent_of_type(node: Node, type: Variant):
+ var visited_nodes = {}
var current_node = node
while current_node != null:
+ if current_node in visited_nodes:
+ break
+ visited_nodes[current_node] = true
if is_instance_of(current_node, type):
return current_node
current_node = current_node.get_parent()
return null
| var node: Node | ||
|
|
||
| func _init(collectable_node: Node): | ||
| if collectable_node.actual_collectable_root_node: | ||
| node = collectable_node.actual_collectable_root_node.duplicate(15) | ||
| else: | ||
| node = collectable_node.duplicate(15) | ||
| node.position = Vector3.ZERO | ||
|
|
||
| icon = collectable_node.icon | ||
| display_name = collectable_node.display_name | ||
| description = collectable_node.description |
There was a problem hiding this comment.
The new code removes the logic for duplicating the collectable_node and setting its position to Vector3.ZERO. If this is intentional and you have moved this logic elsewhere, then it's fine. However, if this logic is still required, you should reintroduce it.
+ var node: Node
+
+ func _init(collectable_node: Node):
+ if collectable_node.actual_collectable_root_node:
+ node = collectable_node.actual_collectable_root_node.duplicate(15)
+ else:
+ node = collectable_node.duplicate(15)
+ node.position = Vector3.ZERO
+
+ icon = collectable_node.icon
+ display_name = collectable_node.display_name
+ description = collectable_node.description
| @@ -34,7 +34,7 @@ func process_other(node: NodotDebug): | |||
| var custom_fields = node.custom_watch_fields | |||
| var target_node = node.target_node | |||
| var values = get_custom_field_values(target_node, custom_fields) | |||
| data.setItem(uid, values) | |||
| data.set_item(uid, values) | |||
There was a problem hiding this comment.
The method call set_item has been updated from setItem. This change is consistent with the naming conventions in Godot, which uses snake_case for methods. However, please ensure that this change is propagated throughout the codebase to maintain consistency.
- data.setItem(uid, values)
+ data.set_item(uid, values)
| @@ -43,7 +43,7 @@ func process_state_machine(node: NodotDebug): | |||
| var values = get_custom_field_values(target_node, custom_fields) | |||
| values.current_state = target_node.get_name_from_id(target_node.current_state) | |||
| values.node_type = "StateMachine" | |||
| data.setItem(uid, values) | |||
| data.set_item(uid, values) | |||
There was a problem hiding this comment.
Similar to the previous comment, the method call set_item has been updated from setItem. Ensure that this change is propagated throughout the codebase to maintain consistency.
- data.setItem(uid, values)
+ data.set_item(uid, values)
| set_effect_speed() | ||
| set_effect_scale() |
There was a problem hiding this comment.
The new methods set_effect_speed() and set_effect_scale() are called without checking if the particle materials are initialized. This could potentially lead to null reference errors. Consider adding a check before calling these methods.
_setup_fire_particle()
_setup_smoke_particle()
_setup_sparks_particle()
+ if fire_particle.process_material and smoke_particle.process_material and spark_particle.process_material:
+ set_effect_speed()
+ set_effect_scale()
if emission_shape:
build_emission_shape(emission_shape)
emission_shape.connect("changed", _emission_shape_set)
| func set_effect_speed(): | ||
| if !fire_particle.process_material or !smoke_particle.process_material or !spark_particle.process_material: return | ||
| fire_particle.process_material.initial_velocity_min = 0.1 * effect_speed | ||
| fire_particle.process_material.initial_velocity_max = 5.0 * effect_speed | ||
| fire_particle.speed_scale = effect_speed | ||
| smoke_particle.process_material.initial_velocity_min = 0.1 * effect_speed | ||
| smoke_particle.process_material.initial_velocity_max = 5.0 * effect_speed | ||
| smoke_particle.speed_scale = effect_speed | ||
| spark_particle.process_material.initial_velocity_min = 0.1 * effect_speed | ||
| spark_particle.process_material.initial_velocity_max = 5.0 * effect_speed | ||
| spark_particle.speed_scale = effect_speed | ||
|
|
There was a problem hiding this comment.
In the set_effect_speed() method, you're setting the initial_velocity_min and initial_velocity_max for each particle type (fire, smoke, spark) to be proportional to effect_speed. However, this might not always be the desired behavior as it couples the initial velocity of particles with the effect speed. Consider introducing separate variables for controlling the initial velocities of different particle types.
func set_effect_speed():
if !fire_particle.process_material or !smoke_particle.process_material or !spark_particle.process_material: return
- fire_particle.process_material.initial_velocity_min = 0.1 * effect_speed
- fire_particle.process_material.initial_velocity_max = 5.0 * effect_speed
+ fire_particle.process_material.initial_velocity_min = 0.1 * fire_initial_velocity
+ fire_particle.process_material.initial_velocity_max = 5.0 * fire_initial_velocity
fire_particle.speed_scale = effect_speed
- smoke_particle.process_material.initial_velocity_min = 0.1 * effect_speed
- smoke_particle.process_material.initial_velocity_max = 5.0 * effect_speed
+ smoke_particle.process_material.initial_velocity_min = 0.1 * smoke_initial_velocity
+ smoke_particle.process_material.initial_velocity_max = 5.0 * smoke_initial_velocity
smoke_particle.speed_scale = effect_speed
- spark_particle.process_material.initial_velocity_min = 0.1 * effect_speed
- spark_particle.process_material.initial_velocity_max = 5.0 * effect_speed
+ spark_particle.process_material.initial_velocity_min = 0.1 * spark_initial_velocity
+ spark_particle.process_material.initial_velocity_max = 5.0 * spark_initial_velocity
spark_particle.speed_scale = effect_speedRemember to declare and initialize fire_initial_velocity, smoke_initial_velocity, and spark_initial_velocity at the appropriate place in your code.
| @@ -56,7 +56,7 @@ func _ready() -> void: | |||
| if magazine_node: | |||
| magazine_node.connect("discharged", func() : | |||
| emit_signal("discharged") | |||
| ) | |||
| ) | |||
There was a problem hiding this comment.
The indentation of the closing parenthesis is inconsistent with the rest of the code. It's a minor issue but it can affect readability and maintainability.
- )
+ )
| var colliders = get_colliding_bodies() | ||
| for collider in colliders: | ||
| if collider is not RigidBody3D: return | ||
| var max_velocity: float = collider.linear_velocity.length() | ||
| if max_velocity > environmental_damage_min_velocity: | ||
| var multiplier = max_velocity / environmental_damage_min_velocity | ||
| var damage = environmental_damage_multiplier * (environmental_damage_min_velocity * multiplier) | ||
| health.add_health(-damage) |
There was a problem hiding this comment.
The logic for calculating the max_velocity has been changed from considering the maximum of the object's own linear velocities in each axis to considering the velocity of colliding bodies. However, there is a potential issue with this new approach. The return statement on line 53 will exit the function if any collider is not a RigidBody3D, which means that if the first collider in the list is not a RigidBody3D, the function will return prematurely and won't consider other colliders that might be RigidBody3D. This could lead to incorrect damage calculation.
To fix this, you should continue the loop instead of returning from the function when encountering a non-RigidBody3D collider.
- if collider is not RigidBody3D: return
+ if collider is not RigidBody3D: continueThis way, all colliders are checked and only those that are RigidBody3D are considered for the damage calculation.
| func write(data: Variant) -> void: | ||
| if write_thread == null: | ||
| write_thread = Thread.new() | ||
| write_thread.start(_write_worker.bind(file_path)) | ||
| queue_mutex.lock() | ||
| write_queue.append(data) | ||
| queue_mutex.unlock() | ||
| queue_semaphore.post() |
There was a problem hiding this comment.
The write function starts a new thread every time it's called if write_thread is null. This could lead to multiple threads being created if write is called concurrently before the first call has had a chance to set write_thread. Consider starting the thread in an initialization function instead.
- if write_thread == null:
- write_thread = Thread.new()
- write_thread.start(_write_worker.bind(file_path))
| var file = FileAccess.open(file_path, FileAccess.READ) | ||
| if file == null: | ||
| read_completed.emit() | ||
| read_in_progress = false | ||
| return |
There was a problem hiding this comment.
There's no error handling for when FileAccess.open fails. It returns null on failure, but you're not checking for this case. You should add error handling to ensure that the file was opened successfully.
- var file = FileAccess.open(file_path, FileAccess.READ)
- if file == null:
- read_completed.emit()
- read_in_progress = false
- return
| var file = FileAccess.open(local_file_path, FileAccess.WRITE_READ) | ||
| file.seek_end() | ||
|
|
||
| while true: | ||
| queue_semaphore.wait() | ||
| if exit_thread: | ||
| break | ||
| queue_mutex.lock() | ||
| var data = null | ||
| if write_queue.size() > 0: | ||
| data = write_queue.pop_front() | ||
| queue_mutex.unlock() | ||
| if data != null: | ||
| file.store_var(data) | ||
| file.flush() | ||
| queue_mutex.lock() | ||
| var is_empty = write_queue.is_empty() | ||
| queue_mutex.unlock() | ||
| if is_empty: | ||
| _emit_write_queue_empty.call_deferred() | ||
| file.close() |
There was a problem hiding this comment.
In the _write_worker function, there's a potential race condition between checking if the write_queue is empty and popping from it. If another thread appends to the queue after the check but before the pop, the queue could be left in an inconsistent state. To fix this, you should move the pop operation inside the critical section protected by the mutex.
- queue_mutex.lock()
- var data = null
- if write_queue.size() > 0:
- data = write_queue.pop_front()
- queue_mutex.unlock()
- if data != null:
- file.store_var(data)
- file.flush()
- queue_mutex.lock()
- var is_empty = write_queue.is_empty()
- queue_mutex.unlock()
- if is_empty:
- _emit_write_queue_empty.call_deferred()
| while true: | ||
| queue_mutex.lock() | ||
| var is_empty = write_queue.is_empty() | ||
| queue_mutex.unlock() | ||
| if is_empty: | ||
| return | ||
| await get_tree().process_frame |
There was a problem hiding this comment.
The wait_until_write_queue_empty function could potentially cause high CPU usage because it's continuously polling the queue in a tight loop. Consider using a condition variable or semaphore to wait until the queue is empty.
- while true:
- queue_mutex.lock()
- var is_empty = write_queue.is_empty()
- queue_mutex.unlock()
- if is_empty:
- return
- await get_tree().process_frame
| func delete(): | ||
| var dir: DirAccess = DirAccess.open(file_path.get_base_dir()) | ||
| if dir != null and dir.file_exists(file_path): | ||
| dir.remove(file_path) |
There was a problem hiding this comment.
The delete function doesn't handle potential errors when removing a file. The remove method returns a boolean indicating success or failure, but you're not checking this value. You should add error handling to ensure that the file was deleted successfully.
- if dir != null and dir.file_exists(file_path):
- dir.remove(file_path)
| func _exit_tree(): | ||
| finish() |
There was a problem hiding this comment.
In the _exit_tree function, you're calling finish, which waits for the write thread to finish. However, if the write thread is blocked waiting for data to write, it will never finish, and your program could hang. Consider adding a timeout to the wait_to_finish call.
- if write_thread != null and write_thread.is_alive():
- write_thread.wait_to_finish()
| action() | ||
|
|
||
| func action(): | ||
| if not character.is_authority(): return | ||
|
|
||
| if enabled and character.input_enabled: | ||
| character.direction = Input.get_vector(left_action, right_action, up_action, down_action) | ||
| character.input_states["direction"] = get_input() |
There was a problem hiding this comment.
The action function is called in _physics_process without any arguments. However, it seems like the action function doesn't do anything if the character is not an authority or if the character's input is not enabled. This could potentially lead to unnecessary calls to the action function. Consider adding a condition before calling action to check if the character is an authority and if the character's input is enabled.
- action()
+ if character.is_authority() and enabled and character.input_enabled:
+ action()
| if get_input(): | ||
| reload() | ||
|
|
||
| func _physics_process(delta: float) -> void: | ||
| action() | ||
|
|
||
| func get_input(): | ||
| return InputMap.has_action(reload_action) and Input.is_action_pressed(reload_action) |
There was a problem hiding this comment.
The get_input() function is a good abstraction for checking if the reload action is pressed. However, it's not clear from the function name that it's specifically checking for the reload action. Consider renaming this function to something more descriptive, like is_reload_action_pressed().
- func get_input():
+ func is_reload_action_pressed():
return InputMap.has_action(reload_action) and Input.is_action_pressed(reload_action)
| func action(): | ||
| var mouse_action = character.input_states.get("mouse_action") | ||
| match mouse_action: | ||
| "next_item": | ||
| next_item() | ||
| "previous_item": | ||
| previous_item() | ||
| "action": | ||
| discharge() | ||
| "release_action": | ||
| release_action(); | ||
| "zoom": | ||
| zoom() | ||
| "zoomout": | ||
| zoomout() |
There was a problem hiding this comment.
The action() function has been significantly refactored to handle different mouse actions. This is a good approach as it makes the code more modular and easier to maintain. However, there seems to be a typo in line 152 where a semicolon is used at the end of the line. In GDScript, semicolons are optional and typically not used unless multiple statements are on the same line.
- 152: release_action();
+ 152: release_action()
| var last_collider: Node3D | ||
| var last_focussed_collider: Node3D; | ||
| var last_focussed_collider: Node3D | ||
| var carried_body_prev_mask: int = 1 | ||
| var carried_body_prev_layer: int = 1 | ||
| var carried_body_physics_material: PhysicsMaterial | ||
| var is_action_pressed: bool = false; | ||
| var character: CharacterBody3D | ||
|
|
||
| func _ready(): | ||
| if not is_multiplayer_authority(): return | ||
|
|
||
| character = Nodot.get_first_parent_of_type(self, CharacterBody3D) | ||
| InputManager.register_action(interact_action, KEY_F) | ||
|
|
||
| func _input(event: InputEvent): | ||
| if !enabled or !event.is_action_pressed(interact_action) or !is_multiplayer_authority(): return | ||
| if is_action_pressed: return; | ||
| func _physics_process(delta: float): | ||
| if !is_multiplayer_authority(): return | ||
|
|
||
| var collider = get_collider() | ||
| if collider and collider.has_meta("NonPickable") and collider.get_meta("NonPickable"): return | ||
| if !is_instance_valid(collider): | ||
| if is_instance_valid(carried_body): | ||
| carry_end(); | ||
| return | ||
|
|
||
| emit_signal("interacted", collider, get_collision_point(), get_collision_normal()) | ||
| if collider.has_method("interact"): | ||
| collider.interact() | ||
| GlobalSignal.trigger_signal("interacted", [collider, get_collision_point(), get_collision_normal()]); | ||
| else: | ||
| if is_instance_valid(carried_body): | ||
| carry_end(); | ||
| else: | ||
| carry_begin(collider) | ||
|
|
||
|
|
||
| func _physics_process(delta): | ||
| if !InputMap.has_action("action"): return | ||
| var is_action_pressed: bool = character.input_states.get("mouse_action") or false | ||
| character.input_states["interact"] = get_input() | ||
| if character.input_states["interact"]: | ||
| action() | ||
|
|
||
| is_action_pressed = Input.is_action_pressed("action"); | ||
| if is_instance_valid(carried_body): | ||
| if not multiplayer.is_server(): return | ||
| if not is_close_body_carry: | ||
| var point = get_collision_point() | ||
| var carry_position = global_transform.origin - global_transform.basis.z.normalized() * (carry_distance + carried_body_width) | ||
| var current_carry_distance = carried_body.global_position.distance_to(global_position) | ||
| if current_carry_distance > carry_distance + max_carry_distance: | ||
| carry_end(); | ||
| return | ||
| if is_action_pressed: | ||
| throw(); | ||
| return; | ||
| var speed = carried_body.global_position.distance_to(carry_position) * 600 | ||
| carried_body.linear_velocity = carried_body.global_transform.origin.direction_to(carry_position) * speed * delta | ||
| else: | ||
| var carry_position = carry_position_node.global_position; | ||
| carried_body.global_position = carry_position; | ||
| if is_action_pressed: | ||
| var prev_carried_body = carried_body; | ||
| throw(); | ||
| return; | ||
| var point = get_collision_point() | ||
| var carry_position = global_transform.origin - global_transform.basis.z.normalized() * (carry_distance + carried_body_width) | ||
| var current_carry_distance = carried_body.global_position.distance_to(global_position) | ||
| if current_carry_distance > carry_distance + max_carry_distance: | ||
| carry_end() | ||
| return | ||
| if is_action_pressed: | ||
| throw() | ||
| return | ||
| var speed = carried_body.global_position.distance_to(carry_position) * 600 | ||
| carried_body.linear_velocity = carried_body.global_transform.origin.direction_to(carry_position) * speed * delta | ||
|
|
There was a problem hiding this comment.
The _physics_process function has been refactored. The is_multiplayer_authority() check is now at the start of the function, which is a good practice to prevent unnecessary computations in non-authoritative instances. However, it seems that the is_close_body_carry logic has been removed. If this was intentional and the functionality is no longer needed, then the changes are fine. But if this functionality is still required, you might need to reintroduce it.
- if not is_close_body_carry:
- var point = get_collision_point()
- var carry_position = global_transform.origin - global_transform.basis.z.normalized() * (carry_distance + carried_body_width)
- var current_carry_distance = carried_body.global_position.distance_to(global_position)
- if current_carry_distance > carry_distance + max_carry_distance:
- carry_end();
- return
- if is_action_pressed:
- throw();
- return;
- var speed = carried_body.global_position.distance_to(carry_position) * 600
- carried_body.linear_velocity = carried_body.global_transform.origin.direction_to(carry_position) * speed * delta
- else:
- var carry_position = carry_position_node.global_position;
- carried_body.global_position = carry_position;
- if is_action_pressed:
- var prev_carried_body = carried_body;
- throw();
- return;
| func action(): | ||
| if !enabled or !is_multiplayer_authority(): return | ||
| if character.input_states.get("mouse_action"): return | ||
|
|
||
| var collider = get_collider() | ||
| if collider and collider.has_meta("NonPickable") and collider.get_meta("NonPickable"): return | ||
| if !is_instance_valid(collider): | ||
| if is_instance_valid(carried_body): | ||
| carry_end() | ||
| return | ||
|
|
||
| emit_signal("interacted", collider, get_collision_point(), get_collision_normal()) | ||
| if collider.has_method("interact"): | ||
| collider.interact() | ||
| GlobalSignal.trigger_signal("interacted", [collider, get_collision_point(), get_collision_normal()]) | ||
| else: | ||
| if is_instance_valid(carried_body): | ||
| carry_end() | ||
| else: | ||
| carry_begin(collider) |
There was a problem hiding this comment.
The action function has been updated to use the character's input states instead of directly checking for input events. This is a good change as it decouples the input handling from the action execution, making the code more modular and easier to maintain. However, please ensure that the character.input_states dictionary is always properly initialized and updated, as it's now a critical part of the interaction logic.
- if !enabled or !event.is_action_pressed(interact_action) or !is_multiplayer_authority(): return
- if is_action_pressed: return;
+ if !enabled or !is_multiplayer_authority(): return
+ if character.input_states.get("mouse_action"): return
| func carry_begin(collider: Node): | ||
| emit_signal("interaction_label_updated", "") | ||
| if enable_pickup and is_instance_valid(collider) and collider is RigidBody3D and collider.mass <= max_mass: | ||
| carried_body = collider |
There was a problem hiding this comment.
The carry_begin function has been refactored. The is_close_body_carry logic has been removed. If this was intentional and the functionality is no longer needed, then the changes are fine. But if this functionality is still required, you might need to reintroduce it.
- is_close_body_carry = carried_body.has_meta("carry_close") and carried_body.get_meta("carry_close")
| carried_body.angular_velocity = Vector3.ZERO | ||
| carried_body.collision_layer = carried_body_prev_layer | ||
| carried_body.collision_mask = carried_body_prev_mask | ||
| carried_body.physics_material_override = carried_body_physics_material | ||
| emit_signal("carry_ended", carried_body) | ||
| GlobalSignal.trigger_signal("carry_ended", carried_body) | ||
| carried_body = null | ||
|
|
||
| func _carry_body_end(): | ||
| carried_body.collision_layer = carried_body_prev_layer | ||
| carried_body.collision_mask = carried_body_prev_mask | ||
| carried_body.physics_material_override = carried_body_physics_material | ||
| emit_signal("carry_ended", carried_body) | ||
| GlobalSignal.trigger_signal("carry_ended", carried_body); | ||
| carried_body = null; | ||
| GlobalSignal.trigger_signal("carry_ended", carried_body) | ||
| carried_body = null | ||
|
|
||
| func throw(): | ||
| if is_instance_valid(carried_body): | ||
| carried_body.angular_velocity = Vector3.ZERO | ||
| carried_body.apply_force(-global_transform.basis.z * throw_force); | ||
| carried_body.apply_force(-global_transform.basis.z * throw_force) |
There was a problem hiding this comment.
The carry_end, _carry_body_end, and throw functions have been refactored. The is_close_body_carry logic has been removed from carry_end. If this was intentional and the functionality is no longer needed, then the changes are fine. But if this functionality is still required, you might need to reintroduce it.
- if is_close_body_carry:
- var prev_carried_body = carried_body;
- carried_body.physics_material_override = carried_body_physics_material
- carried_body.apply_force(-global_transform.basis.z * throw_force);
- emit_signal("carry_ended", carried_body)
- GlobalSignal.trigger_signal("carry_ended", carried_body);
- carried_body = null;
- await get_tree().create_timer(0.5).timeout
- if is_instance_valid(prev_carried_body):
- prev_carried_body.collision_layer = carried_body_prev_layer
- prev_carried_body.collision_mask = carried_body_prev_mask
| if SaveManager.config.has_item("display_mode"): | ||
| display_mode = SaveManager.config.get_item("display_mode") | ||
| if SaveManager.config.has_item("screen"): | ||
| screen = SaveManager.config.get_item("screen") | ||
| if SaveManager.config.has_item("fps_limit"): | ||
| fps_limit = SaveManager.config.get_item("fps_limit") | ||
| if SaveManager.config.has_item("vsync"): | ||
| vsync = SaveManager.config.get_item("vsync") | ||
| if SaveManager.config.has_item("msaa"): | ||
| msaa = SaveManager.config.get_item("msaa") | ||
| if SaveManager.config.has_item("brightness"): | ||
| brightness = SaveManager.config.get_item("brightness") | ||
| if SaveManager.config.has_item("contrast"): | ||
| contrast = SaveManager.config.get_item("contrast") | ||
| if SaveManager.config.has_item("low_spec"): | ||
| low_spec = SaveManager.config.get_item("low_spec") | ||
| if SaveManager.config.has_item("post_process"): | ||
| post_process = SaveManager.config.get_item("post_process") |
There was a problem hiding this comment.
The code for checking if a configuration item exists and then getting the item is repeated multiple times. This could be simplified by creating a helper function that takes the key as an argument, checks if it exists, and then returns the value if it does.
- if SaveManager.config.has_item("display_mode"):
- display_mode = SaveManager.config.get_item("display_mode")
+ display_mode = get_config_item("display_mode", display_mode)
func get_config_item(key: String, default_value):
return SaveManager.config.has_item(key) ? SaveManager.config.get_item(key) : default_value
| SaveManager.config.set_item("display_mode", display_mode) | ||
| SaveManager.config.set_item("screen", screen) | ||
| SaveManager.config.set_item("fps_limit", fps_limit) | ||
| SaveManager.config.set_item("vsync", vsync) | ||
| SaveManager.config.set_item("msaa", msaa) | ||
| SaveManager.config.set_item("brightness", brightness) | ||
| SaveManager.config.set_item("contrast", contrast) | ||
| SaveManager.config.set_item("low_spec", low_spec) | ||
| SaveManager.config.set_item("post_process", post_process) |
There was a problem hiding this comment.
Similar to the previous comment, the code for setting a configuration item is repeated multiple times. This could be simplified by creating a helper function that takes the key and value as arguments and sets the item.
- SaveManager.config.set_item("display_mode", display_mode)
+ set_config_item("display_mode", display_mode)
func set_config_item(key: String, value):
SaveManager.config.set_item(key, value)
| func physics(delta: float) -> void: | ||
| if not is_authority(): return | ||
| action(delta) | ||
|
|
||
| func action(delta: float) -> void: | ||
| if !character.input_enabled: return | ||
|
|
||
| var character_look_angle: Vector2 = character.input_states["look_angle"] | ||
|
|
||
| # Handle look left and right | ||
| character.rotate_object_local(Vector3(0, 1, 0), character.look_angle.x) | ||
| character.rotate_object_local(Vector3(0, 1, 0), character_look_angle.x) | ||
|
|
||
| # Handle look up and down | ||
| head.rotate_object_local(Vector3(1, 0, 0), character.look_angle.y) | ||
| head.rotate_object_local(Vector3(1, 0, 0), character_look_angle.y) |
There was a problem hiding this comment.
The new code introduces a new function action(delta: float) -> void: which encapsulates the logic for character rotation based on input. However, it seems like the check if not is_authority(): return from the old code has been removed. This check is important in multiplayer games to ensure that only the authoritative node (usually the server or the owner of the node) can control the character's movement. Without this check, every client would be able to control the character leading to unpredictable behavior.
func physics(delta: float) -> void:
action(delta)
func action(delta: float) -> void:
+ if not is_authority(): return
if !character.input_enabled: return
var character_look_angle: Vector2 = character.input_states["look_angle"]
# Handle look left and right
character.rotate_object_local(Vector3(0, 1, 0), character_look_angle.x)
# Handle look up and down
head.rotate_object_local(Vector3(1, 0, 0), character_look_angle.y)Please consider adding this check back into your code to prevent potential issues in a multiplayer context.
| if new_state == state_ids["jump"]: | ||
| jump() | ||
|
|
||
| func jump() -> void: | ||
| character.velocity.y = jump_velocity | ||
|
|
||
| func physics(delta: float) -> void: | ||
|
|
There was a problem hiding this comment.
The jump function is now public, which could potentially allow external scripts to trigger a jump. If this is not intended, consider making it private by prefixing with an underscore (_jump). Also, there's no check if the character is on the floor before jumping. This might lead to mid-air jumps.
- func jump() -> void:
+ func _jump() -> void:
+ if not can_jump(): return
character.velocity.y = jump_velocity
| func input(event: InputEvent): | ||
| if not is_authority_owner(): return | ||
|
|
||
| if !character.was_on_floor: | ||
| return | ||
|
|
||
| if Input.is_action_pressed(jump_action): | ||
| if not character.floor_body.has_meta("soft_floor") or character.floor_body.has_meta("can_jump"): | ||
| sm.set_state(state_ids["jump"]) | ||
| if event.is_action_pressed(jump_action): | ||
| character.input_states["jump"] = true | ||
|
|
There was a problem hiding this comment.
The input function lacks a check for whether the character is on the floor before setting the "jump" input state to true. This could potentially lead to mid-air jumps.
if event.is_action_pressed(jump_action):
+ if can_jump():
character.input_states["jump"] = true
| func can_jump(): | ||
| if character.was_on_floor: | ||
| return true | ||
| return false |
There was a problem hiding this comment.
The can_jump function could be simplified to directly return the condition result instead of using an if-else structure.
- if character.was_on_floor:
- return true
- return false
+ return character.was_on_floor
| if !can_jump(): return | ||
|
|
||
| if character.input_states.get("jump"): | ||
| if character.floor_body and character.floor_body.has_meta("soft_floor"): return | ||
| sm.set_state(state_ids["jump"]) | ||
| elif sm.state == state_ids["jump"]: | ||
| sm.set_state(state_ids["land"]) | ||
| elif sm.state == state_ids["land"]: | ||
| sm.set_state(state_ids["idle"]) |
There was a problem hiding this comment.
In the action function, there's no check for whether the character is on the floor before transitioning from the "land" state to the "idle" state. This could potentially lead to incorrect state transitions if the character lands and then falls off a platform.
elif sm.state == state_ids["land"]:
+ if character.was_on_floor:
sm.set_state(state_ids["idle"])
| @@ -14,6 +14,7 @@ class_name CharacterProne3D extends CharacterExtensionBase3D | |||
| ## The input action name for proning | |||
| @export var prone_action: String = "prone" | |||
|
|
|||
| var head: Node3D | |||
There was a problem hiding this comment.
The variable head is declared but not initialized. This could lead to null reference exceptions if it's used before being assigned a value. Consider initializing it with a default value or ensure it's assigned before use.
- var head: Node3D
+ var head: Node3D = null
|
|
||
| initial_head_position = character.head_position | ||
|
|
||
| head = character.get_node("Head") |
There was a problem hiding this comment.
The get_node("Head") method can throw an error if the node does not exist. It would be better to handle this potential error using get_node_or_null("Head") and check if the node exists before using it.
- head = character.get_node("Head")
+ head = character.get_node_or_null("Head")
+ if head == null:
+ print_error("Head node not found")
+ return
| @@ -64,6 +66,6 @@ func physics(delta: float) -> void: | |||
| sm.set_state(state_ids["stand"]) | |||
|
|
|||
| if sm.state == state_ids["prone"]: | |||
| character.head.position = lerp(character.head.position, target_head_position, 0.1) | |||
| head.position = lerp(head.position, target_head_position, 0.1) | |||
There was a problem hiding this comment.
The lerp function is used to interpolate the head position, but there's no check to ensure that head is not null. This could lead to null reference exceptions. Add a null check for head before using it.
- head.position = lerp(head.position, target_head_position, 0.1)
+ if head != null:
+ head.position = lerp(head.position, target_head_position, 0.1)
| var look_angle = character.input_states["look_angle"] | ||
|
|
||
| # Handle look left and right | ||
| if lock_character_rotation: | ||
| character.rotate_object_local(Vector3(0, 1, 0), character.look_angle.y) | ||
| character.rotate_object_local(Vector3(0, 1, 0), look_angle.y) | ||
| else: | ||
| camera_container.rotate_object_local(Vector3(0, 1, 0), character.look_angle.y) | ||
| camera_container.rotate_object_local(Vector3(0, 1, 0), look_angle.y) | ||
|
|
||
| # Handle look up and down | ||
| rotate_object_local(Vector3(1, 0, 0), character.look_angle.x) | ||
| rotate_object_local(Vector3(1, 0, 0), look_angle.x) |
There was a problem hiding this comment.
The change from character.look_angle.y to character.input_states["look_angle"] could potentially introduce a KeyError if the "look_angle" key is not present in the input_states dictionary. It would be safer to use the get() method which returns None if the key is not found, and then handle the case where the value is None.
- var look_angle = character.input_states["look_angle"]
+ var look_angle = character.input_states.get("look_angle")
+ if look_angle is None:
+ returnThis way, you avoid potential crashes due to missing keys in the dictionary.
| @@ -10,6 +10,7 @@ class_name NodotCharacter3D extends CharacterBody3D | |||
| signal current_camera_changed(old_camera: Camera3D, new_camera: Camera3D) | |||
|
|
|||
| var current_camera: Camera3D | |||
| var input_states: Dictionary = {} | |||
There was a problem hiding this comment.
The input_states variable is initialized as an empty dictionary but it's not clear how and where it's used. If it's not used, consider removing it to avoid confusion.
- var input_states: Dictionary = {}
| if !is_current_player: | ||
| return | ||
|
|
||
| func set_current_camera(camera3d: Camera3D): |
There was a problem hiding this comment.
The function set_current_camera no longer checks if the current object is the player before setting the camera. This could potentially lead to issues if non-player characters are trying to change the camera. If this check is still necessary, consider adding it back.
+ func set_current_camera(camera3d: Camera3D):
+ if current_camera != camera3d:
- func set_current_camera(camera3d: Camera3D):
- if !is_current_player:
- return
- if current_camera != camera3d:
|
|
||
| current_camera = camera3d | ||
| current_camera.current = true | ||
|
|
||
| toggle_viewport_camera(camera3d == camera) | ||
|
|
||
| ## Reset the active camera to the character |
There was a problem hiding this comment.
The function set_current_camera now sets the new camera as the current one without hiding the previous camera or showing the new one in the viewport. This might cause multiple cameras to be active at the same time. If the previous behavior was intended, consider adding it back.
+ current_camera = camera3d
+ current_camera.current = true
- current_camera = camera3d
- toggle_viewport_camera(camera3d == camera)
|
|
||
| func _physics_process(delta: float) -> void: | ||
| if !character or not character.is_authority(): return | ||
| action() | ||
|
|
||
| func action(): |
There was a problem hiding this comment.
The _physics_process function is now calling the action function, but there's no check to see if character is null. This could potentially lead to a null reference error. Consider adding a null check for character in the action function.
func action():
+ if not character: return
if not character.is_authority(): return
| if enabled and character.input_enabled: | ||
| character.direction = Input.get_vector(left_action, right_action, up_action, down_action) | ||
| character.input_states["direction"] = get_input() |
There was a problem hiding this comment.
The direction of the character is now being set in a different way. Instead of directly setting character.direction, it's now being set as an input state. If this change is intentional and the input_states dictionary is properly handled elsewhere in the code, then this should be fine. However, if input_states is not used or updated elsewhere, this could lead to unexpected behavior.
| character.input_states["sprint"] = get_input() | ||
| action(delta) | ||
|
|
||
| func action(delta: float): | ||
| var basis: Basis | ||
| if third_person_camera: | ||
| basis = character.current_camera.global_transform.basis | ||
| else: | ||
| basis = character.transform.basis | ||
| direction = (basis * Vector3(character.direction.x, 0, character.direction.y)) | ||
| var character_direction: Vector2 = character.input_states["direction"] | ||
| direction = (basis * Vector3(character_direction.x, 0, character_direction.y)) | ||
|
|
||
| if character.input_enabled and direction != Vector3.ZERO: | ||
| if Input.is_action_pressed(sprint_action): | ||
| if get_input(): | ||
| sm.set_state(state_ids["sprint"]) | ||
| else: | ||
| sm.set_state(state_ids["walk"]) | ||
|
|
||
| if !character.was_on_floor: | ||
| move_air(delta) | ||
| character.move_and_slide() | ||
| else: | ||
| move_ground(delta) | ||
| stair_step() | ||
| character.velocity.y = lerp(character.velocity.y, 0.0, delta * 2.0) | ||
|
|
||
| set_rigid_interaction(); | ||
| set_rigid_interaction() | ||
|
|
||
| func get_input(): | ||
| return Input.is_action_pressed(sprint_action) |
There was a problem hiding this comment.
The physics function has been refactored and a new function action has been introduced. However, the call to character.move_and_slide() has been moved from the physics function to the if !character.was_on_floor: condition in the action function. This could potentially cause issues if the character is not on the floor, as the move_and_slide method is essential for applying gravity and handling slopes. Please ensure that this change does not affect the character's movement when it is not on the floor.
- if !character.was_on_floor:
- move_air(delta)
- else:
- move_ground(delta)
+ if character.input_enabled and direction != Vector3.ZERO:
+ if get_input():
+ sm.set_state(state_ids["sprint"])
+ else:
+ sm.set_state(state_ids["walk"])
+
+ if !character.was_on_floor:
+ move_air(delta)
+ character.move_and_slide()
+ else:
+ move_ground(delta)
+ stair_step()
+ character.velocity.y = lerp(character.velocity.y, 0.0, delta * 2.0)
|
|
||
| func stair_step(): |
There was a problem hiding this comment.
The stair step logic has been encapsulated into a new function stair_step(). This improves the readability of the code by abstracting away the details of the stair stepping logic. However, please ensure that this function is called at the appropriate places in your code to maintain the correct behavior.
| character.global_position = slide_position | ||
| # --- Step up logic --- | ||
|
|
There was a problem hiding this comment.
The line character.velocity.y = lerp(character.velocity.y, 0.0, delta * 2.0) has been removed. This line was responsible for gradually reducing the y-component of the character's velocity towards zero. If this line is removed, the character's vertical velocity might not decrease when it should, which could lead to incorrect behavior. Please ensure that this change does not affect the character's movement.
- # --- Step up logic ---
+ # --- Step up logic ---
+ character.velocity.y = lerp(character.velocity.y, 0.0, delta * 2.0)
| @@ -53,6 +55,7 @@ func action(index: int = -1) -> void: | |||
| _tweak_pitch() | |||
|
|
|||
| play() | |||
| emit_signal("played") | |||
There was a problem hiding this comment.
The addition of the emit_signal("played") is a good way to notify other parts of the system that the sound has been played. However, it's important to ensure that there are handlers for this signal in the relevant parts of the codebase to avoid unhandled signal warnings.
+ emit_signal("played")
| @@ -39,7 +39,7 @@ func _physics_process(delta: float) -> void: | |||
| enabled | |||
| and distance_traveled > frequency | |||
| and parent._is_on_floor() | |||
| and parent.velocity != Vector3.ZERO | |||
| and Vector3(parent.velocity.x, 0.0, parent.velocity.z) != Vector3.ZERO | |||
There was a problem hiding this comment.
The change from comparing the entire velocity vector to just the x and z components might lead to unexpected behavior. If the intention is to ignore vertical movement (y component), it would be better to explicitly state this in a comment for clarity. However, if the y component should be considered, then reverting to the old comparison would be more appropriate.
- and Vector3(parent.velocity.x, 0.0, parent.velocity.z) != Vector3.ZERO
+ and parent.velocity != Vector3.ZEROIf ignoring the y component is intentional, consider adding a comment:
# Ignoring vertical movement (y component) in velocity check
and Vector3(parent.velocity.x, 0.0, parent.velocity.z) != Vector3.ZERO
| ## Capture input and write to buffer | ||
| func _input(event: InputEvent) -> void: | ||
| if enabled: | ||
| if event is InputEventMouseMotion: | ||
| mouse_rotation.y = event.relative.x * mouse_sensitivity | ||
| mouse_rotation.x = event.relative.y * mouse_sensitivity | ||
| camera.time_since_last_move = 0.0 | ||
|
|
||
| if not is_multiplayer_authority(): return | ||
| if !enabled or !character.input_enabled: return | ||
|
|
||
| if event is InputEventMouseMotion: | ||
| # Add mouse movement to buffer | ||
| input_buffer.append({"type": "mouse_motion", "event": event.relative}) | ||
| camera.time_since_last_move = 0.0 | ||
|
|
||
| if Input.is_action_pressed(item_next_action): | ||
| input_buffer.append({"type": "mouse_action", "action": "next_item"}) | ||
| elif Input.is_action_pressed(item_previous_action): | ||
| input_buffer.append({"type": "mouse_action", "action": "previous_item"}) | ||
| elif Input.is_action_pressed(action_action): | ||
| input_buffer.append({"type": "mouse_action", "action": "action"}) | ||
| elif Input.is_action_just_released(action_action): | ||
| input_buffer.append({"type": "mouse_action", "action": "release_action"}) | ||
|
|
There was a problem hiding this comment.
The _input function is capturing and buffering input events. However, it's not clear how the buffer size is managed. If the buffer grows indefinitely, this could lead to memory issues. Consider adding a mechanism to limit the buffer size or clear it at regular intervals.
| func _physics_process(delta: float) -> void: | ||
| if is_editor or character and character.is_authority_owner() == false: return | ||
| if !enabled or !character.input_enabled: return | ||
| action(delta) | ||
|
|
||
| func action(delta: float): | ||
| if not is_multiplayer_authority(): return | ||
| if !enabled or is_editor or !character.input_enabled: return | ||
|
|
||
| if Input.is_action_pressed(camera_rotate_action): return | ||
|
|
||
| var look_angle: Vector2 = Vector2(-mouse_rotation.x * delta, -mouse_rotation.y * delta) | ||
| character.look_angle = Vector2(look_angle.x, look_angle.y) | ||
| mouse_rotation = Vector2.ZERO | ||
| var input: Dictionary = get_input(delta) | ||
|
|
||
| character.input_states["mouse_action"] = input.mouse_action | ||
| character.input_states["look_angle"] = input.look_angle | ||
|
|
There was a problem hiding this comment.
In the action function, there are checks for is_multiplayer_authority(), enabled, is_editor, and character.input_enabled. These checks are also present in the _input and _physics_process functions. To improve maintainability and adhere to the DRY principle, consider extracting these checks into a separate function that can be called from each of these methods.
+ func is_input_allowed() -> bool:
+ return is_multiplayer_authority() and enabled and not is_editor and character.input_enabled
- if not is_multiplayer_authority(): return
- if !enabled or is_editor or !character.input_enabled: return
+ if not is_input_allowed(): return
| func get_input(delta: float) -> Dictionary: | ||
| var look_angle: Vector2 = Vector2.ZERO | ||
| var mouse_action: String = "" | ||
|
|
||
| for entry in input_buffer: | ||
| if entry["type"] == "mouse_motion": | ||
| var mouse_movement: Vector2 = entry["event"] | ||
| look_angle.y -= mouse_movement.x * InputManager.mouse_sensitivity * delta | ||
| look_angle.x -= mouse_movement.y * InputManager.mouse_sensitivity * delta | ||
| elif entry["type"] == "mouse_action": | ||
| mouse_action = entry["action"] | ||
|
|
||
| # Clear the buffer after processing | ||
| input_buffer.clear() | ||
|
|
||
| # Return final processed input | ||
| return { | ||
| "look_angle": Vector2(look_angle.x, look_angle.y), | ||
| "mouse_action": mouse_action | ||
| } |
There was a problem hiding this comment.
The get_input function processes the buffered input and clears the buffer afterwards. This approach might cause loss of input data if the function is called multiple times within a single frame. Consider changing the design to ensure that input data is not lost between frames.
| @@ -11,7 +11,7 @@ config_version=5 | |||
| [application] | |||
|
|
|||
| config/name="Nodot" | |||
| config/features=PackedStringArray("4.2", "Forward Plus") | |||
| config/features=PackedStringArray("4.3", "Forward Plus") | |||
There was a problem hiding this comment.
The config/features value has been updated from "4.2" to "4.3". Ensure that this change is intentional and that your project is compatible with the new version.
| @onready var menu_container: MenuContainer = get_parent() | ||
| @onready var menu_manager: MenuManager = get_parent().get_parent() | ||
|
|
||
| func _input(event: InputEvent): | ||
| if menu_container.visible and event.is_action_pressed(\"escape\"): | ||
| menu_manager.go_back() | ||
| " | ||
|
|
There was a problem hiding this comment.
The new code introduces a get_first_parent_of_type function, but it's not clear how this function is implemented. Ensure that the function correctly retrieves the first parent of the specified type in the node hierarchy.
| [sub_resource type="GDScript" id="GDScript_l8cix"] | ||
| resource_name = "displaymode" | ||
| script/source = "extends MenuButton | ||
|
|
||
| func hide_contents(): | ||
| for tab in tab_contents: | ||
| tab.visible = false | ||
| " | ||
| @onready var popup = get_popup() | ||
|
|
||
| [sub_resource type="GDScript" id="GDScript_yprk4"] | ||
| resource_name = "closebtn" | ||
| script/source = "extends Button | ||
| const display_mode_names = [\"Windowed\", \"x\", \"x\", \"Fullscreen\", \"Exclusive Fullscreen\"] | ||
|
|
||
| @onready var menu_manager: MenuManager = %OptionsMenuControl.menu_manager | ||
| func _ready(): | ||
| popup.connect(\"id_pressed\", _on_pressed) | ||
| update_text() | ||
|
|
||
| func _on_pressed(): | ||
| AudioManager.save_config() | ||
| VideoManager.save_config() | ||
| InputManager.save_config() | ||
| menu_manager.go_back() | ||
| func _on_mouse_entered(): | ||
| %OptionsMenuControl.get_node(\"SFX/HoverTick\").play(); | ||
|
|
||
| func _on_pressed(item_id: int): | ||
| %OptionsMenuControl.get_node(\"SFX/ClickTick\").play(); | ||
| VideoManager.display_mode = item_id as DisplayServer.WindowMode | ||
| update_text() | ||
|
|
||
| func update_text(): | ||
| text = display_mode_names[VideoManager.display_mode] | ||
| " | ||
|
|
||
| [sub_resource type="GDScript" id="GDScript_l8cix"] | ||
| resource_name = "displaymode" | ||
| [sub_resource type="GDScript" id="GDScript_2sg1u"] | ||
| resource_name = "postprocess" |
There was a problem hiding this comment.
In the new code, sound effects are added when the mouse enters or clicks on an item. Make sure these sound effects do not interfere with other sounds in the game and that they enhance the user experience.
| extends GutTest | ||
|
|
||
| var file_stream: FileStream | ||
| var write_error_occurred: bool = false | ||
|
|
||
| func before_each(): | ||
| # Create a new instance of FileStream and set the test file path | ||
| file_stream = FileStream.new() | ||
| file_stream.file_path = "user://test_1mb.dat" | ||
| add_child(file_stream) | ||
| # Delete the test file if it exists | ||
| file_stream.delete() | ||
| # Connect to write_error signal | ||
| file_stream.connect("write_error", _on_write_error) | ||
| write_error_occurred = false | ||
|
|
||
| func _on_write_error(message): | ||
| write_error_occurred = true | ||
| push_error("Write error: %s" % message) | ||
|
|
||
| func after_each(): | ||
| file_stream.finish() | ||
| file_stream.delete() | ||
| file_stream.free() | ||
| file_stream = null | ||
|
|
||
| func test_read_write_1mb_of_data(): | ||
| # Step 1: Generate 1 MB of data | ||
| var data_size = 1024 * 1024 # 1 MB | ||
| var chunk_size = 1024 # 1 KB per chunk | ||
| var num_chunks = data_size / chunk_size | ||
|
|
||
| # Create an array to hold the data to write | ||
| var data_to_write: Array = [] | ||
| var sample_data = String("a").repeat(chunk_size) # 1 KB of 'a's | ||
|
|
||
| for i in range(num_chunks): | ||
| data_to_write.append(sample_data) | ||
|
|
||
| # Step 2: Write data to the file stream | ||
| for data in data_to_write: | ||
| file_stream.write(data) | ||
|
|
||
| # Step 3: Wait until all data has been written | ||
| await file_stream.wait_until_write_queue_empty() | ||
|
|
||
| # Check for write errors | ||
| if write_error_occurred: | ||
| push_error("Write error occurred during the test.") | ||
| return | ||
|
|
||
| # Step 4: Read data back from the file | ||
| var read_data: Array = [] | ||
| file_stream.read(func(chunks): | ||
| read_data.append_array(chunks) | ||
| ) | ||
|
|
||
| await file_stream.wait_until_read_queue_empty() | ||
|
|
||
| # Step 5: Verify that the data read matches the data written | ||
| assert_eq(read_data.size(), data_to_write.size(), "Mismatch in number of chunks read.") | ||
|
|
||
| # Optionally, you can verify the content of each chunk | ||
| for i in range(read_data.size()): | ||
| assert_eq(read_data[i], data_to_write[i], "Data mismatch at chunk %d" % i) | ||
|
|
There was a problem hiding this comment.
The overall structure of the test seems fine, but there are a few areas that could be improved for better error handling and maintainability.
Firstly, in line 9, you're hardcoding the file path. This might lead to issues if multiple tests run concurrently or if you want to reuse this code for different files. Consider passing the file name as an argument to the before_each function.
Secondly, in lines 12 and 23, you're deleting the file without checking if it exists first. This could potentially raise an error. It's better to check if the file exists before trying to delete it.
Lastly, in lines 41-42 and 54-56, you're writing and reading data from the file stream without checking if the operations were successful. You should add error checks after these operations to ensure they completed successfully.
Here are the suggested changes:
extends GutTest
var file_stream: FileStream
var write_error_occurred: bool = false
func before_each(file_name="test_1mb.dat"):
# Create a new instance of FileStream and set the test file path
file_stream = FileStream.new()
-9: file_stream.file_path = "user://test_1mb.dat"
+9: file_stream.file_path = "user://" + file_name
add_child(file_stream)
# Delete the test file if it exists
-12: file_stream.delete()
+12: if file_stream.exists():
+13: file_stream.delete()
# Connect to write_error signal
file_stream.connect("write_error", _on_write_error)
write_error_occurred = false
func _on_write_error(message):
write_error_occurred = true
push_error("Write error: %s" % message)
func after_each():
file_stream.finish()
-24: file_stream.delete()
+24: if file_stream.exists():
+25: file_stream.delete()
file_stream.free()
file_stream = null
func test_read_write_1mb_of_data():
# Step 1: Generate 1 MB of data
var data_size = 1024 * 1024 # 1 MB
var chunk_size = 1024 # 1 KB per chunk
var num_chunks = data_size / chunk_size
# Create an array to hold the data to write
var data_to_write: Array = []
var sample_data = String("a").repeat(chunk_size) # 1 KB of 'a's
for i in range(num_chunks):
data_to_write.append(sample_data)
# Step 2: Write data to the file stream
for data in data_to_write:
-44: file_stream.write(data)
+44: if not file_stream.write(data):
+45: push_error("Failed to write data to file.")
+46: return
# Step 3: Wait until all data has been written
await file_stream.wait_until_write_queue_empty()
# Check for write errors
if write_error_occurred:
push_error("Write error occurred during the test.")
return
# Step 4: Read data back from the file
var read_data: Array = []
file_stream.read(func(chunks):
-59: read_data.append_array(chunks)
+59: if not read_data.append_array(chunks):
+60: push_error("Failed to read data from file.")
+61: return
)
await file_stream.wait_until_read_queue_empty()
# Step 5: Verify that the data read matches the data written
assert_eq(read_data.size(), data_to_write.size(), "Mismatch in number of chunks read.")
# Optionally, you can verify the content of each chunk
for i in range(read_data.size()):
assert_eq(read_data[i], data_to_write[i], "Data mismatch at chunk %d" % i)
Please note that the line numbers have changed due to the added lines.
| class_name Replayer extends Node | ||
|
|
||
| @export var target_node: NodePath # The node whose properties we want to capture | ||
| @export var properties_to_record: Array = ["position"] # The properties to record (can be expanded) | ||
| @export var enable_hotkeys: bool = false | ||
|
|
||
| var file_stream: FileStream | ||
| var is_recording: bool = false | ||
| var is_replaying: bool = false | ||
| var start_time: int = 0 # To track the start of recording for relative timestamps | ||
|
|
||
| # Called when the node enters the scene tree | ||
| func _ready() -> void: | ||
| # Create the FileStream node to handle data | ||
| file_stream = FileStream.new() | ||
| file_stream.file_path = "user://replay.dat" | ||
| add_child(file_stream) | ||
|
|
||
| func _input(event: InputEvent) -> void: | ||
| if !enable_hotkeys: return | ||
|
|
||
| if event.is_action_just_pressed("record"): | ||
| if is_recording: | ||
| stop_recording() | ||
| else: | ||
| start_recording() | ||
| if event.is_action_just_pressed("replay"): | ||
| if is_replaying: | ||
| stop_replay() | ||
| else: | ||
| start_replay() | ||
|
|
||
| # Start recording properties (every frame via _process) | ||
| func start_recording() -> void: | ||
| is_recording = true | ||
| file_stream.delete() # Clear previous recordings | ||
| start_time = Time.get_ticks_msec() # Set the recording start time | ||
|
|
||
| # Stop recording properties | ||
| func stop_recording() -> void: | ||
| is_recording = false | ||
|
|
||
| # Capture the target node's properties every frame | ||
| func _process(delta: float) -> void: | ||
| if is_recording and has_node(target_node): | ||
| var target = get_node(target_node) | ||
| var data_to_record = {} | ||
|
|
||
| # Store the selected properties | ||
| for prop in properties_to_record: | ||
| if target.has_method("get_" + prop): | ||
| data_to_record[prop] = target.get(prop) | ||
| elif target.has_property(prop): | ||
| data_to_record[prop] = target.get(prop) | ||
|
|
||
| # Store the relative timestamp to replay data in sequence | ||
| data_to_record["timestamp"] = Time.get_ticks_msec() - start_time | ||
|
|
||
| # Write the captured properties to the FileStream every frame | ||
| file_stream.write_chunk([data_to_record]) | ||
|
|
||
| # Start replaying recorded properties | ||
| func start_replay() -> void: | ||
| is_replaying = true | ||
| file_stream.read_chunk(_apply_chunk) | ||
|
|
||
| # Callback to apply a chunk of recorded properties | ||
| func _apply_chunk(chunk: Array) -> void: | ||
| if has_node(target_node): | ||
| var target = get_node(target_node) | ||
|
|
||
| # Apply properties in the chunk to the target node | ||
| for data in chunk: | ||
| var timestamp = data.get("timestamp", 0) | ||
| for prop in properties_to_record: | ||
| if data.has(prop): | ||
| target.set(prop, data[prop]) | ||
|
|
||
| # Simulate the delay between property captures based on timestamps | ||
| if chunk.size() > 0: | ||
| var delay = chunk[0]["timestamp"] - Time.get_ticks_msec() + start_time | ||
| delay = max(delay, 0) | ||
| # Schedule the next chunk application after the appropriate delay | ||
| get_tree().create_timer(delay / 1000.0).connect("timeout", Callable(self, "start_replay")) | ||
|
|
||
| # Stop replaying recorded properties | ||
| func stop_replay() -> void: | ||
| is_replaying = false |
There was a problem hiding this comment.
The Replayer class seems to be well implemented. However, there are a few areas that could be improved for better error handling and maintainability.
-
The
target_nodeis a NodePath, but there's no check to ensure that the node at this path exists or is of the expected type. This could lead to runtime errors if the path is incorrect or the node doesn't have the expected properties or methods. Consider adding some validation in the_readymethod or before usingget_node(target_node). -
In the
_processmethod, you're checking if the target node has a getter method or property for each property to record. If it doesn't, you simply skip it. It might be helpful to log a warning message when this happens, as it could indicate a configuration error that would be silent otherwise. -
The
file_streamis created and added as a child in the_readymethod, but it's never removed from the tree or freed. This could potentially lead to memory leaks if instances ofReplayerare created and destroyed frequently. Consider removing and freeing thefile_streamin the_exit_treemethod. -
The
start_replaymethod reads a chunk from the file stream and applies it immediately, then schedules the next chunk application based on the timestamp in the chunk. This assumes that the chunks in the file stream are ordered by their timestamps, which might not always be the case. If they're not ordered, the replay could appear "jumpy" or inconsistent. Consider sorting the chunks by their timestamps before starting the replay. -
The
stop_recordingandstop_replaymethods simply setis_recordingandis_replayingto false, but they don't do anything to clean up any ongoing recording or replaying operations. For example, ifstop_replayis called while a chunk application is scheduled, the application will still happen. Consider adding some cleanup logic to these methods.
Here are some suggested changes:
class_name Replayer extends Node
@export var target_node: NodePath # The node whose properties we want to capture
@export var properties_to_record: Array = ["position"] # The properties to record (can be expanded)
@export var enable_hotkeys: bool = false
var file_stream: FileStream
var is_recording: bool = false
var is_replaying: bool = false
var start_time: int = 0 # To track the start of recording for relative timestamps
# Called when the node enters the scene tree
func _ready() -> void:
# Create the FileStream node to handle data
file_stream = FileStream.new()
file_stream.file_path = "user://replay.dat"
add_child(file_stream)
+ if not has_node(target_node):
+ printerr("Target node does not exist: " + str(target_node))
func _input(event: InputEvent) -> void:
if !enable_hotkeys: return
if event.is_action_just_pressed("record"):
if is_recording:
stop_recording()
else:
start_recording()
if event.is_action_just_pressed("replay"):
if is_replaying:
stop_replay()
else:
start_replay()
# Start recording properties (every frame via _process)
func start_recording() -> void:
is_recording = true
file_stream.delete() # Clear previous recordings
start_time = Time.get_ticks_msec() # Set the recording start time
# Stop recording properties
func stop_recording() -> void:
is_recording = false
# Capture the target node's properties every frame
func _process(delta: float) -> void:
if is_recording and has_node(target_node):
var target = get_node(target_node)
var data_to_record = {}
# Store the selected properties
for prop in properties_to_record:
if target.has_method("get_" + prop):
data_to_record[prop] = target.get(prop)
elif target.has_property(prop):
data_to_record[prop] = target.get(prop)
+ else:
+ printerr("Target does not have property or getter: " + prop)
# Store the relative timestamp to replay data in sequence
data_to_record["timestamp"] = Time.get_ticks_msec() - start_time
# Write the captured properties to the FileStream every frame
file_stream.write_chunk([data_to_record])
# Start replaying recorded properties
func start_replay() -> void:
is_replaying = true
file_stream.read_chunk(_apply_chunk)
# Callback to apply a chunk of recorded properties
func _apply_chunk(chunk: Array) -> void:
if has_node(target_node):
var target = get_node(target_node)
# Apply properties in the chunk to the target node
for data in chunk:
var timestamp = data.get("timestamp", 0)
for prop in properties_to_record:
if data.has(prop):
target.set(prop, data[prop])
# Simulate the delay between property captures based on timestamps
if chunk.size() > 0:
var delay = chunk[0]["timestamp"] - Time.get_ticks_msec() + start_time
delay = max(delay, 0)
# Schedule the next chunk application after the appropriate delay
get_tree().create_timer(delay / 1000.0).connect("timeout", Callable(self, "start_replay"))
# Stop replaying recorded properties
func stop_replay() -> void:
is_replaying = false
Pull Request Description: Multiplayer Preparation
This pull request introduces significant changes in preparation for implementing multiplayer functionality within the project. The main motivation behind these changes is to lay the groundwork for a robust multiplayer system that will enhance user experience and expand gameplay possibilities.
Key Changes:
Removal of Unused Components:
Refactoring for Multiplayer Compatibility:
License Information:
Why This Improves the Project:
Foundation for Multiplayer: By removing outdated and unnecessary components, we create a clean slate that focuses on the multiplayer architecture. This will facilitate easier development and integration of multiplayer features.
Enhanced Performance: Streamlining the codebase helps improve performance and maintainability, allowing developers to focus on new features rather than legacy code.
Future-Proofing: With a clearer focus on multiplayer capabilities, the project is better positioned for future enhancements and expansions, ultimately leading to a richer user experience.
This pull request is a crucial step toward achieving a fully functional multiplayer environment, setting the stage for further developments in this area.
Summary by CodeRabbit
Guttesting framework, removing GUI controls, event handling, and test execution functionality.get_first_parent_of_typeinNodot.gdfor node hierarchy navigation.SaveManager.configfor consistency acrossAudioManager.gd,InputManager.gd,CollectableManager.gd,DebugManager.gd,NetworkManager.gd, andVideoManager.gd.Fire3D.gdandSmoke3D.gd.FileStreamclass inFileStream.gdfor asynchronous file operations.Interaction3D.gdand damage calculation inRigidBreakable3D.gd.LICENSE.mdandOFL.txt.