feat: Firearm QOL stuff #327

Banalny-Banan · 2024-12-23T13:52:56Z

Description

Describe the changes
Things that got left out from #255

What is the current behavior? (You can also link to an open issue here)
New magazine API has sacrificed dev convenience

What is the new behavior? (if this is a feature change)
Ammo & MaxAmmo setters are back, TryGetModule methods added, AddPreference methods made static

Does this PR introduce a breaking change? (What changes might users need to make in their application due to this PR?)
It does, with making Preferences methods static.

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

Other

Still requires more testing

VALERA771 · 2024-12-23T17:54:38Z

EXILED/Exiled.API/Features/Items/Firearm.cs

+        /// <param name="players">The <see cref="IEnumerable{T}"/> of <see cref="Player"/> of which must be added.</param>
+        /// <param name="preference">The <see cref="KeyValuePair{TKey, TValue}"/> of <see cref="Enums.FirearmType"/> and <see cref="AttachmentIdentifier"/>[] to add.</param>
+        public static void AddPreference(IEnumerable<Player> players, KeyValuePair<FirearmType, AttachmentIdentifier[]> preference)
+        {
+            foreach (Player player in players)
+                AddPreference(player, preference.Key, preference.Value);
+        }
+
+        /// <summary>
+        /// Adds or replaces an existing preference to the <see cref="PlayerPreferences"/>.
+        /// </summary>
+        /// <param name="players">The <see cref="IEnumerable{T}"/> of <see cref="Player"/> of which must be added.</param>
+        /// <param name="preference">The <see cref="Dictionary{TKey, TValue}"/> of <see cref="Enums.FirearmType"/> and <see cref="AttachmentIdentifier"/>[] to add.</param>
+        public static void AddPreference(IEnumerable<Player> players, Dictionary<FirearmType, AttachmentIdentifier[]> preference)
+        {
+            foreach ((Player player, KeyValuePair<FirearmType, AttachmentIdentifier[]> kvp) in players.SelectMany(player => preference.Select(kvp => (player, kvp))))
+                AddPreference(player, kvp);
+        }
+
+        /// <summary>
+        /// Removes a preference from the <see cref="PlayerPreferences"/> if it already exists.
+        /// </summary>
+        /// <param name="player">The <see cref="Player"/> of which must be removed.</param>
+        /// <param name="type">The <see cref="Enums.FirearmType"/> to remove.</param>
+        public static void RemovePreference(Player player, FirearmType type)
+        {
+            foreach (KeyValuePair<Player, Dictionary<FirearmType, AttachmentIdentifier[]>> kvp in PlayerPreferences)
+            {
+                if (kvp.Key != player)
+                    continue;
+
+                if (AttachmentsServerHandler.PlayerPreferences.TryGetValue(player.ReferenceHub, out Dictionary<ItemType, uint> dictionary))
+                    dictionary[type.GetItemType()] = type.GetBaseCode();
+            }
+        }
+
+        /// <summary>
+        /// Removes a preference from the <see cref="PlayerPreferences"/> if it already exists.
+        /// </summary>
+        /// <param name="players">The <see cref="IEnumerable{T}"/> of <see cref="Player"/> of which must be removed.</param>
+        /// <param name="type">The <see cref="Enums.FirearmType"/> to remove.</param>
+        public static void RemovePreference(IEnumerable<Player> players, FirearmType type)
+        {
+            foreach (Player player in players)
+                RemovePreference(player, type);
+        }
+
+        /// <summary>
+        /// Removes a preference from the <see cref="PlayerPreferences"/> if it already exists.
+        /// </summary>
+        /// <param name="player">The <see cref="Player"/> of which must be removed.</param>
+        /// <param name="types">The <see cref="IEnumerable{T}"/> of <see cref="Enums.FirearmType"/> to remove.</param>
+        public static void RemovePreference(Player player, IEnumerable<FirearmType> types)
+        {
+            foreach (FirearmType itemType in types)
+                RemovePreference(player, itemType);
+        }
+
+        /// <summary>
+        /// Removes a preference from the <see cref="PlayerPreferences"/> if it already exists.
+        /// </summary>
+        /// <param name="players">The <see cref="IEnumerable{T}"/> of <see cref="Player"/> of which must be removed.</param>
+        /// <param name="types">The <see cref="IEnumerable{T}"/> of <see cref="Enums.FirearmType"/> to remove.</param>
+        public static void RemovePreference(IEnumerable<Player> players, IEnumerable<FirearmType> types)
+        {
+            foreach ((Player player, FirearmType firearmType) in players.SelectMany(player => types.Select(itemType => (player, itemType))))
+                RemovePreference(player, firearmType);
+        }
+
+        /// <summary>
+        /// Clears all the existing preferences from <see cref="PlayerPreferences"/>.
+        /// </summary>
+        /// <param name="player">The <see cref="Player"/> of which must be cleared.</param>
+        public static void ClearPreferences(Player player)
+        {
+            if (AttachmentsServerHandler.PlayerPreferences.TryGetValue(player.ReferenceHub, out Dictionary<ItemType, uint> dictionary))
+            {
+                foreach (KeyValuePair<ItemType, uint> kvp in dictionary)
+                    dictionary[kvp.Key] = kvp.Key.GetFirearmType().GetBaseCode();
+            }
+        }
+
+        /// <summary>
+        /// Clears all the existing preferences from <see cref="PlayerPreferences"/>.
+        /// </summary>
+        /// <param name="players">The <see cref="IEnumerable{T}"/> of <see cref="Player"/> of which must be cleared.</param>
+        public static void ClearPreferences(IEnumerable<Player> players)
+        {
+            foreach (Player player in players)
+                ClearPreferences(player);
+        }
+
+        /// <summary>
+        /// Clears all the existing preferences from <see cref="PlayerPreferences"/>.
+        /// </summary>
+        public static void ClearPreferences()
+        {
+            foreach (Player player in Player.List)
+                ClearPreferences(player);
+        }


