Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

added resource: Chemical Ladder v2.0 (ladders) #547

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

ChemicalCreations
Copy link

@ChemicalCreations ChemicalCreations commented Sep 3, 2024

Adds a resource that allows you to climb various ladders across SA map. Default ladders can be added or disabled via exported functions. This also allows you to add ladders to elements based on modelID (vehicle and object supported).
https://youtu.be/K3JhbzGyQto?si=84lkPPM4I3R189mh

@jlillis
Copy link
Contributor

jlillis commented Sep 4, 2024

Looks pretty cool. This is going to take a while to code review, but the first step would be to look at what the linter has flagged and address those issues.

Just to confirm on the record - are you the original author of the resource?

@ChemicalCreations
Copy link
Author

For the record yes.

@Fernando-A-Rocha
Copy link
Contributor

Awesome scripts! Looking forward to this

@Nico8340
Copy link
Contributor

Any updates on this?

@AlexTMjugador
Copy link
Member

Any updates on this?

[...] the first step would be to look at what the linter has flagged and address those issues.

This step was not done as far as I can see, so no, there are no updates.

local climbers = {}
local climbs = {}

local anim = anims
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is unnecessary, because the anims table is a global variable, so it is accessible in this script from the global namespace

Suggested change
local anim = anims

Comment on lines +79 to +100
local minus
do
local abs = math.abs
-- calculate and return the distance unit2
-- needs to move to reach unit1
function minus(unit1, unit2)
local phi = abs(unit2-unit1) % 360
local sign = 1
-- used to calculate sign
if not ((unit1-unit2 >= 0 and unit1-unit2 <= 180)
or (unit1-unit2 <= -180 and unit1-unit2 >= -360)) then
sign = -1
end
if phi > 180 then
result = 360-phi
else
result = phi
end

return result*sign
end
end
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This can be implemented much more simply, how about this solution?

Suggested change
local minus
do
local abs = math.abs
-- calculate and return the distance unit2
-- needs to move to reach unit1
function minus(unit1, unit2)
local phi = abs(unit2-unit1) % 360
local sign = 1
-- used to calculate sign
if not ((unit1-unit2 >= 0 and unit1-unit2 <= 180)
or (unit1-unit2 <= -180 and unit1-unit2 >= -360)) then
sign = -1
end
if phi > 180 then
result = 360-phi
else
result = phi
end
return result*sign
end
end
function minus(unit1, unit2)
local diff = unit2 - unit1
local diffNormalized = diff % 360
return diffNormalized > 180 and diffNormalized - 360 or diffNormalized
end

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good, don't see why not as long as it works.

function getPedsOnLadder(surface)
local peds = {}
for ped, data in pairs(climbers) do
if surface==nil then
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
if surface==nil then
if not surface then

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wouldn't recommend that. nil and false aren't the same, they are done like that for a reason.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wouldn't recommend that. nil and false aren't the same, they are done like that for a reason.

In this case, it will not change how it works, and the change I suggested is a better solution

function isPedLadderClimbingEnabled(ped)
local eType = ped and isElement(ped) and getElementType(ped)
assert(eType=="ped" or eType=="player") -- isPed
if climbers[ped]==false then return false end
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
if climbers[ped]==false then return false end
if not climbers[ped] then return false end

local dist = ((tx-sx)^2+(ty-sy)^2+(tz-sz)^2)^.5+10
if ((tx-px)^2+(ty-py)^2+(tz-pz)^2)^.5<dist or ((px-sx)^2+(py-sy)^2+(pz-sz)^2)^.5<dist then
local dist, p = getPointDistanceFromLadder(px, py, pz, sx, sy, sz, tx, ty, tz)
if (climbD==nil or dist<climbD) then
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
if (climbD==nil or dist<climbD) then
if (not climbD or dist<climbD) then

local eType = ped and isElement(ped) and getElementType(ped)
assert(eType=="ped" or eType=="player") -- isPed
if climbers[ped]==false then return false end
return climbers[ped]==nil or (climbers[ped] and true)
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do realize this is supposed to return true when it's nil.

return false
end

do
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why is this do block necessary?

function removeLadder(surface, ladder)
local surfaceData = climbs[surface]
assert(surfaceData)
if ladder==nil then
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
if ladder==nil then
if not ladder then

Comment on lines +249 to +253
addDebugHook("preFunction", function(sourceResource, functionName, isAllowedByACL, luaFilename, luaLineNumber, ped) -- just to deal with freeroam
if sourceResource~=resource and climbers[ped] then
return "skip"
end
end, {"setPedAnimation"})
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not a good idea

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agreed

Comment on lines +346 to +347
addDebugHook("postFunction", function(sourceResource, functionName, isAllowedByACL, luaFilename, luaLineNumber, ...)
end, {"createVehicle", "Vehicle", "createObject", "Object"})
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not used for anything

local eType = ped and isElement(ped) and getElementType(ped)
assert(eType=="ped" or eType=="player") -- isPed
if climbers[ped]==false then return false end
return climbers[ped]==nil or (climbers[ped] and true)
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do realize this is supposed to return true when it's nil.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants