Is there a reason why you’re using a metatable in this module? Seems highly unnecessary when you can just return the function directly especially since the table doesn’t get used for anything and the idea here judging from the code sample is that you can require the module to get a function back.
local function ApplyPlayerCharacter(...)
end
return ApplyPlayerCharacter
The way of trying to define the Humanoid is a little strange as well. You’re using a WaitForChild with a timeout of 1 second and then switching to use FindFirstChildWhichIsA (doubly unnecessary because nothing inherits from Humanoid and it’s a superclass on its own). Could you not just settle for FindFirstChildOfClass only and if that returns nil then avoid doing anything animation related?
local humanoid = dummy:FindFirstChildOfClass("Humanoid")
if not humanoid then warn("No Humanoid, animations will not run") end
local animator = humanoid and humanoid:FindFirstChildOfClass("Animator")
if humanoid and not animator then
animator = Instance.new("Animator")
animator.Parent = humanoid
end
The whitelist stuff is also really unnecessary. This loops through the target NPC’s immediate children and for every child, runs another loop that checks an array. Not only could you do this from a dictionary and just make a single lookup per iteration instead, but you can crunch this down to a single if statement and even cut some work out using existing API.
humanoid:RemoveAccessories()
for _, child in ipairs(dummy:GetChildren()) do
if child:IsA("CharacterAppearance") then
child:Destroy()
end
end
Clothing and BodyColors are both subclasses of CharacterAppearance so by checking for CharacterAppearance you’ll catch both of those as well as any other missed assets like ShirtGraphic (T-Shirts). For R6 characters their bundles will be removed as well.
As for your handling of the contents returned from GetCharacterAppearanceAsync, important to remember that this is a single-layered model containing appearance information but some things don’t need to be parented or need to be handled specially. I’d recommend something like this:
for _, charItem in ipairs(characterAppearance:GetChildren()) do
if charItem:IsA("CharacterAppearance") then
charItem.Parent = dummy
elseif charItem:IsA("Accessory") then
-- Same as parenting, can be merged to above
humanoid:AddAccessory(charItem)
elseif charItem:IsA("ValueBase") and charItem.Name:sub(-5) == "Scale" then
charItem.Parent = humanoid
elseif charItem:IsA("Decal") and charItem.Name == "face" then
local face = dummy.Head:FindFirstChild("face", true)
if face then
face.Texture = charItem.Texture
else
charItem.Parent = dummy.Head
-- and any other handlers you want
end
characterAppearance:Destroy()
But to put the cherry on top, well… why would you need to do any of this when you can just use a HumanoidDescription? HumanoidDescriptions were built for the express purpose of having a native and easy way to change a character’s appearance provided you’re working with standard figures. It will clean out assets not required and apply incoming ones properly. In short: you can do what your 53-line module does in far less lines. Assuming this is a live game with readable code:
local Players = game:GetService("Players")
local function ApplyPlayerCharacter(userId, dummy)
local humanoid = dummy:FindFirstChildOfClass("Humanoid")
if not humanoid then warn("No Humanoid, cannot morph") return end
local success, description = pcall(function ()
return Players:GetHumanoidDescriptionFromUserId(userId)
end)
if success and description then
humanoid:ApplyDescription(description)
elseif not success or not description then
warn("Could not get HumanoidDescription: " .. description)
end
end
return ApplyPlayerCharacter
The only thing this code sample doesn’t come with is your application of the idle animation to a dummy. Not a problem, you can just use InsertService to directly get the animation data and play the animation. If you wanted animation functionality, you can handle that quickly too:
-- Working with the assumption that you add the param anim to the function above
-- and that InsertService is declared below the Players service. Also yes
-- this is legal, you don't need to redeclare a param as a local variable.
if not anim and description.IdleAnimation ~= 0 then
local success, idleAnim = pcall(function ()
return InsertService:LoadAsset(description.IdleAnimation)
end)
if success and idleAnim then
-- Heavy assumption that Animation1 will always be the name of the first
-- idle animation and that no other object will be named Animation1.
anim = idleAnim:FindFirstChild("Animation1", true)
elseif not success or not idleAnim then
warn("Could not get idle animation: " .. idleAnim)
end
end
-- Could probably be merged to the above if statement. Got lazy and lost
-- confidence so I just put it as its own reevaluation lol.
if not anim then return end
local track = humanoid:LoadAnimation(anim)
track.Looped = true -- Idles should be implicitly looped but...
track:Play()
Overall a good crack at a module for a system that’s widely used though, so good job in that respect. These are my constructive criticisms, I hope they’ll help in your development down the line.
! I actually learned a lot, so thank you. You can release this by yourself if you like, I can take this post down unless you’d rather me make publish some changes to the existing one