godot-code-review

Godot 代码审查

适用于 Godot 4.3+ 项目的结构化审查指南，涵盖 GDScript 和 C#。请逐项检查各清单，然后使用末尾的输出模板生成审查摘要。

相关技能： godot-testing 用于 TDD 和测试覆盖率，scene-organization 用于场景树最佳实践，godot-optimization 用于性能审查。

---

1. 节点与场景架构

• 每个场景都有单一、明确的职责（玩家、敌人、UI 控件等）
• 继承链保持浅层——优先通过子节点组合，而非深层 extends 层级
• 谨慎使用 Autoload（单例）；只有真正的全局状态才应放在其中
• 节点引用只遍历到直接子节点——不要使用 get_parent() 链
• @onready（GDScript）或 GetNode<T>()（C#）的目标是直接子节点，或同一场景内的命名路径

反模式 — get_parent() 链

# BAD: tight coupling, breaks if the tree changes
func take_damage(amount: int) -> void:
    get_parent().get_parent().get_node("HUD").update_health(health)

// BAD: tight coupling, breaks if the tree changes
public void TakeDamage(int amount)
{
    GetParent().GetParent().GetNode("HUD").Call("UpdateHealth", _health);
}

修复 — 改为发出信号

# GOOD: parent/ancestor listens; child stays decoupled
signal health_changed(new_health: int)

func take_damage(amount: int) -> void:
    health -= amount
    health_changed.emit(health)

// GOOD: parent/ancestor listens; child stays decoupled
[Signal]
public delegate void HealthChangedEventHandler(int newHealth);

public void TakeDamage(int amount)
{
    _health -= amount;
    EmitSignal(SignalName.HealthChanged, _health);
}

---

2. GDScript 风格

• 变量和函数使用 snake_case
• 使用 class_name 声明的类名采用 PascalCase
• 常量使用 SCREAMING_SNAKE_CASE
• 所有函数参数和返回值都带有类型提示
• @export 变量包含显式类型
• 信号声明位于文件顶部，在变量之前

不佳 — 未标注类型

var speed = 200
var health = 100

func move(direction):
    position += direction * speed

func heal(amount):
    health += amount
    return health

// BAD: no explicit types, weak contracts
float speed = 200;
int health = 100;

public void Move(object direction)
{
    Position += (Vector2)direction * speed;
}

public object Heal(object amount)
{
    health += (int)amount;
    return health;
}

良好 — 已标注类型

class_name PlayerController
extends CharacterBody2D

signal health_changed(new_health: int)
signal player_died()

const MAX_HEALTH: int = 100
const BASE_SPEED: float = 200.0

@export var speed: float = BASE_SPEED
@export var max_health: int = MAX_HEALTH

var health: int = max_health

func move(direction: Vector2) -> void:
    velocity = direction * speed
    move_and_slide()

func heal(amount: int) -> int:
    health = mini(health + amount, max_health)
    health_changed.emit(health)
    return health

// GOOD: strongly typed, proper C# conventions
public partial class PlayerController : CharacterBody2D
{
    [Signal]
    public delegate void HealthChangedEventHandler(int newHealth);
    [Signal]
    public delegate void PlayerDiedEventHandler();

    private const int MaxHealth = 100;
    private const float BaseSpeed = 200f;

    [Export] public float Speed { get; set; } = BaseSpeed;
    [Export] public int MaxHp { get; set; } = MaxHealth;

    private int _health;

    public override void _Ready()
    {
        _health = MaxHp;
    }

    public void Move(Vector2 direction)
    {
        Velocity = direction * Speed;
        MoveAndSlide();
    }

    public int Heal(int amount)
    {
        _health = Mathf.Min(_health + amount, MaxHp);
        EmitSignal(SignalName.HealthChanged, _health);
        return _health;
    }
}

---

3. C# 风格

• 节点脚本使用 partial class，以便 Godot 源代码生成器正常工作
• 方法和属性使用 PascalCase；局部变量使用 camelCase
• [Export] 属性使用 PascalCase
• [Signal] 委托遵循 <EventName>EventHandler 命名模式
• 对 GetNode<T>() 的结果进行空值检查，或在 _Ready() 中缓存并验证

// GOOD
public partial class PlayerController : CharacterBody2D
{
    [Signal]
    public delegate void HealthChangedEventHandler(int newHealth);

    [Export] public float Speed { get; set; } = 200f;
    [Export] public int MaxHealth { get; set; } = 100;

    private int _health;
    private AnimationPlayer _animationPlayer = null!;

    public override void _Ready()
    {
        _animationPlayer = GetNode<AnimationPlayer>("AnimationPlayer");
        // Validate at startup rather than silently failing later
        if (_animationPlayer is null)
            GD.PushError("AnimationPlayer node not found on PlayerController");

        _health = MaxHealth;
    }

    public void TakeDamage(int amount)
    {
        _health = Mathf.Max(_health - amount, 0);
        EmitSignal(SignalName.HealthChanged, _health);
    }
}

---

4. 性能

• 绝不在 _process() 或 _physics_process() 中调用 get_node() / $NodePath——始终使用 @onready 缓存
• 不在热路径中调用 load()——使用 preload() 在编译时加载，或缓存结果
• 节点不需要逐帧更新时，应禁用 _process()（set_process(false)）
• 在 _process() 或紧密循环中进行比较时，使用 StringName（或 &"string" 字面量）

反模式 — 在 _process() 中未缓存节点查找结果

# BAD: get_node() traverses the tree every frame
func _process(delta: float) -> void:
    get_node("HUD/HealthBar").value = health
    get_node("HUD/Label").text = str(health)

// BAD: GetNode() traverses the tree every frame
public override void _Process(double delta)
{
    GetNode<ProgressBar>("HUD/HealthBar").Value = _health;
    GetNode<Label>("HUD/Label").Text = _health.ToString();
}

修复 — 使用 @onready 缓存

# GOOD: resolved once at scene load
@onready var _health_bar: ProgressBar = $HUD/HealthBar
@onready var _health_label: Label = $HUD/Label

func _process(delta: float) -> void:
    _health_bar.value = health
    _health_label.text = str(health)

// GOOD: resolved once in _Ready()
private ProgressBar _healthBar = null!;
private Label _healthLabel = null!;

public override void _Ready()
{
    _healthBar = GetNode<ProgressBar>("HUD/HealthBar");
    _healthLabel = GetNode<Label>("HUD/Label");
}

public override void _Process(double delta)
{
    _healthBar.Value = _health;
    _healthLabel.Text = _health.ToString();
}

在热路径中使用 StringName

# BAD: a String compared to a StringName is compared character by character
if animation_name == "run":
    pass

# GOOD: StringName literal — a pointer comparison
if animation_name == &"run":
    pass

// BAD: allocates a new StringName each frame
if (animationName == "run") { }

// GOOD: cache StringName as a static field
private static readonly StringName RunAnim = new("run");

public override void _Process(double delta)
{
    if (animationName == RunAnim) { }
}

---

5. 输入处理

• 所有操作都使用 Input Map 名称（Project > Project Settings > Input Map），而非硬编码的按键常量
• 优先使用 _unhandled_input() 而非 _input()，以便 UI 控件先处理事件
• 连续移动在 _physics_process() 中通过 Input.get_vector() / Input.is_action_pressed() 驱动
• 离散的一次性操作（跳跃、射击）在 _unhandled_input() 中处理

# Continuous movement — physics process
func _physics_process(delta: float) -> void:
    var direction: Vector2 = Input.get_vector(
        &"ui_left", &"ui_right", &"ui_up", &"ui_down"
    )
    velocity = direction * speed
    move_and_slide()

# Discrete action — unhandled input
func _unhandled_input(event: InputEvent) -> void:
    if event.is_action_pressed(&"jump"):
        _jump()

// Continuous movement — physics process
public override void _PhysicsProcess(double delta)
{
    Vector2 direction = Input.GetVector(
        "ui_left", "ui_right", "ui_up", "ui_down"
    );
    Velocity = direction * Speed;
    MoveAndSlide();
}

// Discrete action — unhandled input
public override void _UnhandledInput(InputEvent @event)
{
    if (@event.IsActionPressed("jump"))
    {
        Jump();
    }
}

---

6. 信号与通信

• 信号沿树向上流动（子节点发出，父节点/祖先节点连接）；方法调用向下流动（父节点调用子节点方法）
• 连接在 _ready() 中建立，或在编辑器中配置——不要在 _process() 或仅调用一次的回调中建立
• 节点之间不存在循环信号依赖
• 信号名称使用过去时态描述已发生的事情

# Good signal names
signal health_changed(new_health: int)   # past tense
signal enemy_died()                       # past tense
signal item_collected(item: ItemData)     # past tense

# Bad signal names (present/imperative tense)
# signal update_health(value: int)
# signal die()
# signal collect_item(item: ItemData)

// Good signal names — past tense, EventHandler suffix
[Signal]
public delegate void HealthChangedEventHandler(int newHealth);
[Signal]
public delegate void EnemyDiedEventHandler();
[Signal]
public delegate void ItemCollectedEventHandler(ItemData item);

// Bad signal names (present/imperative tense)
// public delegate void UpdateHealthEventHandler(int value);
// public delegate void DieEventHandler();
// public delegate void CollectItemEventHandler(ItemData item);

