coding conventions when creating a complex system, I have created a modular weapon system in godot

Question

I have created a modular weapon system in godot 4 but I am sure I have not made this system correctly, so I was looking for feedback on how to do it the correct way or Improve it, I know this question might seem broad to answer but simply put I think I am not doing it correctly for these reasons.

1: My weapon data and input are combined, Im sure this is not good but I dont know how to do it another way, the main reason I did it this way was because each weapon class (gun, melee etc) would require different code for their inputs.

2: The weapon resources are calling the main weapon system when requiring anything to do with the scene tree for example, adding children or instancing scenes, this feels wrong to me.

3: I dont think its actually that modular, since the weapons script will keep calling for specific nodes outside of weapon system script it could cause headaches in the future for example, say I have a power up item that gives me ammo but if I am holding a knife it would fail.

My goal with this project is to make a tutorial to share with people but I need feedback on it from people who have plenty of experience, I have put a minimum project up on github so everyone can try it out and see it more clearly(I will explain it here as well.) LINK:https://github.com/Dragon20C/Modular-Weapon-System/tree/main/Modular%20Weapon%20System.

I have put everything related to the weapon system into a folder and its simply a node thats added to the player scene.

class_name Weapon_System
extends Node

@export var action_timer : Timer
@export var raycaster : RayCast3D

@export var hand_node : Node3D
@export var weapon_inventory : Array[Handheld_Weapon]
var inventory_index = 0
var current_weapon : Handheld_Weapon

# This script will hold the weapons and do the switching of weapons and drop and pick ups as well.

func _ready():
    # Switch to the first weapon in the inventory
    switch_to_weapon(weapon_inventory[inventory_index])


func _input(event):
    if event.is_action_pressed("Scroll_up"):
        inventory_index = min(inventory_index+1,weapon_inventory.size()-1)
        switch_to_weapon(weapon_inventory[inventory_index])
        
    elif event.is_action_pressed("Scroll_down"):
        inventory_index = max(inventory_index-1,0)
        switch_to_weapon(weapon_inventory[inventory_index])
        
func switch_to_weapon(weapon):
    # Check if we are not switching to the same weapon
    if current_weapon == weapon: return
    # Check for the start of the scene, and play exit function
    if current_weapon != null:
        current_weapon.on_exit()
        await current_weapon.animator.animation_finished
        # Delete current weapon model when animation is finished.
        hand_node.get_child(0).queue_free()
    # set our selfs to the weapon so it can access raycasts etc
    weapon.weapon_system = self
    # set current weapon to weapon
    current_weapon = weapon
    # instance the model which should also have animations to it.
    var instance = current_weapon.weapon_scene.instantiate()
    hand_node.add_child(instance)
    current_weapon.animator = instance.get_node("Animations")
    # Play on enter of current Weapon
    current_weapon.on_enter()
    # Wait for animation to finish
    await current_weapon.animator.animation_finished

func _physics_process(_delta) -> void:
    # to avoid errors we wait until its not null.
    if current_weapon != null:
        # We play every unqiue action the weapon holds.
        current_weapon.actions()

This script is the main weapon system script that handles the switching and playing the inputs which I named actions.

class_name Handheld_Weapon
extends Resource

@export_group("Default")
@export var weapon_name : String
@export var weapon_scene : PackedScene
var animator : AnimationPlayer
@export var min_damage : int
@export var max_damage : int
@export_range(0,100) var waight_modifier : float

# Use this to get raycasts or other nodes that this resource could use!
var weapon_system : Weapon_System

func actions():
    # Create inputs for each weapon.
    pass

func on_enter():
    # Play enter animations and maybe also set any variables
    pass

func on_exit():
    # Play exit animations and clear up anything left
    pass

This script is the base class of all the weapons so everything else gun, melee etc inherits from this script the actions function is where I do input checks and this is where I think I should not have the inputs and data in the same place but I am not sure how to separate it without it causing me a headache.

class_name Handheld_Gun
extends Handheld_Weapon


@export_group("Gun")
@export_flags ("Automatic", "Semi-automatic") var action_type = 0
@export var ammo_capacity : int
@export var magazine_max : int
@export var current_magazine : int
@export var rpm : int
var rate_of_fire : float
@export var reload_speed : float
@export var bullet_decal : PackedScene

# Create inputs for each weapon.
func actions():
    if action_type == 1:
        if Input.is_action_pressed("Left_Click") and weapon_system.action_timer.is_stopped():
            weapon_system.action_timer.start()
            shoot()
    elif action_type == 2:
        if Input.is_action_just_pressed("Left_Click"):
            shoot()
        
    if Input.is_action_just_pressed("R_key"):
        current_magazine = magazine_max
        print("Reloading!")

func shoot():
    if current_magazine > 0:
        current_magazine -= 1
        print("you have " + str(current_magazine) + " rounds left")
        check_raycast()
    else:
        print("Out of ammo!")
        
    animator.seek(0)
    animator.play("Shoot")
    print("Bang!")

