Skip to content

feat: [😭] Firearm AmmoModules Api#288

Merged
BoltonDev merged 39 commits intoExMod-Team:devfrom
IRacle1:firearmhard
Dec 23, 2024
Merged

feat: [😭] Firearm AmmoModules Api#288
BoltonDev merged 39 commits intoExMod-Team:devfrom
IRacle1:firearmhard

Conversation

@IRacle1
Copy link
Copy Markdown
Member

@IRacle1 IRacle1 commented Dec 1, 2024

Description

Describe the changes

inspired by #255

I was thinking about more flexible logic for setting ammo. My first idea was to make better abstractions of Barrel and Primary magazines(that what i did in that pr firstly). I liked that idea, and that gave me a possibility to add more specific api for specific magazines (Rotate for revolvers, or bolt locking for automatic firearms as example). But there is 2 questions about ammo setters and for modules in general:

  • there was a discussion in 255 pr, taptap , for giving support to more than one primary/barrel magazines at ones. Ive thought about it later, and Ive decided in this case to move away from several magazines abstraction in favour of easier system of maximum 2 magazines (barrel and primary). But if adding custom modules turns to be real and possible, were gonna have to look at this again.
  • main question is an about ammo setter at all, do we generally need such complicated Ammo/MaxAmmo setters? When i finished with main part of api, and started thinking about total Ammo setter. And I really dont think an absolute/total ammo setter like before 14.0 is needed. This setter has an overly complex background and logic. Even if we position Exiled as a highlevel api, I still dont think its necessary to make a single setter for barrel and magazine ammos. But anyway that my honest shizo opinion says. Really wait for feedback😘

oh, and i added RotatingRevolver event😎

What is the current behavior? (You can also link to an open issue here)
😥

What is the new behavior? (if this is a feature change)
😎

Does this PR introduce a breaking change? (What changes might users need to make in their application due to this PR?)
😓

Other information:
🥹

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)
  • Documentations

Submission checklist

  • I have checked the project can be compiled
  • I have tested my changes and it worked as expected

Patches (if there are any changes related to Harmony patches)

  • I have checked no IL patching errors in the console

Main Axioms that i used for api logic, or tldr😎

  • Barrel and Normal magazines should be clearly segregated for the convenience of people
  • In those abstraction Ammo setters should be available, despite the lack of this abstraction in the basegame.
  • As i think now, the best way will to create BarrelAmmo MagazineAmmo full properties, and make TotalAmmo only with getter

@IRacle1
Copy link
Copy Markdown
Member Author

IRacle1 commented Dec 1, 2024

All ammoModules🥹

firearm: GunCOM15
 - barrel: AutomaticActionModule
 - primary: MagazineModule
firearm: GunCOM18
 - barrel: AutomaticActionModule
 - primary: MagazineModule
firearm: GunE11SR
 - barrel: AutomaticActionModule
 - primary: MagazineModule
firearm: GunCrossvec
 - barrel: AutomaticActionModule
 - primary: MagazineModule
firearm: GunFSP9
 - barrel: AutomaticActionModule
 - primary: MagazineModule
firearm: GunLogicer
 - barrel: AutomaticActionModule
 - primary: MagazineModule
firearm: GunRevolver
 - primary: CylinderAmmoModule
firearm: GunAK
 - barrel: AutomaticActionModule
 - primary: MagazineModule
firearm: GunShotgun
 - barrel: PumpActionModule
 - primary: MagazineModule
firearm: GunCom45
 - barrel: AutomaticActionModule
 - primary: MagazineModule
firearm: ParticleDisruptor
 - primary: MagazineModule
firearm: GunFRMG0
 - barrel: AutomaticActionModule
 - primary: MagazineModule
firearm: GunA7
 - barrel: AutomaticActionModule
 - primary: MagazineModule

@IRacle1
Copy link
Copy Markdown
Member Author

IRacle1 commented Dec 1, 2024

Anyway until we come to an agreement on that Ill leave it to the draft

@Misaka-ZeroTwo
Copy link
Copy Markdown
Collaborator

I like the idea of separating the barrel and the magazine.

It will be way too complex to handle them all in one place. We would deal with more edge cases and a much higher chance that something just doesn't work.

@VALERA771
Copy link
Copy Markdown

Lgtm. For some people may be too complex to use but game the game is also is not too easy to understand. So if people don't need to use a lot of classes to get a ammo and and if we don't have a 70 line ammo setter I agree

@VALERA771
Copy link
Copy Markdown

Also waiting for these guys opinions

Copy link
Copy Markdown

@louis1706 louis1706 left a comment

Choose a reason for hiding this comment

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

I am lazzy to finish just inlined everything than can be inlined

@VALERA771
Copy link
Copy Markdown

Is this PR is ready for reviews and tests? I think it's one the best one from other PRs which are fixing firearm properties

@IRacle1 IRacle1 marked this pull request as ready for review December 15, 2024 11:50
@IRacle1 IRacle1 changed the title [😭] Firearm AmmoModules Api feat: [😭] Firearm AmmoModules Api Dec 15, 2024
@VALERA771
Copy link
Copy Markdown

Some things (a lot) can be inlined. But generally looks good if you've tested magazine API

Copy link
Copy Markdown

@louis1706 louis1706 left a comment

Choose a reason for hiding this comment

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

It's all fine to merge when this will be completed (it's can be done after it's merge if needed)

the only important it's for AmmoDrain.cs file about comment on the GetInstructions method

@VALERA771 VALERA771 requested a review from BoltonDev December 20, 2024 17:10
IRacle1 and others added 8 commits December 22, 2024 14:20
Co-authored-by: Yamato <66829532+louis1706@users.noreply.github.com>
…icBarrelMagazine.cs

Co-authored-by: Yamato <66829532+louis1706@users.noreply.github.com>
…Magazine.cs

Co-authored-by: Yamato <66829532+louis1706@users.noreply.github.com>
…Magazine.cs

Co-authored-by: Yamato <66829532+louis1706@users.noreply.github.com>
…Magazine.cs

Co-authored-by: Yamato <66829532+louis1706@users.noreply.github.com>
@BoltonDev BoltonDev changed the base branch from scpsl14 to dev December 23, 2024 00:10
@BoltonDev BoltonDev merged commit 78bc42a into ExMod-Team:dev Dec 23, 2024
@Jacobson-H
Copy link
Copy Markdown

Epic 🧀

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.

6 participants