How bad is my code

Started by SL1200, February 15, 2017, 04:16:47 PM

Previous topic - Next topic
February 15, 2017, 04:16:47 PM Last Edit: February 16, 2017, 01:50:10 AM by SL1200
Hi to all, I started playing around with Solarus over the last month and I'm really enjoying it. I'm a noob in regards to lua coding but I have a little exp with OOP using Actionscript and Java.
Now I know my code isn't pretty and I seem to keep over complicating things with my approach, but I occasionally figure out a much easier and more efficient way to achieve what I was doing.
So after bumbling my way through building a custom entity and its all working as I would like *or as close as possible* I was wondering if anyone would be able to give any pointers on how to make my code simpler and more efficient.

Here is the code for my R-Mag custom entity, yes I'm a PSU fan :)


Code (lua) Select
-- Created by SL1200 2017
-- Ranged Combat Support Machine: aka R-Mag
-- randomly helps player by selecting either:
-- a single enemy to attack
-- or to attack all enemies
-- R-Mag will have no effect on bosses *todo*
-- poss check name of enemy to skip collecting bosses into table *todo*

--[[
-- TODO:
add target marker ? would like the maker inbetween floor and enemy
see if location updates if enemy not frozen as this will mean
the target marker can follow the enemy ? make enemy panic movement if targted

might include abillity to autoheal hero when health is very low, only if player has any of the following: fairy, red potion, blue potion

--]]

local r_mag = ...
local game = r_mag:get_game()
local map = r_mag:get_map()
local hero = game:get_hero()
local hero_x, hero_y
local offset_x 
local offset_y
local hero_x_offset
local hero_y_offset
local hero_offset_pos

local r_mag_x, r_mag_y
local r_mag_pos
local mov_t = sol.movement.create("target")
local sprite
local state
local single_target_cool_down = "ready"
local full_spread_cool_down = "active"
local random_cool_down = "ready"
local entities_length = 0
local entities = {}
local enemies_to_kill
local sgl_tgt_x
local sgl_tgt_y
local start_delay
local s_attack_active
local f_attack_active

-- initialize r_mag
function r_mag:on_created()
r_mag:set_size(16, 16)
r_mag:set_origin(10, 13)
r_mag:set_traversable_by("hero", true)
r_mag:set_layer_independent_collisions()
if r_mag:get_sprite() == nil then
r_mag:create_sprite("entities/r_mag")
end
sprite = r_mag:get_sprite()
--r_mag:get_sprite():set_animation("stopped")
--full_spread_cool_down = "active"

end

-- makes sure full_spread() does not activate as soon R-MAG is activated
if start_delay == nil then
print("start_delay entered full spread on cooldown")
sol.timer.start(60200, function() full_spread_cool_down = "ready" end)
start_delay = false
end
-- movement main: r_mag moves to set positions to flank hero depending on hero direction
sol.timer.start(150, function () -- working
local game = r_mag:get_game()
local map = r_mag:get_map()
local hero = map:get_entity("hero")
local direction = hero:get_direction()
r_mag_x, r_mag_y = r_mag:get_position()
r_mag_pos = r_mag_x, r_mag_y
-- direction_up
if direction == 1 then
offset_x = 16
offset_y = 0
--print("Up")
end
-- direction_right
if direction == 0 then
offset_x = -16
offset_y = -20
--print("Right")
end
-- direction_down
if direction == 3 then
offset_x = 16
offset_y = -20
--print("Down")
end
-- direction_left
if direction == 2 then
offset_x = 16
offset_y = -20
--print("Left")
end
hero_x, hero_y = hero:get_position()
hero_x_offset = hero_x + offset_x
hero_y_offset = hero_y + offset_y
hero_offset_pos = hero_x_offset, hero_y_offset
sprite:set_direction(direction)
mov_t:set_target(hero, offset_x, offset_y)
mov_t:set_speed(100)
mov_t:set_smooth(true)

mov_t:start(r_mag)

return true
end)

-- sets bombos to use random positions
function r_mag:bombos_full_spread() -- working
  local map = r_mag:get_map()
  local hero = map:get_hero()
  local _x, _y, layer = hero:get_position()
  local x, y = map:get_camera():get_position()
  local random_x = math.random(x, x + 320)
  local random_y = math.random(y, y + 240)

  map:create_custom_entity{
    model = "bombos",
x = random_x,
y = random_y,
layer = 2,
width = 16,
height = 16,
    direction = 0,
  }
  sol.audio.play_sound("8bit_bomb_explosion")