Very big breaking change

Still smaller than renaming Ammo to TotalAmmo

Very big breaking change

still valid, non-static Preference has -inf meaning

VALERA771 · 2024-12-23T17:55:19Z

EXILED/Exiled.API/Features/Items/Firearm.cs

+        /// Gets or sets the maximum amount of ammo in the firearm.
+        /// </summary>
+        [Obsolete("Use MaxAmmo instead.")]
+        public int TotalMaxAmmo


Why we need to rename it?

It was renamed to TotalMaxAmmo in 9.0.1 for no practical reason, so I renamed it back and added backward compatibility

This is so people who haven't yet rewrote their plugins to use TotalMaxAmmo and TotalAmmo won't have any problems

VALERA771 · 2024-12-23T17:55:25Z

EXILED/Exiled.API/Features/Items/Firearm.cs

+        /// Gets or sets the amount of ammo in the firearm.
+        /// </summary>
+        [Obsolete("Use Ammo instead.")]
+        public int TotalAmmo


Same as above

IRacle1 · 2024-12-24T13:40:45Z

Okay, i see youre still burning with the idea of writing a union setter for ammos.
Firstly what i want to ask, do you have other arguments to your code, except backward compatibility?

Bcz in my opinion, that code in current realization is very compex and unstable, like setting all ammos to 0 before main code, if/else to different barrel components and processing barrel action itself. by unstable i mean you use too many tools for such a task, and logically, then from any custom patch the logic can get pretty weird

Why shotgun is pumped when i set Ammo to any value?
I patch ammo setters to make my custom ammo logic, why there a 2 calls for setting ammo from primary magazine?

Don't think of these as actual flaws ofc, but rather examples of what could potentially go wrong

I understand we want to make dev life easier, but actually a method with such complex logic can backfire on them altogether.

