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 steel armor clothing mod #34593

Merged
merged 26 commits into from
Nov 22, 2019
Merged

Add steel armor clothing mod #34593

merged 26 commits into from
Nov 22, 2019

Conversation

snipercup
Copy link
Contributor

@snipercup snipercup commented Oct 9, 2019

Summary

SUMMARY: Content "Adds steel armor clothing mod"

Purpose of change

Follow up on #34571.

Describe the solution

You can now pad your clothes with steel_armor and a tailors kit.

Describe alternatives you've considered

I used the same stats as leather_padded but i feel like there should be a difference. maybe have leather_padded add warmth?

Additional context

image

image

@tenmillimaster
Copy link
Member

tenmillimaster commented Oct 9, 2019

Should the steel padded armor variants get obsoleted then?

Also I'd call it 'lined', not 'padded'.

@snipercup
Copy link
Contributor Author

Can you give me an example of a steel padded armor variant?

@ZhilkinSerg ZhilkinSerg added [JSON] Changes (can be) made in JSON Crafting / Construction / Recipes Includes: Uncrafting / Disassembling Items / Item Actions / Item Qualities Items and how they work and interact labels Oct 9, 2019
@ghost
Copy link

ghost commented Oct 9, 2019

Can you give me an example of a steel padded armor variant?

Armored leather jacket, armored leather vest, pair of armored fingerless gloves, plated leather armor, pair of armored gauntlets, pair of armored boots.

@Rivet-the-Zombie
Copy link
Member

pair of armored gauntlets, pair of armored boots

These two aren't just 'a bit of clothing with added metal' but are rather meant to be armor in their own right - the non-metal bits just serve to keep the metal pieces together.

@snipercup
Copy link
Contributor Author

I'm scratching my head over the plated leather armor but I think i'll come up with something and hopefully I can extend that to the other items you mentioned.

Copy link
Member

@kevingranade kevingranade left a comment

Choose a reason for hiding this comment

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

The problem with the clothing modification system is it doesn't have any meaningful limitations on which items can be modified. Steel is particularly problematic due to it's high weight.

I'm not particularly inclined to add more to this system at all, and to have any chance of this being ok, it needs to have a mechanism for limiting what clothes you can apply it to.

@snipercup
Copy link
Contributor Author

The problem with the clothing modification system is it doesn't have any meaningful limitations on which items can be modified. Steel is particularly problematic due to it's high weight.

I'm not particularly inclined to add more to this system at all, and to have any chance of this being ok, it needs to have a mechanism for limiting what clothes you can apply it to.

Yeah I thought it was a bit strange when I applied steel plating to my socks. I don't feel confident in my C++ skills yet but I did think about looking at the code for repairing. There is a filter applied there somewhere. I'll leave the PR open for 1 day and then close it unless someone is interested in helping develop this idea.

@Davi-DeGanne
Copy link
Contributor

I am willing to take a stab at the C++ legwork, if you come up with a JSON flag/property you want to use to differentiate it.

@snipercup
Copy link
Contributor Author

I am willing to take a stab at the C++ legwork, if you come up with a JSON flag/property you want to use to differentiate it.

Thanks for wanting to help. The first thing that comes up is a whitelist flag that can be applied to any clothing that you want to be able to be padded. For example when entering an item's flags, you add can_pad_steel, can_pad_leather, can_line_fur

Alternatively you could check an itemgroup and items in that group can be padded

@snipercup snipercup closed this Oct 10, 2019
@snipercup snipercup deleted the add-steel-armor-clothing-mod branch October 10, 2019 18:01
@snipercup snipercup restored the add-steel-armor-clothing-mod branch October 10, 2019 18:02
@snipercup snipercup reopened this Oct 10, 2019
@Davi-DeGanne
Copy link
Contributor

Davi-DeGanne commented Oct 10, 2019

How about using something existing, like material_thickness or STURDY?

@snipercup
Copy link
Contributor Author

I will look into it.

@snipercup
Copy link
Contributor Author

I think a pretty good selection is items that have material: leather, flag: sturdy and material_thickness: >3 and only Covers: Legs and/or torso (and arms). I can probably write a query to see what items come up but I'm pretty confident that these items are a good selection to start with. This would also exclude the items LaVeyanFiend mentions except for the plated leather armor. I can think of some options for the plated leather armor but i'll get to that later.

@Davi-DeGanne
Copy link
Contributor

Just thought of a more elegant way of doing it, sort of an extension of one of your ideas: We'll have an optional JSON property valid_items for clothing mods which, if present, restricts the clothing mod such that it can only be applied to the listed items.

@kevingranade Is this an acceptable solution?

@kevingranade
Copy link
Member

kevingranade commented Oct 12, 2019

That's better than the heuristic approach.
Generally you'd flip it around and put it on the clothing, i.e. "valid_mods": [ "steel_insert" ],

@Davi-DeGanne
Copy link
Contributor

What's the justification for doing it that way instead? I kind of prefer my approach, as it doesn't invalidate the existing four clothing mods until someone goes through all the existing armor/clothing and adds valid_mods entries where applicable.

@kevingranade
Copy link
Member

Neither one invalidates anything unless you code it to do so. Treat it as a whitelist and everything is fine.

As for the reason, it's far easier to maintain tags on items than it is to maintain centralized lists of items. Similarly it handles new items added by mods more seamlessly.

@snipercup
Copy link
Contributor Author

@kevingranade Can this PR be merged now that #34705 is merged? I will make another PR that will put this steel clothing mod to use.

@cainiaowu
Copy link
Contributor

mostly lack of maintainers time, you may need to ask directly on discord or it may get missed.

@snipercup
Copy link
Contributor Author

@kevingranade Could you take another look at this PR?

@kevingranade
Copy link
Member

It needs to be marked restricted as discussed, and have at least one article of clothing with it added to the modification whitelist.

@snipercup
Copy link
Contributor Author

Steel padded clothing mod is now restricted.
I added valid_mods to some clothing items.
Steel padded can now be applied:
image

Cannot be applied to other clothing:
image

Please review/merge

@kevingranade kevingranade merged commit 2eef78f into CleverRaven:master Nov 22, 2019
@snipercup snipercup deleted the add-steel-armor-clothing-mod branch November 22, 2019 23:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Crafting / Construction / Recipes Includes: Uncrafting / Disassembling Items / Item Actions / Item Qualities Items and how they work and interact [JSON] Changes (can be) made in JSON
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants