Code review of lua unit script

[smoth]

Code: Select all

-------------------------------------------------------
-- License:	Public Domain
-- Author:	Steve (Smoth) Smith
-- Date:	3/17/2011
-------------------------------------------------------

-- Piece names
	turret		= piece 'turret'
	base		= piece 'base'
	left		= piece 'left'
	right		= piece 'right'
	base_sleeve	= piece 'base_sleeve'
	base_barrel	= piece 'base_barrel'
	turret		= piece 'turret'
	sleeve_a	= piece 'sleeve_a'
	sleeve_b	= piece 'sleeve_b'
	pivot_a		= piece 'pivot_a'
	pivot_b		= piece 'pivot_b'
	barrel_a	= piece 'barrel_a'
	barrel_b	= piece 'barrel_b'
	wheels		= piece 'wheels'
	treads1		= piece 'treads1'
	treads2		= piece 'treads2'
	treads3		= piece 'treads3'
	treads4		= piece 'treads4'
	treads5		= piece 'treads5'

	firea		= piece 'firea'
	fireb		= piece 'fireb'

	bfire1		= piece 'bfire1'

	lfire1		= piece 'lfire1'
	lfire2		= piece 'lfire2'
	lfire3		= piece 'lfire3'

	rfire1		= piece 'rfire1'
	rfire2		= piece 'rfire2'
	rfire3		= piece 'rfire3'
	rfire4		= piece 'rfire4'
	
	lsmoke1		= piece 'lsmoke1'
	lsmoke2		= piece 'lsmoke2'
	lsmoke3		= piece 'lsmoke3'

	rsmoke1		= piece 'rsmoke1'
	rsmoke2		= piece 'rsmoke2'
	rsmoke3		= piece 'rsmoke3'
	rsmoke4		= piece 'rsmoke4'
	
-- State variables
	isMoving, isAiming = false, false
	local treadnum = 0
-- Firepoint vars
local shootingNumA, shootingNumB = 1, 1

-- Signal definitions
local SIG_AIM	=	{2,4,8,16}

-- effects for emitters
local effectA = 1024

local function Treads()
	while( 1 ) do
  		if(isMoving == true) then
			if(treadnum == 1) then			
				Hide (treads5)
				Show (treads1)
			end
			if(treadnum == 2) then
				Hide (treads1)
				Show (treads2)
			end
			if(treadnum == 3) then		
				Hide (treads2)
				Show (treads3)
			end
			if(treadnum == 4) then		
				Hide (treads3)
				Show (treads4)
			end
			if(treadnum == 5) then		
				Hide (treads4)
				Show (treads5)
			end
			treadnum = treadnum + 1
		
			if( treadnum == 6) then
				treadnum = 1
			end
				
		end
		Sleep (50)
	end
end


function script.StartMoving()
	isMoving = true
end

function script.StopMoving()
	isMoving = false
end	

--------------------------------------------------------
--start ups :)
--------------------------------------------------------
function script.Create()
	-- Initial State
	Turn(lsmoke1,	y_axis, 3.1415927, math.rad(90))
	Turn(lsmoke2,	y_axis, 3.1415927, math.rad(90))
	Turn(lsmoke3,	y_axis, 3.1415927, math.rad(90))

	Turn(rsmoke1,	y_axis, 3.1415927, math.rad(90))
	Turn(rsmoke2,	y_axis, 3.1415927, math.rad(90))
	Turn(rsmoke3,	y_axis, 3.1415927, math.rad(90))
	Turn(rsmoke4,	y_axis, 3.1415927, math.rad(90))
	
	Hide (treads2)
	Hide (treads3)
	Hide (treads4)
	Hide (treads5)

	StartThread(Treads)
end

function script.AimFromWeapon(weaponID)
	if(weaponID == 1) then
		return turret
	elseif(weaponID == 2) then
		return left
	elseif(weaponID == 3) then
		return right
	elseif(weaponID == 4) then
		return base_sleeve 
	end
end

function script.QueryWeapon(weaponID)
	if(weaponID == 1) then
		return firea
	elseif(weaponID == 2) then
		if (shootingNumB == 1) then
			return lfire1
		elseif (shootingNumB == 2) then
			return lfire2
		elseif (shootingNumB == 3) then
			return lfire3
		end
	elseif(weaponID == 3) then
		if (shootingNumA == 1) then
			return rfire1
		elseif (shootingNumA == 2) then
			return rfire2
		elseif (shootingNumA == 3) then
			return rfire3
		elseif (shootingNumA == 4) then
			return rfire4
		end
	elseif(weaponID == 4) then
		return bfire1
	end
end
	
-----------------------------------------------------------------------
--gun functions;
-----------------------------------------------------------------------	

-----------------------------------------------------------------------
-- This coroutine is restarted with each time a unit reaims, 
-- not the most efficient and should be optimized. Possible
-- augmentation needed to lus.
-----------------------------------------------------------------------
local function RestoreAfterDelayA()
		Sleep(1000)

		Turn(turret, y_axis, 0, math.rad(90))
		Turn(pivot_b,	x_axis, 0, math.rad(100))
		Turn(pivot_a,	x_axis, 0, math.rad(100))
		
		WaitForTurn(turret, y_axis)
		WaitForTurn(pivot_a, x_axis)
		WaitForTurn(pivot_b, x_axis)