But still, I was wrong in terms of it being unrealistic to do generic Ammo setter, you could very well make a new Ammo setter that depends on abstractions in the right magazine classes, (most simple case like internal virtual SetAmmo method in every magazine class, and then some math in main ammo setter to call it properly, there are infinity ways to organize it), without multiple calls and reload methods - and I'd have no actual complaints about it

IRacle1 · 2024-12-24T13:49:25Z

The naming issue is simpler, we already have Barrel... and Magazine... ammo setters/getters, but if youll keep old name(just Ammo or MaxAmmo), there will be a little misunderstanding, like ammo what? so I clarified the naming and added Total to make it clearer to people. Plus, I wanted to make people aware that what they used to do with ammo doesn't work like it was. So that they could at least read the documentation and rewrite their code as they really need it
Even if we generalize Ammo setter, still people should understand what it actually does, like it is much higher level than simple ammo related stuff
I think that's reason enough to rename the method and broke plugins😔

IRacle1 · 2024-12-24T13:51:06Z

@VALERA771 @BoltonDev @Misaka-ZeroTwo @louis1706 @Misfiy I would also like to hear your opinion on this, its actually an important thing

VALERA771 · 2024-12-24T14:01:25Z

I agree with IRacle. By changing names we force devs to rewrite their stuff/read docs on new methods/see how they're working. Also I strongly disagree with changing preference methods to static now. Such breaking changes (changing stuff that is working) are not good for stable releases. And current ammo setter looks too much complicated. Setting ammo to 0, then setting it to actual required value. At least it can be done easily with using BarrelAmmo and MagazineAmmo

IRacle1 · 2024-12-24T14:10:16Z

I disagree about the preference methods, non static prefs are pure nonsense that made 0 sense when used, I agree with that change

Banalny-Banan · 2024-12-24T15:29:05Z

Okay, i see youre still burning with the idea of writing a union setter for ammos.
Firstly what i want to ask, do you have other arguments to your code, except backward compatibility?

Bcz in my opinion, that code in current realization is very compex and unstable, like setting all ammos to 0 before main code, if/else to different barrel components and processing barrel action itself. by unstable i mean you use too many tools for such a task, and logically, then from any custom patch the logic can get pretty weird

Why shotgun is pumped when i set Ammo to any value?
I patch ammo setters to make my custom ammo logic, why there a 2 calls for setting ammo from primary magazine?

Don't think of these as actual flaws ofc, but rather examples of what could potentially go wrong

I understand we want to make dev life easier, but actually a method with such complex logic can backfire on them altogether.

But still, I was wrong in terms of it being unrealistic to do generic Ammo setter, you could very well make a new Ammo setter that depends on abstractions in the right magazine classes, (most simple case like internal virtual SetAmmo method in every magazine class, and then some math in main ammo setter to call it properly, there are infinity ways to organize it), without multiple calls and reload methods - and I'd have no actual complaints about it

As an example on my server I have a weapon that players can't reload, but it slowly starts reloading automatically after not being fired for some time. If there are 2 different setters I will have to manage the distribution myself, instead of exiled doing it internally.

Another example: I want to empty the firearm. With the Ammo setter I can do it in one line with firearm.Ammo = 0. With 2 setters I have to set both. And if do it frequently in multiple places it starts to go against DRY.

Example 3: I have a weapon that gets ammo when it deals damage. I can do it with firearm.Ammo += amount. With two setters I will have to manually check if there is no ammo in the barrel, and if so add it there, and then add remaining to the primary mag. So basically replicate the whole setter just to add ammo to the firearm.

I can go on and on with all the examples, but I think you get the idea.

Banalny-Banan · 2024-12-24T15:33:34Z

what they used to do with ammo doesn't work like it was

Except for shotguns that get auto-pumped to avoid desync, it does. And if I find a way to make it work the same it did for shotguns, it will be a 1 to 1 replacement.