end

-- sets bombos to use targeted location
function r_mag:bombos_single_target() -- working
  local map = r_mag:get_map()
  local hero = map:get_hero()
 
  map:create_custom_entity{
    model = "bombos",
x = sgl_tgt_x,
y = sgl_tgt_y,
layer = 2,
width = 16,
height = 16,
    direction = 0,
  }
  sol.audio.play_sound("8bit_bomb_explosion")
end

-- triggers cooldown of either attack
function r_mag:reset_cool_down() -- working
r_mag:get_sprite():set_animation("walking")
target_collected = false
if single_target_cool_down == "active" then

sol.timer.start(30000, function()
single_target_cool_down = "ready"

print("single target attack available")
end)

elseif full_spread_cool_down == "active" then

sol.timer.start(1200000, function()
full_spread_cool_down = "ready"

print("full spread attack available")
end)
else

end
end

-- collects a single enemy and location for single bombos to target
function r_mag:collect_target() -- working

-- find and place an enemey in table
local x, y = map:get_camera():get_position()
-- collect all entities on screen
for other in r_mag:get_map():get_entities_in_rectangle(x, y, 320, 240) do
-- if they are an enemy put into table entities
if other:get_type() == "enemy" then
table.insert(entities, other)
-- break kills loop so only 1 enemy is placed inside
break
end
end

-- freeze targeted enemy and grab its position
for j, enemies in ipairs(entities) do
enemies:stop_movement()
sgl_tgt_x, sgl_tgt_y = enemies:get_position()
--print(sgl_tgt_x)
--print(sgl_tgt_y)
end

-- place single bombos on targeted enemy
r_mag:bombos_single_target()

-- set delay while animation plays then kill enemy in table
sol.timer.start(200, function()
for i, enemies in ipairs(entities) do
enemies:hurt(10)
r_mag:reset_cool_down()
s_attack_active = false
print("Single target attack complete, on cooldown")
end
end)
end

-- triggers single enemy attack
function r_mag:single_target() -- working
if state == "flanking" and single_target_cool_down == "ready" then
print("choosing random enemy")
single_target_cool_down = "active"
s_attack_active = true

sol.timer.start(1000, function()

r_mag:collect_target()
end)
end
return entities
end

-- triggers all enemies on screen attack
function r_mag:full_spread() -- working
if state == "flanking" and full_spread_cool_down == "ready" then
r_mag:get_sprite():set_animation("attacking")
full_spread_cool_down = "active"
f_attack_active = true
local x, y = map:get_camera():get_position()
local bdi = 200 -- bomb delay interval
local bombos_count = 0
local entities = {}

-- collect all enemies and place reference in table
for other in r_mag:get_map():get_entities_in_rectangle(x, y, 320, 240) do

