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

[Review request] Add support for pistols chambered in .45 ACP by introducing new model #40

Merged
merged 8 commits into from
Nov 26, 2023
Merged

Conversation

macinsight
Copy link
Collaborator

This PR relates to #39, adding support for differentiating calibers, specifically the .45 ACP caliber.

Signed-off-by: macinsight <gh@macinsight.net>
Signed-off-by: macinsight <gh@macinsight.net>
Signed-off-by: macinsight <gh@macinsight.net>
Signed-off-by: macinsight <gh@macinsight.net>
Signed-off-by: macinsight <gh@macinsight.net>
Signed-off-by: macinsight <gh@macinsight.net>
@macinsight
Copy link
Collaborator Author

@coavins It appears there's a slight issue (Check it yourself, changed the Glock 21 to the new Pistol_45acp model) but it's not jumping out at me. A second set of eyes would be appreciated.
Once this PR is merged, I'd like to update the docs to expand upon adding new models for the future.

Signed-off-by: macinsight <gh@macinsight.net>
@macinsight macinsight changed the title Add support for pistols chambered in .45 ACP by introducing new model [Review request] Add support for pistols chambered in .45 ACP by introducing new model Nov 25, 2023
Copy link
Owner

@coavins coavins left a comment

Choose a reason for hiding this comment

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

Looks good to me - don't forget to add the item names into the translation files (English only is fine). They are under coavinsfirearms/media/lua/shared/Translate/EN. The PistolReceiver_45acp for example should probably be called "Pistol Frame (45 acp)" or something like that.

When disassembling a Glock 21, the name ("Glock 21 Pistol") will be prepended to the name of the receiver, so you will get something like "Glock 21 Pistol Pistol Frame (45acp)" depending on what name you use for the receiver. This is unfortunate, but we definitely want to make sure the receiver is identifiable as a pistol frame in the event that the player finds it on its own, without the name prepended like that.

Maybe we could improve that concatenation to avoid duplicate terms in cases like this.

Don't forget also that you can re-use parts between different models, so you could re-use the same pistol receiver and simply have a 45 acp barrel, if you're so inclined. Then players could swap the caliber just by finding a new barrel. But personally I would err on the side of realism.

@macinsight macinsight mentioned this pull request Nov 25, 2023
Signed-off-by: macinsight <gh@macinsight.net>
@macinsight macinsight marked this pull request as ready for review November 25, 2023 20:51
@coavins coavins changed the base branch from master to develop November 26, 2023 04:28
@coavins coavins merged commit 49635ca into coavins:develop Nov 26, 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
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants