Can I improve this code for my FPS ViewModel system?

Help and Feedback Code Review
,
#1

I have been following a youtube video for a FPS viewmodel. I have placed the viewmodel inside a Folder in ReplicatedStorage. I also added a ModuleScript stored in a Folder in ReplicatedStorage. I then made a Folder with a LocalScript inside it.

I was just wondering if there was any way I could improve these codes:

ModuleScript:

local ViewModelConfigurationModule = {}

ViewModelConfigurationModule.OffsetFromCamera = CFrame.new(1, -1.2, 2)

return ViewModelConfigurationModule

LocalScript:

local ReplicatedStorage = game:GetService("ReplicatedStorage")
local RunService = game:GetService("RunService")

local ViewModel = ReplicatedStorage:WaitForChild("ViewModels"):WaitForChild("ViewModel"):Clone()

local ViewModelConfigurationModule = require(ReplicatedStorage:WaitForChild("ModuleScripts"):WaitForChild("ViewModelConfiguration"))

local Camera = workspace.CurrentCamera

ViewModel.Parent = Camera

function PositionViewModel()
	ViewModel:SetPrimaryPartCFrame(Camera.CFrame * ViewModelConfigurationModule.OffsetFromCamera)
end

RunService.RenderStepped:Connect(PositionViewModel)
#2

This is all just for readability purposes. Your approach is fine, but it looks cluttered to me.
Here is my approach:

ModuleScript: Use the table form that a module script offers, it’s way cleaner.

return {
	OffsetFromCamera = CFrame.new(1, -1.2, 2)
}

LocalScript:

local RunService		= game:GetService("RunService")

local Workspace			= game:GetService("Workspace")
local CurrentCamera		= Workspace["CurrentCamera"]

local ReplicatedStorage		= game:GetService("ReplicatedStorage")

local ModuleScripts		= ReplicatedStorage:WaitForChild("ModuleScripts")
local ViewModelConfig	= ModuleScripts:WaitForChild("ViewModelConfiguration")
local Configuration		= require(ViewModelConfig)

local Folder		= ReplicatedStorage:WaitForChild("ViewModels")
local ViewModel		= Folder:FindFirstChild("ViewModel"):Clone()
do
	ViewModel["Parent"] = CurrentCamera
end

RunService["RenderStepped"]:Connect(function()
	ViewModel:SetPrimaryPartCFrame(CurrentCamera["CFrame"] * Configuration["OffsetFromCamera"])
end)
1 Like
#3

Yes. When I read over my original code I knew it felt unorganized and hard to read. Thanks for the reply.

1 Like
#4

I’ve never understood what

do
	--code
end

does, but I’ve seen it in a few big frameworks like profileservice. If you don’t mind, are you explain about what it does and it’s uses?

3 Likes
#5

Logic inside of a “do end” executes in a new local scope. Let’s say we have a variables “x”, it allows you to separate the “x” from the rest of the outside logic.

Example 1:

do
    local x = 5
end
print(x)
--[[
    If we try to print out x, it will just print nil,
    because the x was never defined outside the local scope.
]]
--Output: nil

Example 2:

local x
do
    x = 5
end
print(x)
--[[
    If we try to print out x, it will print 5.
]]
--Output: 5
1 Like
#6

So it’s still the same as just defining things in a scope? But without the need for any connections?

local part = script.Parent

part.Touched:Connect(function(otherpart)
   print(otherpart) -- prints something
end)
print(otherpart) -- prints nil

As interesting as it is i don’t think i understand any use cases for defining locals in another scope that i may/or may not be using, at least from my experience.

#7

You can use do ends to basically seperate different scopes, it’s helpful if you are working with variable scopes and garbage collection. Most of the times, you don’t need to use it, but it’s nice to be able to use it here and there for better readability.

Documentation: Programming in Lua : 4.2

We can delimit a block explicitly, bracketing it with the keywords do - end . These do blocks can be useful when you need finer control over the scope of one or more local variables.

1 Like
#8

It can also provide a micro optimisation, as do-end reduces the size of a scope (helping with environment indexing speeds). My knowledge of this comes from LuaJIT and Lua. So I’m unsure if LuaU does some sort-of compiler optimisations, making this redundant.

1 Like
#9

There was no need for a do end here:

do
	ViewModel["Parent"] = CurrentCamera
end

You have very well explained what’s the purpose of a do end, and since there’s no variable declaration in it, it’s not necessary.
Also, indexing properties with the bracket notation when it’s possible to use dot notation isn’t idiomatic, so is adding unnecessary whitespace to align equal signs.

#10

I’m sorry but that’s really bad orginization, if you use the wiki on roblox style, you will see that doing this:

local RunService		= game:GetService("RunService")

Is really bad, it looks worse and it’s hard to read.

You can look at it in General Whitespace:

https://roblox.github.io/lua-style-guide/

1 Like
#11

Ah, I actually didn’t know that, thank you. I will try to avoid unnecessary whitespaces from now on!

#12

Use ViewModel:PivotTo instead.

For more info click here

1 Like
#13

Yes, I was trying to imply that there is no need for a do end, but I still used it for better readability. Also, I have gotten so used to using the notations instead of periods that I try to avoid switching back because it will create a lot more work and mess up all the organization in my previous logic(s) and it also looks cleaner to me than using periods to index properties.