Just wanted to know if this was good draft/concept for a OOP combat system

Help and Feedback Code Review
1

I’m basically trying to bring my work a step further, and revamp my previous combat because I didn’t like the feeling of it. I know that’s more up to the vfx part of the game, but I also the back end part of the work could also need a revamp as I go along with that VFX path of the game


local DamageInterpretation = {}
DamageInterpretation.__index = DamageInterpretation

function DamageInterpretation:New(Damage: number, Knockback: boolean, Blockable: boolean, AttackType: string, QiType: string, BlockBreak: boolean, KBTime: number)
	local DMGInter = setmetatable({}, self)
	
	DMGInter.__index = DMGInter
	
	DMGInter.Damage = Damage
	DMGInter.KnockBack = Knockback
	DMGInter.Blockable = Blockable
	DMGInter.AttackType = AttackType
	DMGInter.QiType = QiType
	DMGInter.BlockBreak = BlockBreak
	DMGInter.KBTime = KBTime
	
	
	return DMGInter
end


function DamageInterpretation:Damage(Character, HitHum, HitChar)
	
	local DMG: number = self.Damage
	local OGDMG = DMG
	
	if self.KnockBack == true then
		local BV = Instance.new("BodyVelocity")
		BV.Parent = HitChar:FindFirstChild("HumanoidRootPart")
		BV.MaxForce = Vector3.new(100000, 22500, 100000)
		BV.P = 33300
		BV.Velocity = Character.HumanoidRootPart.CFrame.LookVector * 40
		Debris:AddItem(BV, self.KBTime)
	end
	
	local Player = Players:GetPlayerFromCharacter(Character)
	local ePlayer = Players:GetPlayerFromCharacter(HitChar)
	
	if self.Blockable == true and self.AttackType == "Blunt" then
		DMG = DMG/2
	end
	
	damageHum(HitHum, DMG)
	
end

return DamageInterpretation

changed the topic place cause it was wrong

1 Like
2

The New method instead of having multiple parameters, why not have a single parameter which is a table. Also it’s not necessary to use “:” on New instead use “.”.

local AvailableProperties = {
	BlockBreak = "boolean",
	KnockBack = "boolean",
	Blockable = "boolean",
	AttackType = "string",
	Damage = "number",
	KBTime = "number",
	QiType = "string",
}

function DamageInterpretation.new(properties: {})
	return setmetatable({}, DamageInterpretation):_Initialize(properties)
end

function DamageInterpretation:_Initialize(properties: {})
	for key, value in pairs(properties) do
		local dataType = AvailableProperties[key]

		--[[
			This step is unnecessary all you need is `self[key] = value`,
			this just checks if the type of data is correct/the same and
			the key should be added.
		]]
		if dataType then
			if typeof(value) == dataType then
				self[key] = value
			else
				error(("%s must be a type of %s"):format(key, dataType))
			end
		else
			warn("%s can't be added")
		end
	end

	return self
end
3 Likes
3

Is this similar to an assert what you are doing in initialize so the scripter could know if the wrong value was input?

Otherwise, this is an interesting way to do it, glad I learned it before I got deep into OOP. Thank you.

Read through it and I think I understand it better. When you input the values it’s just checking if they are all equal to those specific values in the avaliable properties, and if so, then procceed to set the self/avaliable property values to that, or it’ll warn/error that the value isn’t equal to the avaliable properties.

(I just do this to understand better, like of active recalling for me.)

1 Like
4

Yes, and not many people would recommend using assert, especially on big projects where a lot of errors might occur and cause some performance issues.

I tend to make my own function which is Assert with the capital A.

local function Assert(value, errorMessage: string?)
	if not value then
		error(errorMessage)
	end
end

You can even make this as a global variable simply removing the local though this would only work with frameworks where, initializing the important parts get loaded first.

5

I think why assert causes performance issues on big projects is probably because it formats/concatenates the string with the method traceback on the Debug library.

1 Like
6

If you aren’t going to directly mutate self in the constructor, then you can just directly use the properties table for it.

--!strict

local Damager = {}
Damager.__index = Damager


type Properties = {
	Damage: number,
	KBTime: number,
	
	UseKnockBack: boolean,
	IsBlockable: boolean,
	OnBlockBreak: boolean,
	
	AttackType: "Blunt" | "etc.", -- We can use literals,
	QIType: "Ditto"
}

-- Hackish way of getting proper Type-Checking
export type Damager = typeof(setmetatable({} :: Properties, Damager))


function Damager.new(Properties: Properties): Damager
	return setmetatable(Properties, Damager)
end

function Damager:Damage(Character: Model): ()
	local self: Damager = self -- Allows Type-Checker to know what 'self' is
end


return Damager

local newDamager = Damager.new {
	Damage = 1,
	KBTime = 2,

	UseKnockBack = true,
	IsBlockable = false,
	OnBlockBreak = true,

	AttackType = "Blunt",
	QIType = "Ditto"
}

One issue with using a table for the constructor is that you don’t get a very good Auto-Complete recommendation when using it, but it’s much cleaner than listing out all the arguments.

2 Likes
7

I see, so that’s what the export is used for. I never really tried to mess with this because I thought this only works with plugins. I always have wanted to have an auto correct when doing something like this.

1 Like
8

Is this using Inhertiance to make a new class? I’m a little confused on a few of these functions such as type, export. I just skimmed over it somewhat of course so let me read through it more.

1 Like
9

It’s my rendition of this idiom that allows proper type-checking inside the constructor and method scopes + outside logic but most of it is syntax sugar. It doesn’t support inheritance but for standalone classes like this it’s more preferable.

10

so export type automatically makes the certain properties an automatically type check what it was : to, or if not what does type do and what does export do?

11

If a type has been exported from a module, it can be used as a type in any script that requires it.

So this

export type Foo = number

return Module

Allows you to do this

local Module = require(Module)

local Bar: Module.Foo = 1

It’s not necessary but it’s useful if you ever needed the type for your class on the receiving end. Otherwise, it functionally does nothing.

1 Like
12

Ah, so it just makes that specific variable get type-checked. Now, with damager new, would I make a different oop part of damageinterpretation and input that table into the values?

13

Took me some time to figure out but it’d probably just make a seperate damage function that takes those values and does what damage does. All in all, these responses were pretty helpful.

14

I happen to have a question about the implemenation of the actual table within the damage script after using _Initialize, because for some reason self is not acknowledged and all the values are considered nil. Do I need to turn _Initialize into a variable?

15

That’s because self gets the first parameter from the function, like this:

local module = {}

module.foo = function(self, a)
	print(a)
end

And there are two ways to call the method:

module.foo(module, "something")
module:foo("something")

So if you have called the method correctly self becomes the first parameter of the function. If you called the funciton like this module._Initialize() without passing in the object then self becomes null.

16

That’s the issue, it’s :_Initialize, not . so I don’t really understand as to why self is acting the way it is.