end
-----------------------------------------------------------------------	
local function RestoreAfterDelayB()

		Sleep(1000)
		Turn(base, y_axis, 0, math.rad(50))
		Turn(right,	x_axis, 0, math.rad(100))
		
		WaitForTurn(base, y_axis)
		WaitForTurn(right, x_axis)

end
-----------------------------------------------------------------------	
local function RestoreAfterDelayC()
		Sleep(1000)
		Turn(base, y_axis, 0, math.rad(50))
		Turn(left,	x_axis, 0, math.rad(100))
		
		WaitForTurn(base, y_axis)
		WaitForTurn(left, x_axis)
end
-----------------------------------------------------------------------	
local function RestoreAfterDelayD()
		Sleep(1000)
		Turn(base_sleeve,	x_axis, 0, math.rad(390))
end

function script.AimWeapon(weaponID, heading, pitch)
	if (weaponID == 1) then
		Signal(SIG_AIM[weaponID])
		SetSignalMask(SIG_AIM[weaponID])
		
		Turn(turret, y_axis, heading, math.rad(50))
		Turn(pivot_b, x_axis, 0 - pitch, math.rad(100))
		Turn(pivot_a, x_axis, 0 - pitch, math.rad(100))
		
		WaitForTurn(turret, y_axis)
		WaitForTurn(pivot_a, x_axis)
		WaitForTurn(pivot_b, x_axis)
		
		StartThread(RestoreAfterDelayA)
	elseif (weaponID == 2) then
		Signal(SIG_AIM[weaponID])
		SetSignalMask(SIG_AIM[weaponID])
		
		Turn(base, y_axis, heading, math.rad(30))
		Turn(right,	x_axis, 0 - pitch, math.rad(100))
		
		WaitForTurn(base, y_axis)
		WaitForTurn(right, x_axis)
		
		StartThread(RestoreAfterDelayB)
	elseif (weaponID == 3) then
		Signal(SIG_AIM[weaponID])
		SetSignalMask(SIG_AIM[weaponID])
		
		Turn(base, y_axis, heading, math.rad(30))
		Turn(left, x_axis, 0 - pitch, math.rad(100))
		
		WaitForTurn(base, y_axis)
		WaitForTurn(left, x_axis)
		
		StartThread(RestoreAfterDelayC)
	elseif (weaponID == 4) then
		Signal(SIG_AIM[weaponID])
		SetSignalMask(SIG_AIM[weaponID])
		Spring.Echo(SIG_AIM[weaponID])
		Turn(base_sleeve, x_axis, 0 - pitch, math.rad(390))
		Turn(base_sleeve, y_axis, heading, math.rad(30))
		
		WaitForTurn(base_sleeve, y_axis)
		WaitForTurn(base_sleeve, x_axis)
		StartThread(RestoreAfterDelayD)
	end

	return true
end
	
function script.FireWeapon(weaponID) 
	if(weaponID == 1) then
		Move(barrel_a, z_axis,  -1)
		Move(barrel_b, z_axis,  -1)
		EmitSfx(firea, 1025) 
		EmitSfx(fireb, 1025) 
		EmitSfx(fireb, 2048) 
		Sleep(120)
		Move(barrel_b, z_axis, 0, 1)	
		Move(barrel_a, z_axis, 0, 1)	
	elseif(weaponID == 2) then
		if (shootingNumB == 1) then
			EmitSfx(lfire1, 1025) 
			EmitSfx(lsmoke1, 1027) 
		elseif (shootingNumB == 2) then
			EmitSfx(lfire2, 1025) 
			EmitSfx(lsmoke2, 1027) 
		elseif (shootingNumB == 3) then
			EmitSfx(lfire3, 1025) 
			EmitSfx(lsmoke3, 1027) 
		end
		shootingNumB = shootingNumB +1
		if (shootingNumB >=4) then
			shootingNumB = 1
		end		
	elseif(weaponID == 3) then
		if (shootingNumA == 1) then
			EmitSfx(rfire1, 1025) 
			EmitSfx(rsmoke1, 1027) 
		elseif (shootingNumA == 2) then
			EmitSfx(rfire2, 1025) 
			EmitSfx(rsmoke2, 1027) 
		elseif (shootingNumA == 3) then
			EmitSfx(rfire3, 1025) 
			EmitSfx(rsmoke3, 1027) 
		elseif (shootingNumA == 4) then
			EmitSfx(rfire4, 1025) 
			EmitSfx(rsmoke4, 1027) 
		end
		shootingNumA = shootingNumA + 1
		if (shootingNumA >=5) then
			shootingNumA = 1
		end
	elseif(weaponID == 4) then
		Move(base_barrel, z_axis,  -1)
		EmitSfx(bfire1, 1026) 
		Sleep(120)
		Move(base_barrel, z_axis, 0, 1)	
	end
end
	