if other:get_type() == "enemy" then
table.insert(entities, other)
--print(#entities)
end

-- stop enemies in table moving
for i, enemies in ipairs(entities) do
--print("freeze loop")

enemies:stop_movement()
end
end

-- set timer for delay between bombos explosions
sol.timer.start(bdi, function()
r_mag:bombos_full_spread()
bombos_count = bombos_count + 1
return bombos_count < 15
end)

-- set timer delay for animations then kill everything in the table
sol.timer.start(bdi * 15, function()
for i, enemies in ipairs(entities) do
enemies:hurt(10)
r_mag:reset_cool_down()
f_attack_active = false
print("full spread attack complete, on cooldown")
end
end)
end
end

-- chooses which attack to use depending on cooldowns and if already attacking
function r_mag:attack_request() -- working
-- check if there are any enemies to kill on screen
local x, y = map:get_camera():get_position()

for other in r_mag:get_map():get_entities_in_rectangle(x, y, 320, 240) do
if other:get_type() == "enemy" then
enemies_to_kill = true

break
else
enemies_to_kill = false
end
end

-- !!! end script here if there are no enemies to kill !!!
if enemies_to_kill ~= true  then return false end

-- if there are enemies then check we can proceed with an attack
if full_spread_cool_down == "ready" and s_attack_active ~= true then

r_mag:get_sprite():set_animation("attacking")
r_mag:full_spread()
enemies_to_kill = false

elseif single_target_cool_down == "ready" and f_attack_active ~= true then

r_mag:get_sprite():set_animation("attacking")
r_mag:single_target()
enemies_to_kill = false

else
-- R-MAG does nothing as either cooldown is active or other attack in progress
enemies_to_kill = false
end
end

-- updates if r_mag is moving or flanking hero and triggers attack request
function r_mag:on_movement_changed(mov_t) -- working
local direction = hero:get_direction()
sprite:set_direction(direction)
if hero_offset_pos == r_mag_pos then
state = "flanking"
else
state = "moving"
end
r_mag:attack_request()
end

-- constructor function
function r_mag:create_r_mag() -- n/a used to create the r_mag
local map = r_mag:get_map()
local hero = map:get_hero()
local _direction = hero:get_direction()
local _x, _y
if direction == 0 then
_x, _y = 12, -4
elseif direction == 1 then
_x, _y = 0, -18
elseif direction == 2 then
_x, _y = -14, -4
elseif direction == 3 then
_x, _y = 0, 10
end
local x, y, layer = hero:get_position()
map:create_custom_entity{
model = "r_mag",
x = x + 16,
y = x + 16,
layer = layer,
width = 16,
height = 16,
direction = _direction,
}
end

return r_mag


Any advice would be great



February 16, 2017, 12:23:28 AM #1 Last Edit: February 16, 2017, 12:29:06 AM by Diarandor
If you already have some background in OOP and already made that script, I would not say that you are a noob (at least for what you will need to do in Lua); I am not a "true" programmer in the sense that I don't have studied a programming degree and I can say that Lua is very easy (compared to C++ and other languages), so probably you already know almost all what you need.

I am curious about what exactly this R-Mag entity is, do you have a video to see this entity in action? :)

Note that there is a typo in the line "enemies_to_kil = false" that should be "enemies_to_kill = false". I recommend that, when you post code here and use the code enviroinment, write
Code (Lua) Select
[code=Lua]
instead of
[code]
so that we can see the line numbers (very important to comment things about the code) and the code as in Lua.

EDIT: another remark: when you use "random" functions, it is recommended that you initialize them with a random seed, which is usually done by using the time with "math.randomseed( os.time() )"; you only need to do this once in your script before you call random functions.
"If you make people think they're thinking, they'll love you. But if you really make them think, they'll hate you."

February 16, 2017, 02:32:24 AM #2 Last Edit: February 16, 2017, 02:35:14 AM by SL1200
Thanks for the tip on code environment.
My background has definitely helped me with Solarus as mostly everything isn't foreign to me, and yes the language is much simpler than languages like Java or C++. That being said never done any real training its mainly learning from tutorials / books and trial and error, its the trig that completely mystifies me I've got a lot of learning to do in that area.

R-Mags are from the Phantasy Star series: They are like a familiar that follows you and attacks enemies every now and again.
Here is a link to a video of it in action:
https://www.dropbox.com/s/bo7cd2g3f9xkccs/R-MAG.avi?dl=0

And yes the info about using a random seed is really helpful I originally set up the mag to pick random number 1-100 and if it divided by 10 with no remainder to then call the single target attack and if it landed on 77 to do full spread, but the random number was always the same, this explains why : "When you call math.random(1,100) from Lua, the low-bit difference vanishes and you see the same integer result." may just keep on timers or if I change it back to random I'll add an auto-heal when health is critical so it serves more purpose.

Many thanks for the info and the typo :)

February 16, 2017, 10:01:11 AM #3 Last Edit: February 16, 2017, 02:04:38 PM by Diarandor
Wow, this is very cool. It's like a useful drone-version of Navy Navi. ;D

Some other small advices (not so important). I might be wrong with some of them.

1-The context for the timer on line 65 is the map, so if you leave that map the timer is cancelled, which may produce a bad behavior, maybe. You could use a more global context instead, so that the timer is preserved between maps. That timer has a long delay and it is unlikely that the player remains in the same map for so long, unless the map is huge. There are other timers in your code with huge delays, so the same could be done.

2-If you use several times the "map" and "hero" entities, it is worth it to save them in a variable that is common for that script, i.e., a variable defined in the script scope (above in the script), so that you avoid having to get them again and again. Note that map and hero do not change if you don't leave a map (actually, the hero entity is never removed).

