Door Visualisation Code

client
modulescript
programming
raw

#1

Resources

Place file:
DoorVisualiser.rbxl (14.7 KB)

Relevant object hierarchy:

StarterPlayer
> StarterPlayerScripts
> > DoorVisualiser
> > > CellDoor (example)

DoorVisualiser Code:

--[[
	Visualises the state of doors on the client. Children are intended to facilitate the process.
	
	colbert2677 - January 2019
--]]

local CollectionService = game:GetService("CollectionService")

local DoorTypes = script:GetChildren()

for _, DoorType in pairs(DoorTypes) do
	local DoorScript = require(DoorType)
	
	local DoorCollection = CollectionService:GetTagged(DoorType.Name)
	
	for _, Door in pairs(DoorCollection) do
		DoorScript.Attach(Door)
	end
end

CellDoor Code:

--[[
	CellDoor door type.
	
	colbert2677 - January 2019
--]]

local TweenService = game:GetService("TweenService")

local Visualiser = {}

Visualiser._TweenData = TweenInfo.new(0.75, Enum.EasingStyle.Back, Enum.EasingDirection.Out)
Visualiser._RotateAngle = 100

function Visualiser.Attach(Model)
	local Operator = Model.Operator
	local Open = Operator.Open
	local PrimaryPart = Model.Door.PrimaryPart
	local PrimaryPartCFrame = PrimaryPart.CFrame
	
	local self = {}
	
	self._CurrentTween = nil
	
	if Open.Value == true then
		PrimaryPart.CFrame = PrimaryPartCFrame * CFrame.Angles(math.rad(Visualiser._RotateAngle), 0, 0)
	end
	
	Open.Changed:Connect(function (NewValue)
		if self._CurrentTween and typeof(self._CurrentTween) == "Instance" then
			self._CurrentTween:Destroy()
			self._CurrentTween = nil
		end
		if NewValue == true then
			self._CurrentTween = TweenService:Create(PrimaryPart, Visualiser._TweenData, {CFrame = PrimaryPartCFrame * CFrame.Angles(math.rad(Visualiser._RotateAngle), 0, 0)})
			self._CurrentTween:Play()
		else
			self._CurrentTween = TweenService:Create(PrimaryPart, Visualiser._TweenData, {CFrame = PrimaryPartCFrame})
			self._CurrentTween:Play()
		end
	end)
end

return Visualiser


Feedback Description

Description:
The code I have posted above is, as the name states, code intended to establish the visuals for doors (specifically the tweening towards its open state). Previously, I handled tweening for the door model on the server but I figure that having the client do it would be far more effective and aesthetically pleasing. The server handles authentication (as it is supposed to be a card-locked door), while the client opens it as necessary. There is currently no server-side verification: I have not accounted for exploiters for a great number of reasons irrelevant for the purposes of this thread.

Issue & Prior Attempted Solution(s):
The current issue I take up with this code is how disgusting it looks, so I’m turning to Code Review to see if there are better ways to show my code or whether it’s fine as it is. Specifically the part that’s going on in the Changed statements has me unnerved.

	Open.Changed:Connect(function (NewValue)
		if self._CurrentTween and typeof(self._CurrentTween) == "Instance" then
			self._CurrentTween:Destroy()
			self._CurrentTween = nil
		end
		if NewValue == true then
			self._CurrentTween = TweenService:Create(PrimaryPart, Visualiser._TweenData, {CFrame = PrimaryPartCFrame * CFrame.Angles(math.rad(Visualiser._RotateAngle), 0, 0)})
			self._CurrentTween:Play()
		else
			self._CurrentTween = TweenService:Create(PrimaryPart, Visualiser._TweenData, {CFrame = PrimaryPartCFrame})
			self._CurrentTween:Play()
		end
	end)

Creating tweens is not an expensive operation so it’s not the optimisation part that I’m fidgety over, though I think that this isn’t proper. I’m creating new tweens every time I need to change the aesthetic of the door. Before this code manifested, I created the tweens upon attaching the code to a door (method Door.Attach) and placed them into the local environment. When the door was signalled to be open, it cancelled the closing animation if playing and played the open animation - this process, vice versa, for when the door was signalled to close. The difference for this code is destroying previously playing tweens and creating and playing a tween according to the state.

Is this code fine, or is there something better I can do?

Lots of notes to consider before providing feedback:

  • I’m not changing my naming convention. Don’t suggest things like “constants should be CAPS_AND_UNDERSCORES” or any of that. I’m not interested.
  • I’m not changing from ValueObjects, do not suggest I change to remotes. The only difference that’ll make is that I’ll experience communication delays.
  • Remain on-topic. My query pertains to whether the code I have is fine as is, or if there is a way I can modify it so that I’m not creating tweens every time the ValueObject’s value changes.
  • Though my major worry lies with the Changed section of code, I am also accepting improvements on the LocalScript DoorVisualiser. I currently grab all children, regardless of what they are and use them to act. This goes under the assumption that the children of the LocalScript will only ever be ModuleScripts.
  • Tags are only used to group objects. You don’t need to know of any other code associated with tags, because I add them via Studio’s command bar. The main code for doors on the server has the list of tags hard coded, while the client bases them off of child ModuleScripts to DoorVisualiser.
  • Doors are welded together using WeldConstraints. Everything except the PrimaryPart is unanchored. I’d also appreciate a brief addendum on if this is fine, as some people told me not to do it due to potential physics throttling acting on these parts. I refuse to use SetPrimaryPartCFrame due to offset issues and I have not completed my custom implementation of SetPrimaryPartCFrame yet.

