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

Add Magic Sword #18

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

Add Magic Sword #18

wants to merge 2 commits into from

Conversation

yousifhub
Copy link
Contributor

@yousifhub yousifhub commented Dec 24, 2023

Changes

Added a sword that lets you leap heal and --kill--

Impact

Fun weapon to mess with

Testing

Leaping attacking the icons are correct the functions word as intended

Helpful Links

Discord

@yousifhub yousifhub requested a review from a team as a code owner December 24, 2023 08:22
SWEP.Instructions =
"Left click to attack | Right click to leap | Reload to heal | While holding the sword you don't take fall damage"
SWEP.Purpose = "To Become a cool swordsman that can fly"
SWEP.Contact = "Don't Contact me i dont like people"
Copy link
Member

Choose a reason for hiding this comment

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

You can just omit the contact property

Copy link
Contributor Author

Choose a reason for hiding this comment

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

check

SWEP.Category = TASWeapons.Category
SWEP.Spawnable = true
SWEP.AdminOnly = false
SWEP.BounceWeaponIcon = false
Copy link
Member

Choose a reason for hiding this comment

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

What was the reason for explicitly disabling this? It looks like most weapons leave it enabled

Copy link
Contributor Author

Choose a reason for hiding this comment

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

unchanged

Comment on lines +27 to +28
SWEP.ViewModel = "models/weapons/v_dmascus.mdl"
SWEP.WorldModel = "models/weapons/w_damascus_sword.mdl"
Copy link
Member

Choose a reason for hiding this comment

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

I think this model was part of M9K specialties which we removed (for a number of reasons, including numerous client-side Lua errors).

We can either bundle these assets into this repo (not ideal) or I'll try to get the TF2 sword viewmodel working at some point in the next couple days

Copy link
Contributor Author

Choose a reason for hiding this comment

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

check

Comment on lines +32 to +33
SWEP.HitAnim = ACT_VM_HITCENTER
SWEP.MissAnim = ACT_VM_MISSCENTER
Copy link
Member

Choose a reason for hiding this comment

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

These properties don't seem to exist on the SWEP struct: https://wiki.facepunch.com/gmod/Structures/SWEP

Comment on lines +44 to +50
SWEP.PrimaryDamage = 50
SWEP.PrimaryDelay = 0.38
SWEP.PrimaryRange = 70

SWEP.HitSound = Sound("weapons/blades/swordchop.mp3")
SWEP.SwingSound = Sound("weapons/blades/woosh.mp3")
SWEP.HitWallSound = Sound("weapons/blades/hitwall.mp3")
Copy link
Member

Choose a reason for hiding this comment

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

Similar to above, these properties are not part of the SWEP struct.

If you need these variables defined for code below, then you can just do local PRIMARY_DAMAGE = 50 etc.

⬆️ SHOUTY_SNAKE_CASE is what we generally use for constants in TAS repos


if trace.Hit then
if trace.Entity:IsPlayer() or trace.Entity:IsNPC() then
self:SetNextPrimaryFire(CurTime() + self.PrimaryDelay)
Copy link
Member

Choose a reason for hiding this comment

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

You're already setting the next primary fire time on line 76 above

Comment on lines +69 to +74
local tr = {}
tr.start = self:GetOwner():GetShootPos()
tr.endpos = self:GetOwner():GetShootPos() + (self:GetOwner():GetAimVector() * self.PrimaryRange)
tr.filter = self:GetOwner()
tr.mask = MASK_SHOT_HULL
local trace = util.TraceLine(tr)
Copy link
Member

Choose a reason for hiding this comment

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

nit: Can you rename the tr variable to traceData and trace to traceResult so this is a little clearer please?

You could also drop the tr variable entirely and just define the table in the call to TraceLine

function SWEP:SecondaryAttack()
self:SetNextSecondaryFire(CurTime() + 2)
local owner = self:GetOwner()
if IsValid(owner) and owner:IsOnGround() then
Copy link
Member

Choose a reason for hiding this comment

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

Given you're applying the velocity in the direction that the player is looking, it might be cool to allow dashing while mid-air too

Not a review comment, just a gameplay suggestion

Copy link
Contributor Author

Choose a reason for hiding this comment

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

i kept it on the ground to make it fair cause if added a double dash then that will be less fair i would say, i can try to make the dash be powerful on how long you hold the secondaryattack button

Copy link
Contributor Author

Choose a reason for hiding this comment

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

unchanged

local looking = owner:GetAimVector()
local velocity = looking * 1000

owner:SetVelocity(velocity)
Copy link
Member

Choose a reason for hiding this comment

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

Instead of overriding the player's velocity, it might be better to apply force instead to make the dash less jarring

end
end

hook.Add("EntityTakeDamage", "PreventFallDamage", function(target, dmginfo)
Copy link
Member

Choose a reason for hiding this comment

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

It looks like the GetFallDamage hook would be the better choice here

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: First Review - Markups
Development

Successfully merging this pull request may close these issues.

2 participants