func check_raycast():
    var raycast = weapon_system.raycaster
    if raycast.is_colliding():
        var point = raycast.get_collision_point()
        var normal = raycast.get_collision_normal()
        spawn_decal(point,normal)

func spawn_decal(position: Vector3, normal: Vector3):
    var decal = bullet_decal.instantiate()
    weapon_system.get_tree().get_root().add_child(decal)
    decal.global_transform.origin = position
    
    # Check if normal is not down or up then we do look at
    if normal != Vector3.UP and normal != Vector3.DOWN:
        decal.look_at(position + normal,Vector3.UP)
        decal.transform = decal.transform.rotated_local(Vector3.RIGHT, PI/2.0)
    # Else we check if its up and we do a 180 to get it to rotate correctly!
    elif normal == Vector3.UP:
        decal.transform = decal.transform.rotated_local(Vector3.RIGHT, PI)
        
    decal.rotate(normal, randf_range(0, 2*PI))
    
    weapon_system.get_tree().create_timer(5).timeout.connect(func destory_decal(): decal.queue_free())

func on_enter():
    # Play enter animations and maybe also set any variables
    rate_of_fire = 1.0 / (rpm / 60.0)
    weapon_system.action_timer.wait_time = rate_of_fire
    animator.play("BringIn")
    print("Bring " + weapon_name + " in!")

func on_exit():
    # Play exit animations and clear up anything left
    print("Putting " + weapon_name + " away...")
    animator.play("PutAway")

This is how my gun class looks like it looks messy specially since the data and input are together and feels hard to expand on it, Thank you for taking your time on looking at my post and project.

Answer 1

I'll try to answer in general terms, so this is useful for more people.

---

1: My weapon data and input are combined

Your Resources are wrapping scenes. Let the instance of the scene handle its input.

I'm aware that that sounds like combining appearance with input, but...

If you have imported a model (for the weapon in this case) it comes with the meshes, materials and animations. You don't want to modify that, in case you need to import a newer version.

Instead make a new scene where you instantiate the one you imported, and add code, physics colliders, and perhaps other stuff. This way, you can swap the imported scene (which has the appearance) and keep the code.

You could have more than two layers. For example: The first one is the imported model. The second one adds physics colliders and bone attachments. And the third one adds the code.

So you have an that has its own code, and you can swap the its appearance if you need to. So, you can instantiate the logic scene, and let it handle input. Its code would use its own _physics_process where it uses Input (or its own _input) which Godot will call, so you don't need to write code to call into it.

Plus, you can still have a base class for the script of the scenes.

---

2: The weapon resources are calling the main weapon system when requiring anything to do with the scene

First of all, if the scene you have has code, it can deal with any internal stuff (e.g. playing an animation on itself). It would also be OK for it do its own ray casting, assuming it is parented appropriately. Similarly it could spawn projectiles or decals if necessary.

About communicating outside of that... There are a few solutions:

• You can inject a dependency. That is an object that the outer code sets so the inner code can call methods on it. This allows the inner code to control the outer code (which is known as inversion of control).
• You can use signals. You can have the inner code emit signals, and the outer code connect to those signals and react to them.
• Another alternative is to have the inner code return (when the outer code calls into it) custom objects that tell the outer code what to do. You can think of them as command objects.

I'd suggest signals. Right now you are injecting the AnimationPlayer and the Weapon_System, which I'm guessing you won't need anymore.

---

I suspect that once you separate code between the Resource and the scene, the Resource would have no need to communicate outwards. I expect it to be just properties. So what I said above goes for the code in the scene.

It is probably OK to inject the Resource into the scene (if it needs to read its information), and use signals for anything else.

---

3: (...) say I have a power up item that gives me ammo but if I am holding a knife it would fail.

You could have the weapon interrogate the power up. For example, with duck typing:

if "ammo" in obj:
    current_ammo += obj.get("ammo")

*You might also find the methods has_method and call useful.

Alternatively, if you rather, you could check for specific types. For example:

var ammo_power_up := obj as AmmoPowerUp
if is_instance_valid(obj):
    current_ammo += ammo_power_up.ammo

Or using is and as:

if obj is AmmoPowerUp:
    current_ammo += (obj as AmmoPowerUp).ammo

Having the weapon interrogate the power up, would let the weapon decide if you consume it ("pick it up") at all. Of course, having the power up interrogate the weapon is also viable.

---

I know you don't ask about damage, so I won't dwell on this... But I want to point out that having physical damage objects that exist in the world and detecting them via physics can be useful to reduce code duplication (multiple weapons can emit the same damage object, and multiple targets can react to same damage object differently). Allow the damage to do its own decals and particles. Plus unlocks having enemy agents react to them via Area(2D/3D) "view cone" or "hearing"... And I remind you that not all damage has to come from weapons.