#2

In my opinion, you’re fine in terms of the .Changed function. Tweens don’t have any sort of “reset to start” functionality, so it makes sense that you would delete and create new ones as needed.

The thing that seems to me to be a problem is your .Attach function is very restrictive and creates a potential memory leak. Currently, all of the data and logic related to the tween is hidden away inside of the function, and there is no way for another location to access it. This creates a potential memory leak because all of that information will stay in memory until the ValueObject is Destroyed, and you have no way to modify that data.

Although this code structure might be fine for you currently, it could hurt you sometime in the future. If I were you, I would want to make the code more OOP friendly by adding inheritance (super and sub classes) and the ability to track all of the data you’re using (return an Object or table of some sort)

Side note:

  • I’m not changing from ValueObjects . . . I’ll experience communication delays.

What do you mean by “communication delays”? As far as I know, RemoteEvents and ValueObjects are virtually equivalent in terms of communication speed.


#3

In what mannerism is the method Attach restrictive? It’s not necessarily intended to be loose. Nothing is destroyed except tweens, that data is meant to stay there. The only thing I can see wrong with it is that I’m storing a reference to a tween in a table within the local scope opened by the Attach method. I’m not sure how this would leak memory either. That data isn’t meant to be modified either.

How so? If you’re going to raise a point, please explain it.

Why? There is no need to use OOP here. This approach is fine as it is and I don’t feel that OOP is appropriate for a primitive system.

The least I can see inheritance modules doing for me is reducing how much work I put into creating a system, but even then that would be useless. One superclass would be enough to give me the basic structure of what I require to make doors but subclasses would not look any different from what I’m doing now.

Each door visualisation module is intended to be respectively unique. Inheritance and classes do not suit my situation, so I am not going to be switching to OOP. I would consider ECS, but not OOP. It’s not necessary. This coding format is not harmful, it works fine and it is not a concern I raised as per the Code Review request.

As far as I know, requests between the server and client need to travel to Roblox servers, so depending on the server’s region this time can be delayed (requests can take longer to cross, as it is device -> server and vice versa). If it only applies from client to server then excuse me, but if it applies from server to client then that is not good for me at all. ValueObjects take almost no time for their properties to replicate to the client once set from the server and I can listen for changes the way I currently do so with a 100% guarantee of no delays.

Typically the difference is fairly negligible from server to client, so it could be possible that I’m not aware of what I’m talking about. Regardless of whether my statement has truth to it or not, I do not wish to use remotes for this scenario and that is a firm decision.


#4

When I say “restrictive”, I mean that it makes individual Door objects (the self variable) more static and difficult to modify. If you know that throughout the development of your game you’ll never ever need to access a specific door and change something about it, then don’t change it. I simply think it’s a good idea to keep your options open and design it so that you have some way to access those Door objects.

As an (abstract) example of what I’m talking about: what if your player is in some part of the map, far from spawn, but most of the other players in the game are in spawn opening and closing a bunch of doors. This could potentially create excess lag for the player. If you can disable the animations for those doors while the player is nowhere near them, you can decrease lag and increase the player’s experience.

Again, if you don’t feel that you’re going to run into problems in the future, then leave it alone. I picked OOP as an example because it’s the most well-known on this forum. It seemed to me that having a Door superclass would help reduce redundancy, especially if all of your doors share similar operations, such as movement and SFX. ECS also works, but it would be annoying to implement the framework for it unless you already had it as part of your game.

At the end of the day, though, if it works it works. It’s your program and you know it better than I do, I’m just offering suggestions to give you some food for thought.

Communication Delays: Sorry, I wasn’t trying to dissuade you from using ValueObjects, I was just trying to understand your reasoning behind it. I don’t know the in-depth stats about Values vs REs, but I’ve always thought of RemoteEvents as the replacement for .Changed Server-Client events. :slight_smile:


#5

Did my use of the self variable confuse you as to what approach I’m taking? My code is intended to remain in the imperative paradigm. I simply used self because it was convenient, I had nothing else to use and I originally included more indices within the self table. The Attach method is intended to be restrictive and the self variable has no direct link to doors other than acting as a variable storage for the local scope.

I can’t realistically see this happening since the map is close together (it is not widespread), however I now understand where you are coming from. I can think of such optimisations when I work with larger maps, however the map I’m working with right now is a relatively small building. In addition to this, the lag created by accounting for doors not in a close proximity to the player should be fairly negligible until I start mass-creating doors. Your input has been kept in mind.

Typically boils down to a preference thing and people preference remotes over values, sometimes enough to state it as if it was an objective fact. Some tell you to avoid ValueObjects like the plague but there’s truthfully no harm in using them. I preferred to use ValueObjects mainly because it best suits my use case.

In any case though, I can reasonably discern that there doesn’t necessarily seem to be anything wrong with the approach I am making and should continue forward with it. Thanks for your input.