# Parent connects to child signal in _ready()
func _ready() -> void:
    $Enemy.enemy_died.connect(_on_enemy_died)
    $Player.health_changed.connect(_on_player_health_changed)

// Parent connects to child signal in _Ready()
public override void _Ready()
{
    GetNode<Enemy>("Enemy").EnemyDied += OnEnemyDied;
    GetNode<Player>("Player").HealthChanged += OnPlayerHealthChanged;
}

---

7. 资源管理

• 对编辑时已知的资源（场景、纹理、音频）使用 preload()；对运行时解析的路径使用 load()
• 运行时加载大型或特定关卡的资源时，使用 ResourceLoader.load_threaded_request() 以避免帧卡顿
• 动态实例化的节点使用 queue_free() 而非 free() 释放，以避免释放后使用导致崩溃

# Compile-time — path is validated by the editor
const BULLET_SCENE: PackedScene = preload("res://scenes/bullet.tscn")

# Runtime — path comes from data
func _load_level(path: String) -> void:
    ResourceLoader.load_threaded_request(path)

func _check_load(path: String) -> void:
    if ResourceLoader.load_threaded_get_status(path) == ResourceLoader.THREAD_LOAD_LOADED:
        var scene: PackedScene = ResourceLoader.load_threaded_get(path)
        get_tree().change_scene_to_packed(scene)

# Cleanup
func _on_enemy_died() -> void:
    queue_free()   # safe — deferred until end of frame

// Compile-time equivalent — load once in a static field or _Ready()
private static readonly PackedScene BulletScene =
    GD.Load<PackedScene>("res://scenes/bullet.tscn");

// Runtime — path comes from data
private void LoadLevel(string path)
{
    ResourceLoader.LoadThreadedRequest(path);
}

private void CheckLoad(string path)
{
    if (ResourceLoader.LoadThreadedGetStatus(path) == ResourceLoader.ThreadLoadStatus.Loaded)
    {
        var scene = ResourceLoader.LoadThreadedGet(path) as PackedScene;
        GetTree().ChangeSceneToPacked(scene);
    }
}

// Cleanup
private void OnEnemyDied()
{
    QueueFree(); // safe — deferred until end of frame
}

---

8. 容易出错的模式

模式	问题	修复方法
await get_tree().create_timer(t).timeout 在 queue_free() 之后（GDScript）	已释放节点的协程会被静默丢弃：没有错误，并且 await 之后的代码不会运行，因此其中的 is_instance_valid(self) 永远不会执行	在 queue_free() 之前完成必须执行的工作；在 await 之后用 is_instance_valid() 重新检查其他节点；对于应随节点一起销毁的效果，请使用 create_tween()
脆弱的节点路径，例如 $A/B/C/D/E	场景树重新组织后会在运行时失败（Node not found）	重构为直接子节点并使用信号，或导出一个 NodePath
到处使用 call_deferred()	延迟调用适用于跨帧安全，不是通用解决方案；过度使用会掩盖真正的设计问题	仅在跨越物理／主线程边界或打破调用循环时使用延迟调用
在 _physics_process() 中调用 set_physics_process(true)	每帧都会冗余调用，浪费 CPU	在实际需要启用或禁用处理的时刻调用一次
直接设置 CharacterBody2D 的 position	绕过碰撞检测；会瞬移刚体，并可能导致穿透	使用 velocity 调用 move_and_slide()；仅在有意瞬移时设置 position／global_position

---

9. 审查输出格式

交付审查时，请使用此模板：

## Code Review — <FileName or Feature>

### Critical
Issues that will cause bugs, crashes, or significant performance problems.

- [ ] <node/line> — <issue> — **Suggested fix:** <fix>

### Improvements
Code quality, style, or maintainability concerns that should be addressed.

- [ ] <node/line> — <issue> — **Suggested fix:** <fix>

### Positive
What the code does well — reinforce good patterns.

- <observation>

---
Reviewed against: Godot 4.3+ best practices

示例

## Code Review — PlayerController.gd

### Critical
- [ ] _process() line 42 — `get_node("HUD/HealthBar")` called every frame — **Suggested fix:** Cache with `@onready var _health_bar: ProgressBar = $HUD/HealthBar`
- [ ] take_damage() line 67 — no type hints on parameter or return — **Suggested fix:** `func take_damage(amount: int) -> void:`

### Improvements
- [ ] Line 12 — signal `updateHealth` should be past tense — **Suggested fix:** Rename to `health_changed`
- [ ] Line 8 — `var speed = 200` missing type hint — **Suggested fix:** `var speed: float = 200.0`

### Positive
- Signals are declared at the top of the file
- Constants correctly use SCREAMING_SNAKE_CASE
- `queue_free()` used correctly for cleanup

---
Reviewed against: Godot 4.3+ best practices