I am currently doing a progress bar from the client, and i need to add +0.1 every time in a renderstepped loop, would it be fine if i were to send :FireServer in that loop and change the value of the number in the server?, and is there any difference if i use a unreliableRemoteEvent?
I would avoid this but if there’s no other way, then use unreliable remote events. They are better for performance when you are repeatedly firing remotes.
Also the client shouldn’t be changing server values because an exploiter could use it to their advantage, just a heads up.
Hope this helps.
3 Likes
not good. as the reasons mentioned by scavengingbot, also firing events will increase network traffic
your user case seem to be like having a cooldown or sprint point recovering and you use renderstepped to make the ui visual smoother.
we can do it this way, best if server initiate the cooldown recover request
-- server
player.Cooldown.Value = duration
CooldownStart:FireClient(player, duration)
task.wait(duration)
player.Cooldown.Value = 0
-- client on received do visual
3 Likes
Thank you both for your response, as my use case, is a supply drop open progress bar , I’ll probably need to reorganize how i do this system, Because there’s no current way the system can tell if you are opening the supply drop, But yeah, i’ll have it in consideration, thank you
1 Like
Why not just a proximity prompt?
you can always use ProximityPromptService if you don’t like the circle and want to style it into a progress bar.
3 Likes
we could use RemoteFunction
-- client , AttemptOpenSupply is RemoteFunction
local success, duration = AttemptOpenSupply:FireServer(supplyDrop)
if success then
-- visual that last for 'duration' amount of time
end
-- server
AttemptOpenSupply.OnServerInvoke = function(player, supplyDrop)
-- check if a valid request
if not CanOpenSupplyDrop(player, supplyDrop) then return false, math.huge end
task.spawn(function()
task.wait(duration)
supplyDrop.OpenBy(player)
end)
return true, duration
end
Proximity Prompt is also great
2 Likes
If possible, keep it all on the client. You could use prompts as suggested.
If you have to update client state on the server however, you should make use of Roblox’ replication as much as possible! For example, have a NumberValue that the client can access, and bind :GetPropertyChangedSignal("Value") to update the progress bar.
This way, the updates for the value are handled automatically by Roblox, doesn’t call an update if the value is unchanged and keeps it out of your bandwidth budget.
2 Likes
Since the supply drop is kinda global, that means all the people that have the “Progress bar” gui opened will contribute to the current progress number, depending on the amount of players opening it, you’re right, i could use a proximity prompt, my current idea is to add a tag to the crate model and then the server detects the instance with the tag, add some values into it like, ProgressValue, CurrentPlayersHelping, UnlockedBool and Finally Opened, if you are curious, this is
Now if i go for what you guys suggested, doing only the visuals on the client, then i guess i could do a remote event on the gui and do :fireclient to enable the renderstepped and disable it
For now, there’s only the crate loot gui opening function
how i have it right now :
task.wait(.1)
local players = game:GetService("Players")
local CollectionService = game:GetService("CollectionService")
local ReplicatedStorage = game:GetService("ReplicatedStorage")
local gfxfolder = workspace:WaitForChild("gfx")
local spark_part = ReplicatedStorage:WaitForChild("SparkPart")
local flare_bullet = ReplicatedStorage:WaitForChild("FlareBullet")
local set_crateownership = ReplicatedStorage:WaitForChild("SetCrateGuiOwnership")
local set_crateCurrentModel = ReplicatedStorage:WaitForChild("SetCurrentCrate")
local Flare_Visuals = ReplicatedStorage:WaitForChild("Flare_Visual")
local crateGui = ReplicatedStorage:WaitForChild("CrateGui")
local player = players.LocalPlayer
local current_ui: ScreenGui = nil
CollectionService:GetInstanceAddedSignal("SupplyCrate"):Connect(function(model: Model)
local Crate_E = model:WaitForChild("Crate_E")
model:SetAttribute("Opened", false)
local prox = Instance.new("ProximityPrompt")
prox.RequiresLineOfSight = false
prox.MaxActivationDistance = 12
prox.HoldDuration = 0
prox.Parent = Crate_E
prox.Triggered:Connect(function()
if player then
if model:GetAttribute("Opened") == false then
model:SetAttribute("Opened", true)
current_ui = set_crateownership:InvokeServer("Add_crategui", model)
else
model:SetAttribute("Opened", false)
if current_ui then
set_crateownership:InvokeServer("Remove_crategui")
end
end
end
end)
end)
local supply_gui = ReplicatedStorage:WaitForChild("CrateGui")
local SupplyCrates = workspace:WaitForChild("SupplyCrates")
local toolsFolder = sv:WaitForChild("Tools")
local GetTool = ReplicatedStorage:WaitForChild("GetTool")
local Ownership = ReplicatedStorage:WaitForChild("SetCrateGuiOwnership")
local SetCurrentCrate = ReplicatedStorage:WaitForChild("SetCurrentCrate")
Ownership.OnServerInvoke = function(player: Player, modetype: string, model_)
if player then
if modetype == "Add_crategui" then
if supply_gui:HasTag("UI_") then
if not model_:IsA("Model") then
player:Kick("...")
return nil
end
local clone_gui = supply_gui:Clone()
if model_ then
if clone_gui:FindFirstChild("CurrentCrate") then
local current_crate = clone_gui:WaitForChild("CurrentCrate")
current_crate.Value = model_
else
player:Kick("...")
end
end
clone_gui.Parent = player.PlayerGui
return clone_gui
end
elseif modetype == "Remove_crategui" then
local crate_ui = nil
if player.PlayerGui:FindFirstChild("CrateGui") then
crate_ui = player.PlayerGui:WaitForChild("CrateGui")
if crate_ui then
crate_ui:Destroy()
end
return true
end
end
end
return nil
end
I’ll try that
2 Likes
At the end i just decided for something like this, it may not be the best logic, but it works , somehow, if someone is wondering, I just update the Value on the server, may not be the best because, well, lag, but i am sure it’s better than Looping with RemoteEvents, Then i update the Crate attributes on the client so it’s not global, Like the “Opened” Value, Which tracks if the crate loot gui is Opened in the client
SupplyHandler (Located in starterplayerscripts) :
progressCrateFunction.OnClientInvoke = function(modetype: string, cratemodel: Model)
warn(cratemodel)
if cratemodel then
if not cratemodel:IsA("Model") then
player:Kick("...")
end
else
warn("Can't found "..cratemodel)
player:Kick("...")
end
if modetype == "Add_ProgressGui" then
progress_currentgui = progress_gui:Clone()
progress_currentgui.Parent = player.PlayerGui
-- Update Bar Size/Visuals with a Connection using RenderStepped
return progress_currentgui
elseif modetype == "Remove_ProgressGui" then
if progress_currentgui then
progress_currentgui:Destroy()
progress_currentgui = nil
else
player:Kick("...")
end
end
return nil
end
set_crateCurrentModel.OnClientInvoke = function(mode_: string | number, Attribute_: string, ValueToSet: boolean | number | string, cratemodel: Model)
if cratemodel then
if not cratemodel:IsA("Model") then
player:Kick("...")
end
else
warn("Can't found "..cratemodel)
player:Kick("...")
end
if mode_ == "SetAttribute" then
local find_ProgressValue = cratemodel:WaitForChild("OpenProgress")
if not find_ProgressValue then return warn("Can't find "..find_ProgressValue) end
if Attribute_ == "Unlocked" then
if typeof(ValueToSet) == "boolean" then
if ValueToSet == true and find_ProgressValue.Value < 100 then
player:Kick("...")
else
cratemodel:SetAttribute(Attribute_, ValueToSet)
end
end
else
cratemodel:SetAttribute(Attribute_, ValueToSet)
end
elseif mode_ == "CheckAttribute" then
local _att = cratemodel:GetAttribute(Attribute_)
return _att
end
return false or nil
end
Supply Handler (ServerScriptService) :
CollectionService:GetInstanceAddedSignal("SupplyCrate"):Connect(function(model: Model)
model:SetAttribute("Unlocked", false)
model:SetAttribute("Opened", false)
local Crate_E = model:WaitForChild("Crate_E")
local ProgressValue = model:WaitForChild("OpenProgress")
local prox = Instance.new("ProximityPrompt")
prox.RequiresLineOfSight = false
prox.MaxActivationDistance = 13
prox.ObjectText = ""
prox.ActionText = ""
prox.HoldDuration = 0
prox.Parent = Crate_E
ProgressValue:GetPropertyChangedSignal("Value"):Connect(function()
if ProgressValue.Value >= 100 then
prox:InputHoldEnd()
model:GetAttribute("Unlocked", true)
end
end)
prox.Triggered:Connect(function(player)
if model:GetAttribute("Unlocked") == false then
local progress_guiclone: ScreenGui = progressCrateFunction:InvokeClient(player, "Add_ProgressGui", model)
-- Update the Progress Value by Adding +1 every milisecond and so on with Heartbeat
end
end)
prox.TriggerEnded:Connect(function(player)
if model:GetAttribute("Unlocked") == true then return end
progressCrateFunction:InvokeClient(player, "Remove_ProgressGui", model)
end)
prox.Triggered:Connect(function(player)
if model:GetAttribute("Unlocked") == false then return end
if player then
if model:GetAttribute("Unlocked") == true then
if model:GetAttribute("Opened") == false then
model:SetAttribute("Opened", true)
Ownership:Invoke("Add_crategui", model)
else
model:SetAttribute("Opened", false)
Ownership:Invoke("Remove_crategui")
end
end
end
end)
end)
I’ll probably add more Sanity Checks
On a side note : something tells me this is not the most realiable way to do something like this, because it applies to a more expanded concept, like, Wanting to equip a gui when equipping a tool and doing the same (updating the visuals on the client), having to use a lot of remote functions and events, which may be very confusing
1 Like
omg, okay code review time, here’s mine, what do you think:
Small Issues
You are also making a local variable inside of the collectionservice handler, but you are only using it once, so you can just inline it.
--local crate_E = model:WaitForChild("Crate_E")
--(...)
--prox.Parent = Crate_E
prox.Parent = model.Crate_E--no reason to wait for child here.
You also might want to split the modetypes into different functions/remotes for maintainability, you want a function to generally only do one thing, not a function that is a jack of all trades, that would be a pain to maintain. By having different remotes you also don’t waste time on checking the modetype, making your code slightly (negligible) faster.
Also, remove the redundant player check in the beginning, player will never be null in a proximity prompt.
Error handling
Currently, you’re Kick()ing the player whenever an error occurs, you shouldn’t ever do that. Definitely not for something as trivial as moving a Gui into the player’s PlayerGui. Players will not like this. If some parameters don’t match the expected types (common with exploiters & server) then just ignore the request.
You can add some type checks at the beginning to secure this remote function, and just ignore requests that fail the type checks:
progressCrateFunction.OnClientInvoke = function(modetype: string, cratemodel: Model)
if typeof(cratemodel) ~= "Instance" or not cratemodel:IsA "Model" then return end
--(...)
end
Consistency
In programming, naming stuff like variables, is unavoidable. But random names will make your code base unmaintainable, so people have created naming conventions. Which is someone's style guide when they write code. If you work for a company, you'll follow their naming convention.
There are different styles of names, for example:
-
PascalCase -
CoolNameExample
each first letter of a new word in the name is capitalized. -
camelCase -
coolNameExample
each first letter of a new word, except the first letter of the name, is capitalized. -
snake_case -
cool_name_example
each space is replaced by an underscore. -
hungarian notation -
pStruct(had to use different example)
a prefix is added to the name to indicate something about the data that the symbol refers to, examples arepStructforpointer to a structure, orszArrayforsize of array -
etc…
naming conventions are style guides that use a combination of naming styles, for example a naming convention could say that you have to use PascalCase for top level symbols, like types, classes and functions that aren’t part of a class, camelCase for members of a class, and snake_case for variables.
You can see a clear naming convention for the Roblox API, examples are: camelCase for constructors, but PascalCase for types, classes and their members:
Color3.fromRGB()
Color3 - Classname - PascalCase | fromRGB - Constructor - camelCase
The point is, you need to be consistent with how you name things, and currently it doesn’t seem like you’re doing that, you use various names for variables like progress_currentgui (hybrid snake_case, lowercase?) and modetype (literally just lowercase), or mode_ (???).
Code itself
Now let’s compare what the code is trying to do, versus how it is doing it, and see if it is optimal.
It seems like you’re making a progress bar, like so said. And it must be global. You’re using a lot of values which you really don’t want to use. Values create an entire roblox instance, just to store one object, of like 8 bytes (a pointer to some object(8), a number(8), a boolean(1), or a string(variable number of bytes))
You seem to really like the mode_ approach instead of separation of concerns.
Finally, you let the client handle the managing, the client sets the attributes, etc; First of all, attributes are replicated one way, from the server to the client, therefore if the client sets attributes, only that client would see them, and no one else.
Let’s just ignore that detail for now, in your system, the client has the control and the server merely requests that something be done. This is not how it’s supposed to be, the server should decide everything and the clients request things. If the clients have the authority then exploiters can do whatever they want, you should make a system where client stuff is done on the client (like moving UIs, manipulating them, vfx, sfx etc) and server stuff is done on the server (physics, other computations, raycasting) and the client requests the server do something (like setting an attribute). The server should only ever request the client to have the client activate some vfx, sfx, or UI.
Your remotefunction, shouldn’t be a remotefunction, it should be a remote event, and the client will have to handle the UI bar, that’s not the server’s job, the server shouldn’t even be getting the UI element. And it should definitely not be updated in renderstep, that is very unoptimized.
Instead what you could do, is decide how long it takes for the supply drop to be collected. (eg 10 seconds) and have the client play a tween of the selected duration, then tell the client when to stop playing the tween. Then on the server you just wait the selected duration of time and then you do whatever you’d do when the supply crate is collected, here is an example script of how you can do this, with a consistent naming convention (snake case for local variables, pascal for functions) and it hopefully works:
local tween_service = game:GetService "TweenService"
local proximity_prompt_service = game:GetService "ProximityPromptService"
local prompts: {ProximityPrompt} = {}
local tweens: {Tween} = {}
proximity_prompt_service.PromptShown:Connect(function(prompt: ProximityPrompt)
if not prompt:HasTag "CratePrompt" then return end
local gui = Instance.new "BillboardGui"
local frame = Instance.new "Frame"
local filling = Instance.new "Frame"
local keycode = Instance.new "TextLabel"
keycode.Text = prompt.KeyboardKeyCode.Name
keycode.TextColor3 = Color3.new(1, 1, 1)
keycode.TextStrokeTransparency = 0
keycode.BackgroundTransparency = 1
keycode.Font = Enum.Font.BuilderSans
keycode.TextScaled = true
keycode.Size = UDim2.fromScale(1, 1)
keycode.ZIndex = 2
filling.BackgroundColor3 = Color3.fromRGB(85, 170, 255)
filling.Size = UDim2.fromScale(0, 1)
Instance.new("UICorner", filling)
frame.Size = UDim2.fromScale(1, 1)
frame.BackgroundColor3 = Color3.new(1, 1, 1)
Instance.new("UICorner", frame)
gui.Size = UDim2.fromScale(4, 1)
gui.StudsOffset = Vector3.new(0, 2, 0)
keycode.Parent = frame
filling.Parent = frame
frame.Parent = gui
gui.Parent = prompt.Parent
prompts[prompt] = gui
end)
proximity_prompt_service.PromptHidden:Connect(function(prompt: ProximityPrompt)
if prompts[prompt] then
prompts[prompt]:Destroy()
prompts[prompt] = nil
end
end)
proximity_prompt_service.PromptButtonHoldBegan:Connect(function(prompt: ProximityPrompt)
if not prompts[prompt] then return end
tweens[prompt] = tween_service
:Create(prompts[prompt]:FindFirstChildOfClass "Frame" : FindFirstChildOfClass "Frame", TweenInfo.new(prompt.HoldDuration), {["Size"] = UDim2.fromScale(1, 1)})
tweens[prompt]:Play()
end)
proximity_prompt_service.PromptButtonHoldEnded:Connect(function(prompt: ProximityPrompt)
if not tweens[prompt] then return end
tweens[prompt]:Cancel()
tweens[prompt] = nil
tween_service
:Create(prompts[prompt]:FindFirstChildOfClass "Frame" : FindFirstChildOfClass "Frame", TweenInfo.new(.5), {["Size"] = UDim2.fromScale(0, 1)})
:Play()
end)
this is the client script, it uses proximity prompt service to customize proximity prompts, then it plays a tween with the same length as the prompt’s HoldDuration.
It checks for a tag in case you have other prompts that shouldn’t be transformed,
and it saves tweens and created elements for destruction later.
On the server you just handle the .Triggered event, that’s what I meant with:
I meant you could use it like this.
On the server do this;
local collection_service = game:GetService "CollectionService"
local function AddPrompt(prompt: ProximityPrompt)
prompt.Style = Enum.ProximityPromptStyle.Custom --required for custom style
prompt.Triggered:Connect(function(plr: Player)
--assuming the parent is the lootbox
--award the player some score, or coins or whatever.
prompt.Parent:Destroy()--delete the lootbox.
end)
end
collection_service:GetInstanceAddedSignal "CratePrompt" : Connect(AddPrompt)
for _, prompt in ipairs(collection_service:GetTagged "CratePrompt") do AddPrompt(prompt) end
And boom, your system is done.
PS: you might want to add a .Destroying handler to avoid memory leaks in the client script in PromptShown:
prompt.Destroying:Connect(function()
if prompts[prompt] then
prompts[prompt]:Destroy()
prompts[prompt] = nil
end
if tweens[prompt] then
tweens[prompt]:Cancel()
tweens[prompt] = nil
end
end)
Hope this helps!
This was my first actual code review, most of the time I just point out some flaws, but I don’t actually go much in depth. I was initially doing a code review of your previous code, but you posted your new code so I deleted the mistakes that were now irrelevant and continued with your new code, luckily(unlucky for you 
) you made the same mistakes, I could keep two entire sections unmodified because of this, oof, I didn’t have to rewrite those.
PPS: if you don’t want it to be a billboard gui, just turn it into a screen gui, and parent it to the player’s PlayerGui instead.
2 Likes