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 auto shotgun swep #16

Merged
merged 14 commits into from
Dec 20, 2023
Merged

Add auto shotgun swep #16

merged 14 commits into from
Dec 20, 2023

Conversation

yousifhub
Copy link
Contributor

adding a shotgun that makes the normal hl2 shotgun faster no cocking and faster reload and another version for admins to have fun and does more damage than the non-admin one

@yousifhub yousifhub requested a review from a team as a code owner September 12, 2023 19:59
Copy link

@yogwoggf yogwoggf left a comment

Choose a reason for hiding this comment

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

I think the code could greatly benefit from extracting constant values you use in the attack functions to constant variables grouped at some part of the file (preferably the top)

Also, is it necessary for the cat picture to have a material bound to it?

lua/weapons/weapon_Auto_Shotgun.lua Outdated Show resolved Hide resolved
lua/weapons/weapon_Auto_Shotgun.lua Outdated Show resolved Hide resolved
lua/weapons/weapon_Auto_Shotgun.lua Outdated Show resolved Hide resolved
lua/weapons/weapon_Auto_Shotgun.lua Outdated Show resolved Hide resolved
lua/weapons/weapon_Auto_Shotgun.lua Outdated Show resolved Hide resolved
lua/weapons/weapon_Auto_Shotgun.lua Outdated Show resolved Hide resolved
lua/weapons/weapon_Auto_Shotgun.lua Outdated Show resolved Hide resolved
lua/weapons/weapon_Auto_Shotgun.lua Outdated Show resolved Hide resolved
lua/weapons/weapon_Auto_Shotgun.lua Outdated Show resolved Hide resolved
@yogwoggf
Copy link

Forgot to mention in the original review but I think you should rename the weapon file to weapon_autoshotgun instead of weapon_Auto_Shotgun since traditionally weapon names are all lowercase.

@yogwoggf
Copy link

LGTM. The PR will be merged once someone can do QA and it passes without any blockers.

Copy link
Member

@Derpius Derpius left a comment

Choose a reason for hiding this comment

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

It doesn't look like you've added the resources to be downloaded by clients using https://wiki.facepunch.com/gmod/resource

lua/weapons/weapon_autoshotgun.lua Outdated Show resolved Hide resolved
@yousifhub
Copy link
Contributor Author

Not really sure if SWEP.ShootSound = Sound("weapons/xm1014/xm1014-1.wav") people without css will hear but
their problem since people mostly got css since its mostly used in servers

@Derpius
Copy link
Member

Derpius commented Oct 11, 2023

@yousifhub can you please add the unrelated changes to separate feature branches and PRs.

This should just be for the auto shotgun, and not everything you’re working on. Otherwise we’re forced to merge either everything or nothing.

Also, the extra hud isn’t a swep, so it should be in a different repository entirely

@yousifhub yousifhub requested a review from yogwoggf December 15, 2023 21:22
Copy link
Member

@Derpius Derpius left a comment

Choose a reason for hiding this comment

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

Looks fine now bar the seemingly unused shoot and reload sounds. Just needs someone to QA (@yogwoggf you have lots of free time and not much experience doing QAs :trollface:)

Copy link
Member

Choose a reason for hiding this comment

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

If these files are unused can they be removed?

@yogwoggf
Copy link

Will probably do the QA on the weekend, although, @yousifhub - you do have merge conflicts because you accidently removed el patito from the authors list.

@Derpius
Copy link
Member

Derpius commented Dec 16, 2023

@yogwoggf merge conflicts resolved

Copy link

@yogwoggf yogwoggf left a comment

Choose a reason for hiding this comment

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

QA Result: Failed ❌

Blockers ❌

  • When re-equipping the weapon after spawning it from the weapons tab in the spawnmenu, the swep will error locally and cause a lua error. ❌
    • The error: [sweps] addons/sweps/lua/weapons/weapon_autoshotgun.lua:49: attempt to call method 'GiveAmmo' (a nil value) 1. unknown - addons/sweps/lua/weapons/weapon_autoshotgun.lua:49 (x12)
  • Similar to the first blocker, the entire logic for the giving ammo on the first deploy is broken and doesn't actually run when spawned. ❌
    • Furthermore, it's shadowed by the original SWEP logic that is being used anyways (SWEP.Primary.DefaultClip = 30)
    • I would remove it entirely
  • When the clip is empty, the secondary attack (right mouse button) spams the empty click sound each frame, resulting in a discordant sound. ❌
    • I'm arguing this is a blocker as it can easily cause someone to get startled if they're using headphones

Warnings ⚠️

  • Kill icon is not specified ⚠️
    • Causes a warning in the console
    • In my opinion, I think it could just be set to the normal shotgun icon
  • There is a "cat.vtf" and a "cat.vmt" but also a "cat.jpg" in materials/ ⚠️
    • Choose one or the other, it's unnecessary to have different formats, especially as GMod has first-class support for jpegs.
    • Your code also flip flops between the vmt and the jpg.

Passed ✅

  • Primary fire ✅
    • Fires ordinary shotgun bullets
    • Takes 1 ammo out of the clip
    • Plays fire sound correctly
  • Secondary fire ✅
    • Fires three shotgun bullets
    • Takes 3 ammos out of the clip
    • Plays fire sound correctly
    • Won't fire if less than 3 ammos in clip
  • HUD ✅
    • Icon appears correctly in spawnmenu
    • Icon appears decently correct in weapon selection (stretched)
  • Appearance ✅
    • World model is a shotgun
    • View model is the normal shotgun
    • FoV is correct
    • Hold type is shotgun, so correct

@yousifhub yousifhub requested a review from yogwoggf December 18, 2023 15:14
@yogwoggf
Copy link

Re-QA Result: Passed with Warnings ✅

Warnings ⚠️

  • There are still the 3 redundant files of the icon. ⚠️

Passed ✅

  • When re-equipping the weapon after spawning it from the weapons tab in the spawnmenu, the swep will error locally and cause a lua error. ✅
  • The entire logic for the giving ammo on the first deploy is broken and doesn't actually run when spawned. ✅
  • When the clip is empty, the secondary attack (right mouse button) spams the empty click sound each frame, resulting in a discordant sound. ✅

@Derpius Derpius merged commit 7356b33 into TAServers:master Dec 20, 2023
1 check passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Development

Successfully merging this pull request may close these issues.

4 participants