Scripting Animations

Help and Feedback Code Review
#1

I’ve scripted a few combat systems, and for animating I’ve gone between sending over animationIDs or animation names. All animations are stored in ReplicatedStorage, and this is what the code would look like.

Remote.OnServerEvent:Connect(function(code)
    for i, v in repstore.Animations:GetChildren() do
        if v.AnimationId == code then
            --play animation
        end
    end
end)

Is this a good way to do it? Can it be improved?

2 Likes
#2

If they are named correctly then also possible is:

Remote.OnServerEvent:Connect(function(code)
    local anim = repstore.Animations:FindFirstChild(code)
        if anim then
            --play animation
        end
end)

Otherwise, it looks pretty good to me.

2 Likes
#3

Did you notice that I’m checking animationId and not name?

#4

I did, your original post mentioned both IDs and names, it’s probably simpler and more readable to find the name, so long as everything is named correctly and uniquely.
If you do just want to use the IDs, then I can’t think of a better way, except to add a break to your loop once you’ve found the animation to play. Saves looping for no reason, and if you did happen to have duplicated an anim would prevent it playing twice.

#5

I used this fairly recently and it’s an amazing way to optimze. You should definitely try naming your animation the animation_id and probably concatenate it with something that makes it known what it is. Then just index it with repstore.Animations[string] instead of looping and checking. The internal system probably handles that faster and it looks cleaner.

#6

I have done this before, it may be faster but cleaner? Of course not… There is no way to differentiate animations when you use the name… I name animations by what they are about so I can easily find them in explorer

#7

One thing to note is that the GetChildren() function returns an unordered array, which means that the loop will iterate over the animations in a random order. This could cause issues if you have multiple animations with the same name or ID(Edge case which would never actually happen), as it would not guarantee that the correct animation is played.

To avoid this, you could consider storing the animations in a dictionary (using the animation ID or name as the key) instead of an unordered array. This would allow you to easily retrieve the correct animation using its ID or name. You pretty much cache everything to allow instant retrieval instead of forcing an O(n) lookup each time the method is called.

Here is an example code snippet that uses a dictionary to store the animations:

local animations = {}
for _, animation in pairs(repstore.Animations:GetChildren()) do
    animations[animation.AnimationId] = animation
end

Remote.OnServerEvent:Connect(function(code)
    local animation = animations[code]
    if animation then
        -- play the animation
    else
        warn("Animation not found for code: " .. code)
    end
end)

This code creates a dictionary of animations using the animation ID as the key. When a request is received from the server, it retrieves the animation from the dictionary using the provided code. If the animation is found, it can be played. If not, a warning message is printed to the console.

1 Like