3-This is not important, just a matter of taste, but if you like to make the code shorter, lines 77-99 can be shortened with something like:
Code (Lua) Select

local offset_x_list = {-16, 16, 16, 16}
local offset_y_list = {-20, 0, -20, -20}
offset_x = offset_x_list[direction + 1]
offset_y = offset_y_list[direction + 1]

And similarly for lines 328-337. (I like short and compact code since it is usually faster to read.)
"If you make people think they're thinking, they'll love you. But if you really make them think, they'll hate you."

February 16, 2017, 11:28:15 AM #4 Last Edit: February 16, 2017, 12:18:20 PM by SL1200
Many thanks, not sure what Navy was so had a look. Yes they are pretty much the same thing, ideally I would want mine to be activated on a keybinding once it has been acquired so the hero could turn it on or off.
These are the sort of things I was looking for help with :)
The map you see while large enough to scroll around is purely for testing everything as its made or used from source like hookshot and boomerang.
There are no other maps connected to it so I would not have seen that timer problem till some time later.

Yes I am curious about the scope issue, maybe I'm just missing something simple but as I understand it : the scope of variables declared at top (I see them as a global variable) can be used by everything below and any local variable declared inside a function has its scope limited to that function while the value of it can be passed out / processed to be used elsewhere.
Rightly so a global var should have been all I needed and I use them elsewhere without issue, but for some reason I ran into problems with accessing something and adding them resolved it, I'll have to have a look back into what I did to make it mess up.

Yeah that's much cleaner and shorter I'll have to get that implemented.

February 16, 2017, 12:15:54 PM #5 Last Edit: February 16, 2017, 01:29:27 PM by SL1200
Just had a quick look over removing the local vars that should be using globals, when I started the script the first goal was movement and think this is where the problem stemmed from:
The script originally had the global: local direction =  hero:get_direction() but this function would not pick it up. I have just re-tested it and it wont pick up the global so the entity wont change direction unless its declared locally.

Code (lua) Select
local direction = hero:get_direction()
function r_mag:on_movement_changed(mov_t) -- working
--local direction = hero:get_direction() --<< only works if un-commented despite the var being declared outside with the rest
sprite:set_direction(direction)
if hero_offset_pos == r_mag_pos then
state = "flanking"
else
state = "moving"
end
r_mag:attack_request()
end


Think I was just being overly cautious as to my understanding it should have access, as it didn't work that way I followed adding them to all functions created after this issue just to be sure.
I've now removed everything else except for two references and it's all still working so any light to be shed on the reason this particular global cannot be accessed correctly would be great.

I did not read the whole code but globals are generally bad practice, everything should be declared local unless there is a good reason.

-I wanted to say Navi (the fairy), not Navy, I misspelled that.

-The variables in your script (at least for the first version that you posted) are all local (the scope is limited), you should not call them global because global means another thing (global variables are accesible from other scripts). Whenever you write "local", you are defining a local variable, obviously. You can call them file-local/function-local/block-local variables, depending on the case.

Global variables should not be used unless it is necessary, so it is recommended not to use them unless you know what you are doing (these are never garbage collected, and use more "memory" or whatever that is called); so don't remove your "local" declarations from your variables. Similarly, local variables with bigger scopes use more "resources" (whatever that means), so it is better to use the smaller scope in most of the cases; for some variables that are used a lot in a file, like the map or the hero, it is ok to use file-local variables to avoid repeating code to get them, which also makes the code faster; but that is not always the case.

-In your last code above, it is unnecessary and bad to define the direction outside the function (line 1), so just remove that line and define it inside the function. Besides, if you define it outside, the direction value will not be updated each time you call the function.
"If you make people think they're thinking, they'll love you. But if you really make them think, they'll hate you."

Yeah I understand there is a difference with global vars and local vars, it was more as you said for the sake of differentiating the scope of the variables inside the script, its just me using bad terminology.
Well that makes sense and it was set that way, it was me not clicking on it wouldn't get updated if it was outside the function, when  I was trying to reduce the use of local vars inside functions that could be file-local.

Looks like there is still plenty of things to work out on this entity that I never considered.

Many thanks for the help on this, it's much appreciated.