Question about efficiency and optimization

[Tails]

This might be a rather embarrassing question... but I was really wondering how to make the code below a little more efficient.

I probably could manage to shorten the code but it would take a long time to figure out how I do that. I'd really appreciate your guys' input on this.

As you can see I'm I got several markers all pointing to the same destination. The entrance markers have one destination and the exit markers have one destination.

I've always learned that copy and pasting code like this is that in the end it's going to turn out really bad for your script and how you're managing it. It's way too inefficient. Not only is it going to be resource-intensive but it's also going to be one heck of a job to make tiny adjustments to the code.

For some reason, though, I always manage to find myself writing code like how it's written below because it's the only way that makes sense to me.

In all honesty, I kind of exaggerated the code below a little bit but it's only to show you that I don't know another way of doing it.

ENTRANCE_1 = createMarker(-1962.752, 440.72, 36.172,"arrow",2,251,253,5) 
ENTRANCE_2 = createMarker(-1962.634, 434.199, 36.172,"arrow",2,251,253,5) 
ENTRANCE_3 = createMarker(-1946.38, 456.753, 36.172,"arrow",1.8,251,253,5) 
ENTRANCE_4 = createMarker(-1939.389, 457.292, 36.172,"arrow",1.8,251,253,5) 
ENx,ENy,ENz,ENr = -1943.941, 418.03, -4.327, 3.084 -- destination (inside) 
  
EXIT_1 = createMarker(-1942.011, 415.468, -3.427,"arrow",1.8,251,253,5) 
EXIT_2 = createMarker(-1946.967, 415.51, -3.427,"arrow",1.8,251,253,5) 
EXIT_3 = createMarker(-1923.083, 445.064, -0.25,"arrow",2,251,253,5) 
EXIT_4 = createMarker(-1924.974, 438.646, -0.25,"arrow",2,251,253,5) 
EXx,EXy,EXz,EXr = -1970.884, 437.612, 35.172, 89.306 --destination (outside) 
  
function enterRoom(player) 
    local elementType = getElementType(player) 
    if elementType == "player" and not isPedInVehicle(player) then 
        if source == ENTRANCE_1 then 
            fadeCamera(false,1.0,0,0,0) 
            setTimer(fadeCamera,1200,1,true,1.0,0,0,0) 
            setTimer(setElementPosition,1000,1,player,ENx,ENy,ENz) 
        elseif source == ENTRANCE_2 then 
            fadeCamera(false,1.0,0,0,0) 
            setTimer(fadeCamera,1200,1,true,1.0,0,0,0) 
            setTimer(setElementPosition,1000,1,player,ENx,ENy,ENz) 
        elseif source == ENTRANCE_3 then 
            fadeCamera(false,1.0,0,0,0) 
            setTimer(fadeCamera,1200,1,true,1.0,0,0,0) 
            setTimer(setElementPosition,1000,1,player,ENx,ENy,ENz) 
        elseif source == ENTRANCE_4 then 
            fadeCamera(false,1.0,0,0,0) 
            setTimer(fadeCamera,1200,1,true,1.0,0,0,0) 
            setTimer(setElementPosition,1000,1,player,ENx,ENy,ENz) 
        elseif source == EXIT_1 then 
            fadeCamera(false,1.0,0,0,0) 
            setTimer(fadeCamera,1200,1,true,1.0,0,0,0) 
            setTimer(setElementPosition,1000,1,player,EXx,EXy,EXz) 
        elseif source == EXIT_2 then 
            fadeCamera(false,1.0,0,0,0) 
            setTimer(fadeCamera,1200,1,true,1.0,0,0,0) 
            setTimer(setElementPosition,1000,1,player,EXx,EXy,EXz) 
        elseif source == EXIT_3 then 
            fadeCamera(false,1.0,0,0,0) 
            setTimer(fadeCamera,1200,1,true,1.0,0,0,0) 
            setTimer(setElementPosition,1000,1,player,EXx,EXy,EXz) 
        elseif source == EXIT_4 then 
            fadeCamera(false,1.0,0,0,0) 
            setTimer(fadeCamera,1200,1,true,1.0,0,0,0) 
            setTimer(setElementPosition,1000,1,player,EXx,EXy,EXz) 
        end 
    end 
end 
addEventHandler("onClientMarkerHit",resourceRoot,enterRoom) 

Here another example of bad coding. Isn't there a way so you only have to write the functions down once?

function toggleWindow() 
    if guiGetVisible(WINDOW) then 
        guiSetVisible(WINDOW,false) 
        showCursor(false) 
        showPlayerHudComponent("all",true) 
        showChat(true) 
    else 
        guiSetVisible(WINDOW,true) 
        showCursor(true) 
        showPlayerHudComponent("all",false) 
        showChat(false) 
    end 
end 

[Noki]

entryMarkers = {[1] = {15, 66, 7}, [2] = {4, 7, 8}} 
  
for _, v in ipairs(entryMarkers) do 
     local entryMarker = createMarker(v[1], v[2], v[3]) 
     addEventHandler("onClientMarkerHit", entryMarker, handlerFunction) 
end 

This won't necessarily make the runtime more efficient, but it will make editing the code in the future much easier. You can also do the same with your exit markers. That way you get rid lf the big if and elseif statements.

function toggleWindow() 
     guiSetVisible(window, not guiGetVisible(window)) 
     showCursor(not isCursorShowing()) 
end 

You basically set these values to what they boolean opposite currently is (true becomes false, false becomes true).

[Tails]

Thank you very much!

I'm trying to destroy a bunch of markers that I've spawned using your method but it can't find the element even though they spawn.

Any ideas?

ambilightEnabled = false 
function handleAmbilight(cmd, switch) 
    if cmd then 
        ambilightEnabled = not ambilightEnabled 
    end 
    for _, v in ipairs(ambiMarkers) do 
        if ambilightEnabled == true then 
            local ambiMarker = createMarker(v[1], v[2], v[3], ambiType,ambiSize, aR ,aG, aB) 
        else 
            destroyElement(ambiMarker) 
        end 
    end 
end 
addCommandHandler("ambilight",handleAmbilight) 

[Noki]

Each marker is assigned a local variable within that loop which is then destroyed, so you can't access the marker element outside of that function and its attached event handler. And making it a global variable won't help as it will only iterate over the previous marker each time. That's where we use tables.

allMarkers = {} 
entryMarkers = {[1] = {15, 66, 7}, [2] = {4, 7, 8}} 
  
for i, v in ipairs(entryMarkers) do 
     allMarkers[i] = createMarker(v[1], v[2], v[3]) 
     addEventHandler("onClientMarkerHit", allMarkers[i], handlerFunction) 
end 

You assign each marker an index position within the table (numbers from one onward). To access a marker element, just do allMarkers[number], where number is the index from one to the amount of markers.

To destroy the markers:

for i in ipairs(entryMarkers) do 
     if (allMarkers[i] and isElement(allMarkers[i])) then -- Check to see if the marker exists within the table and also check to see if the value of that key is an element 
          destroyElement(allMarkers[i]) 
     end 
end 

Edited December 16, 2015 by Guest

[Tails]

I'm getting an unexpected symbol near '[' at

 allMarkers[i] = createMarker(v[1], v[2], v[3]) 

or on line 17 in my code. I just can't figure out why that is. Been changing it for the past 1.5 hours now.

allAmbiMarkers = {} 
  
ambiMarkers = {[1] = {-1963.7998046875,454.2998046875,0.30000001192093},[2] = {-1963.7998046875,454.2998046875,6.6999998092651}, 
                [3] = {-1963.7998046875,454.2998046875,13.10000038147},[4] = {-1953.7998046875,454.2998046875,13.10000038147}, 
                [5] = { -1943.5,454.2998046875,13.10000038147 },[6] = {-1933.400390625,454.2998046875,13.10000038147}, 
                [7] = {-1923.2001953125,454.2998046875,13.10000038147},[8] = {-1923.2001953125,454.2998046875,6.6999998092651}, 
                [9] = {-1923.2001953125,454.2998046875,0.30000001192093}, 
} 
  
ambilightEnabled = false 
function handleAmbilight(cmd, switch) 
    if cmd then 
        ambilightEnabled = not ambilightEnabled 
    end 
    if ambilightEnabled == true then 
        for i, v in ipairs(ambiMarkers) do 
            local allAmbiMarkers[i] = createMarker(v[1], v[2], v[3], ambiType,ambiSize, aR ,aG, aB) 
        end 
    else 
        for index, marker in ipairs(entryMarkers) do 
            if (allAmbiMarkers[i] and isElement(allAmbiMarkers[i])) then 
                destroyElement(allAmbiMarkers[i]) 
            end 
        end 
    end 
end 
addCommandHandler("ambilight",handleAmbilight) 

Edited December 16, 2015 by Guest

[Saml1er]

I'm getting an unexpected symbol near '[' at

 allMarkers[i] = createMarker(v[1], v[2], v[3]) 

or on line 17 in my code. I just can't figure out why that is. Been changing it for the past 1.5 hours now.

allAmbiMarkers = {} 
  
ambiMarkers = {[1] = {-1963.7998046875,454.2998046875,0.30000001192093},[2] = {-1963.7998046875,454.2998046875,6.6999998092651}, 
                [3] = {-1963.7998046875,454.2998046875,13.10000038147},[4] = {-1953.7998046875,454.2998046875,13.10000038147}, 
                [5] = { -1943.5,454.2998046875,13.10000038147 },[6] = {-1933.400390625,454.2998046875,13.10000038147}, 
                [7] = {-1923.2001953125,454.2998046875,13.10000038147},[8] = {-1923.2001953125,454.2998046875,6.6999998092651}, 
                [9] = {-1923.2001953125,454.2998046875,0.30000001192093}, 
} 
  
ambilightEnabled = false 
function handleAmbilight(cmd, switch) 
    if cmd then 
        ambilightEnabled = not ambilightEnabled 
    end 
    if ambilightEnabled == true then 
        for i, v in ipairs(ambiMarkersA) do 
            local allAmbiMarkers[i] = createMarker(v[1], v[2], v[3], ambiType,ambiSize, aR ,aG, aB) 
        end 
    else 
        for index, marker in ipairs(entryMarkers) do 
            if (allAmbiMarkers[i] and isElement(allAmbiMarkers[i])) then 
                destroyElement(allAmbiMarkers[i]) 
            end 
        end 
    end 
end 
addCommandHandler("ambilight",handleAmbilight) 

  
  [9] = {-1923.2001953125,454.2998046875,0.30000001192093}, -- REMOVE THIS COMMA 
} 
  

[Tails]

A comma at the end is never an issue .

Even if I remove the v[1],v[2],v[3] from the line, it's still giving the same error, so it has to be the .

Edit:

Had a typo on line 16: ambiMarkersA should be ambiMarkers. However, still didn't fix the problem.

[Noki]

Check your second for loop. for index, marker in ipairs(entryMarkers) do. Yet, you're checking for the value of i in allAmbiMarkers on line 21. That was my bad from my last post, sorry! I edited my post.

[Tails]

Okay, I've fixed that. The problem on line 17 still remains however, "Loading script failed: unexpected symbol near '['"

local allAmbiMarkers[i] = createMarker(v[1], v[2], v[3], ambiType,ambiSize, aR ,aG, aB) 

Been trying to fix it for hours now, haha

[Dealman]

allAmbiMarkers is defined at the top of your script, yet in the for loop you're trying to define it as a local variable. Try and change it to;

allAmbiMarkers[i] = createMarker(v[1], v[2], v[3], ambiType,ambiSize, aR ,aG, aB) 

[Addlibs]

Just to correct a misconception:

allAmbiMarkers is defined at the top of your script, yet in the for loop you're trying to define it as a local variable.

It's not a matter of whether it is defined local or global, it's just that you can't set indexes on tables with local. Tables work slightly different to normal variables - you can make the whole table local ( i.e. local aTable = {} ) but you can't make a specific variable local ( i.e. you'll get an error when attempting to use local aTable[1] = "Hello World" )

[Tails]

Thank you, Dealman. And MrTasty for clarifying.

I appreciate it!