Code review on my physics pickup Module

Hey there! Was thinking about releasing my Physics Pickup script for open source, although i’m not fully confident in the code, here is my module, if someone could go through it and tell me what to improve on, that would be appreciated!

local part = Instance.new("Part")
part.Anchored = true
part.CanCollide = false
local alignpostion = Instance.new("AlignPosition")
local Orientationalign = Instance.new("AlignOrientation")
alignpostion.Responsiveness = 50
local attachment = Instance.new("Attachment")
local player = game:GetService("Players").LocalPlayer
local runservice = game:GetService("RunService")
local grab = nil
local attachmentpart
local PhysicsPickUp = {}
local PhysicsService = game:GetService("PhysicsService")

for i, v in ipairs(player.Character:GetDescendants()) do
    if v:IsA("BasePart") then
        v.CollisionGroup = "player"
    end
end

function PhysicsPickUp.PartToCamera()
    part.Parent = player.Character:WaitForChild("Head")
    alignpostion.Parent = part
    Orientationalign.Parent = part
    Orientationalign.Mode = Enum.OrientationAlignmentMode.OneAttachment
    attachment.Parent = part
    alignpostion.Attachment1 = attachment
    part.Position = game.Workspace.CurrentCamera.CFrame.LookVector * 4 + player.Character:WaitForChild("Head").Position
    part.Orientation = part.Orientation
end

function PhysicsPickUp.CheckForObject()
    local rayOrigin = player.Character:WaitForChild("Head").Position
    local rayDirection = game.Workspace.CurrentCamera.CFrame.LookVector * 10
    local raycastparams = RaycastParams.new()
    raycastparams.FilterDescendantsInstances = {player.Character}
    raycastparams.FilterType = Enum.RaycastFilterType.Blacklist
    local raycastResult = workspace:Raycast(rayOrigin, rayDirection,raycastparams)
    
    if raycastResult then
        return raycastResult.Instance
    else
        return nil
    end
end

function PhysicsPickUp.CheckModel(Object)
    if Object:FindFirstAncestorOfClass("Model") then
        return Object:FindFirstAncestorOfClass("Model")
    else
        return nil
    end
end

function PhysicsPickUp.DisableCollision(Object)
    local model = PhysicsPickUp.CheckModel(Object)
    if model then
        for i, v in ipairs(model:GetDescendants()) do
            if v:IsA("BasePart") then
                v.CollisionGroup = "held"
            end
        end
    else
        Object.CollisionGroup = "held"
    end
end

function PhysicsPickUp.EnabledCollision(Object)
    local model = PhysicsPickUp.CheckModel(Object)
    if model then
        for i, v in ipairs(model:GetDescendants()) do
            if v:IsA("BasePart") then
                v.CollisionGroup = "Default"
            end
        end
    else
        Object.CollisionGroup = "Default"
    end
end

function PhysicsPickUp.ToggleGrab()
    local object = PhysicsPickUp.CheckForObject()
    if grab == nil then
        if object and object.Anchored == false then
            local model = PhysicsPickUp.CheckModel(object)
            if model then
                if model.PrimaryPart then
                    object =  model.PrimaryPart
                end
            end
            PhysicsPickUp.DisableCollision(object)
            attachmentpart = Instance.new("Attachment")
            attachmentpart.Parent = object
            Orientationalign.Attachment0 = attachmentpart
            grab = object
            alignpostion.Attachment0 = attachmentpart
            
        end
    else
        PhysicsPickUp.Drop()
    end
end

function PhysicsPickUp.CheckDistance()
    if grab then
        local Distance = (grab.Position - player.Character.HumanoidRootPart.Position).Magnitude
        Orientationalign.CFrame = CFrame.lookAt(grab.Position, player.Character.Head.Position)
        if Distance > 10 then
            PhysicsPickUp.Drop()
        end
    end
end

function PhysicsPickUp.Drop()
    alignpostion.Attachment1 = nil    
    Orientationalign.Attachment0 = nil
    if attachmentpart then
        attachmentpart:Destroy()
    end
    PhysicsPickUp.EnabledCollision(grab)
    grab = nil
end

function PhysicsPickUp.Start()
    PhysicsPickUp.PartToCamera()
    PhysicsPickUp.CheckDistance()
end

--game:GetService("CollectionService"):HasTag(object, "Interaction")
return PhysicsPickUp

Best Regards,
Universe

Here are a few of my suggestions

  1. Comments: Your code is easy to read and understand. But, it’s a good idea to add comments to explain what each function and variable does (especially if this is open source). This can help others (and your future self) understand what the code is doing to mitigate the potential confusion.

  2. PhysicsService: Instead of changing the CollisionGroup of each individual part. You can use the PhysicsService:SetPartCollisionGroup to set the collision group for an entire model or part hierarchy. This is a very easy way to simplify your code and make it more efficient. I strongly recommend it, though again it doesn’t change the functionality just improves the overall user experience when modifying the code.

  3. Timeout: Instead of using WaitForChild without a timeout in your PartToCamera function, you could use WaitForChild with a timeout to avoid issues with waiting indefinitely.

  4. Performance: To improve performance consider using a debounce to limit the frequency of certain actions. (Such as checking the distance between the player and the currently grabbed object).

2 Likes

Correction, :SetPartCollisionGroup() is a deprecated method (and should not be used for new projects) and can only set the collision group of a single part according to the API reference

1 Like

That’s correct upon further review. My apologies.

2 Likes