function script.Killed(recentDamage, maxHealth)
	local severity = recentDamage/maxHealth
	if (severity <= 99) then
		-- Explode(left, SFX.FALL)
		-- Explode(right, SFX.FALL)
		-- Explode(l_arm, SFX.FALL)
		-- Explode(l_forearm, SFX.FALL)
		-- Explode(r_arm, SFX.FALL)
		-- Explode(r_forearm, SFX.FALL)
		-- Explode(gun, SFX.FALL)
		return 3
	else
		return 0
	end
end

Anyone have any thoughts on how I could possibly make this better?

[FLOZi]

Use tables. For example:

Code: Select all

local treads = {}
local numTreads = 5
local treadNum = 1
for i = 1, numTreads do
  treads[i] = piece ("treads" .. i)
end

local function Treads()
  while true do
    if isMoving then
      for i = 1, numTreads do
        if i == treadNum then
          Show(treads[i])
        else
          Hide(treads[i])
        end
      end
      treadNum = treadNum + 1
      if treadNum == 6 then treadNum = 1 end
    end
  Sleep(50)
  end
end

Otherwise, you are just writing bos.

Also noteworthy is that you can do multiple piece assignments on a single line:

Code: Select all

 local sleeve_a, pivot_a, barrel_a = piece("sleeve_a", "pivot_a", "barrel_a")

You can use SFX.CEG as syntactic sugar for 1024 when using EmitSFX.

In the mechwarrior mod, we have CEG's associated with each weapon, so we added a customParam to the weapon indicating which muzzleFX it should use, then added a bit of code in weapondefs_post.lua to automatically add these CEGs to the unitdefs, such that you can just use SFX.CEG + weaponID to reference the corresponding CEG to each weapon.

[knorke]

like Flozi said:

for the weapons too ofc.


To save the for i = 1, numTreads do loop try:
hide all threads pieces at Create() and do:

Code: Select all

local function Treads()
  while true do
    if isMoving then
       if i == threadNum then hidei = 1 else hidei = i-1 end
       Hide(treads[hidei])
	   Show(treads[i])
      treadNum = treadNum + 1
      if treadNum == 6 then treadNum = 1 end
    end
  Sleep(50)
  end
end

---
Turn(lsmoke1, y_axis, 3.1415927, math.rad(90))
math.pi
why mix rad & deg like this?
---
EmitSfx(firea, 1025)
->
local muzzleflash = SFX.CEG + 1
EmitSfx(firea, muzzleflash)
---
Flozi was faster so i stop.
also pastebin

[FLOZi]

Yeah, was a quick example, there's lots of room for improvement (Can surely be done with just the threadNum variable)

Also instead of having a table of SIG_AIMs, you could just do
Signal(2 ^ weaponID)

[jK]

Code: Select all

-- Piece names
	turret		= piece 'turret'
	base		= piece 'base'
...
-- State variables
	isMoving, isAiming = false, false
...

always define vars as local. (Except you use those vars in included scripts.)

Code: Select all

...	
function script.FireWeapon(weaponID) 
...
	elseif(weaponID == 2) then
		if (shootingNumB == 1) then
			EmitSfx(lfire1, 1025) 
			EmitSfx(lsmoke1, 1027) 
		elseif (shootingNumB == 2) then
			EmitSfx(lfire2, 1025) 
			EmitSfx(lsmoke2, 1027) 
		elseif (shootingNumB == 3) then
			EmitSfx(lfire3, 1025) 
			EmitSfx(lsmoke3, 1027) 
		end
		shootingNumB = shootingNumB +1
		if (shootingNumB >=4) then
			shootingNumB = 1
		end
...

also as the others already said use more tables, here:

Code: Select all

local wp2firepieces = {lfire1, lfire2, lfire3}
local wp2smokepieces = {lsmoke1, lsmoke2, lsmoke3}

function script.FireWeapon(weaponID) 
...
	elseif(weaponID == 2) then
		EmitSfx(wp2firepieces[shootingNumB], 1025) 
		EmitSfx(wp2smokepieces[shootingNumB], 1027)
		shootingNumB = (shootingNumB % 3) + 1
...

(note, it can even further optimized by putting those tables in another one to handle the weaponID, too)

[smoth]

Also noteworthy is that you can do multiple piece assignments on a single line:

Code: Select all

 local sleeve_a, pivot_a, barrel_a = piece("sleeve_a", "pivot_a", "barrel_a")

For short assignments this is ok but I avoid doing large sets like that. WAAY too many times have I had to check over long sql insert statements in stored procs with >20 fields.

[knorke]

does lua allow this?

Code: Select all

local   sleeve_a,   pivot_a,   barrel_a = 
piece ("sleeve_a", "pivot_a", "barrel_a")

[FLOZi]

Also noteworthy is that you can do multiple piece assignments on a single line:

Code: Select all

 local sleeve_a, pivot_a, barrel_a = piece("sleeve_a", "pivot_a", "barrel_a")

For short assignments this is ok but I avoid doing large sets like that. WAAY too many times have I had to check over long sql insert statements in stored procs with >20 fields.

I'm not suggesting you put all your pieces on a single line, but rather group related pieces as in my example.