And I believe that we should work on solving the problem, instead of saying "nah, we tried" and making the devs carry out the 2-different-setters fuckery that NW created. So I encourage you to try to find a better solution instead of arguing how the one I made is not as perfect as it can be, because this is what Exiled exists for - simplify the interactions with base-game stuff.

Rysik5318 · 2024-12-31T12:31:46Z

The changes are fully vetted by me, I can say it's not a bad PR

Trevlouw · 2025-02-09T22:01:59Z

I personally like this PR, but why did you use var Banalny 😭

Added Ammo and MaxAmmo back with setters

72e9384

github-actions bot added the API label Dec 23, 2024

github-actions bot assigned Banalny-Banan Dec 23, 2024

Banalny-Banan added 3 commits December 23, 2024 15:56

Now using exiled magazines

a5888d1

Preferences methods are now static cause they can be

f3f8860

Aiming check now uses module interface instead of a derived type

85db023

Banalny-Banan marked this pull request as ready for review December 23, 2024 14:03

Banalny-Banan changed the title ~~Firearm QOL stuff~~ feat: Firearm QOL stuff Dec 23, 2024

Banalny-Banan added 2 commits December 23, 2024 16:18

backward compatibility for TotalAmmo & TotalMaxAmmo

fdde4de

sync fix

cf9aba7

VALERA771 reviewed Dec 23, 2024

View reviewed changes

more utility methods

e29f022

louis1706 added the breaking-changes label Jan 16, 2025

louis1706 changed the base branch from xmas-2024 to dev January 16, 2025 18:24

Merge branch 'dev' into firearm-ammosetter

cd731f6

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: Firearm QOL stuff #327

feat: Firearm QOL stuff #327

Banalny-Banan commented Dec 23, 2024 •

edited

Loading

VALERA771 Dec 23, 2024

Banalny-Banan Dec 23, 2024

IRacle1 Dec 24, 2024

VALERA771 Dec 23, 2024

Banalny-Banan Dec 23, 2024

Banalny-Banan Dec 23, 2024

VALERA771 Dec 23, 2024

IRacle1 commented Dec 24, 2024

IRacle1 commented Dec 24, 2024

IRacle1 commented Dec 24, 2024

VALERA771 commented Dec 24, 2024 •

edited

Loading

IRacle1 commented Dec 24, 2024

Banalny-Banan commented Dec 24, 2024

Banalny-Banan commented Dec 24, 2024 •

edited

Loading

Rysik5318 commented Dec 31, 2024

Trevlouw commented Feb 9, 2025

feat: Firearm QOL stuff #327

Are you sure you want to change the base?

feat: Firearm QOL stuff #327

Conversation

Banalny-Banan commented Dec 23, 2024 • edited Loading

Description

Types of changes

Submission checklist

Other

VALERA771 Dec 23, 2024

Choose a reason for hiding this comment

Banalny-Banan Dec 23, 2024

Choose a reason for hiding this comment

IRacle1 Dec 24, 2024

Choose a reason for hiding this comment

VALERA771 Dec 23, 2024

Choose a reason for hiding this comment

Banalny-Banan Dec 23, 2024

Choose a reason for hiding this comment

Banalny-Banan Dec 23, 2024

Choose a reason for hiding this comment

VALERA771 Dec 23, 2024

Choose a reason for hiding this comment

IRacle1 commented Dec 24, 2024

IRacle1 commented Dec 24, 2024

IRacle1 commented Dec 24, 2024

VALERA771 commented Dec 24, 2024 • edited Loading

IRacle1 commented Dec 24, 2024

Banalny-Banan commented Dec 24, 2024

Banalny-Banan commented Dec 24, 2024 • edited Loading

Rysik5318 commented Dec 31, 2024

Trevlouw commented Feb 9, 2025

Banalny-Banan commented Dec 23, 2024 •

edited

Loading

VALERA771 commented Dec 24, 2024 •

edited

Loading

Banalny-Banan commented Dec 24, 2024 •

edited

Loading