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

[RDY] Crafting Overhaul: Tailoring #20270

Merged
merged 59 commits into from Feb 23, 2017

Conversation

Projects
None yet
7 participants
@ZhilkinSerg
Copy link
Contributor

commented Feb 13, 2017

A couple of months ago I've created Crafting Overhaul: Tailoring mod (see http://smf.cataclysmdda.com/index.php?topic=13880.0).

With this mod enabled you won't need to reload your sewing tools with threads/fiber/sinews anymore when crafting textile items - just have it available in your possession (inventory or nearby).

There weren't too much hype in the forums, but still I want to move these changes into core.

P.S.: I hope @kevingranade hasn't changed his mind and will still vote "Yes". 🇧🇫

@ZhilkinSerg ZhilkinSerg changed the title [RDY] Crafting Overhaul: Tailoring Crafting Overhaul: Tailoring Feb 14, 2017

ZhilkinSerg added some commits Feb 14, 2017

@WizardOfOoo

This comment has been minimized.

Copy link
Contributor

commented Feb 14, 2017

This is a very good idea, and once it's perfected, it could be expanded to welding and the like... maybe even letting a welder take charges from a storage battery?

Did you resolve the repair/reinforce issue you had? Or do you still need to load the tool to do repairs?

@DangerNoodle

This comment has been minimized.

Copy link
Contributor

commented Feb 14, 2017

Interesting concept, but some flaws.

First, yarn is not included in the recipes, when it is a usable thread.
Second, this does not yet resolve the greater tedium of needing to load the tools to repair items, but I understand this will require more complex changes.
Third, any changes to Arcana and Magic Items or PK Rebalance mods will additionally warrant informing both mod authors of this change, as both are maintaining this mod in separate repositories.

@DangerNoodle

This comment has been minimized.

Copy link
Contributor

commented Feb 14, 2017

Lastly, running the changed recipes through the online web linting tool ( http://dev.narc.ro/cataclysm/format.html ) will help with current commit failure cause.

@ZhilkinSerg ZhilkinSerg changed the title Crafting Overhaul: Tailoring [WIP] Crafting Overhaul: Tailoring Feb 14, 2017

ZhilkinSerg added some commits Feb 14, 2017

@ZhilkinSerg

This comment has been minimized.

Copy link
Contributor Author

commented Feb 14, 2017

Thanks for the feedback! What a shame I forgot about yarn.

@Coolthulhu

This comment has been minimized.

Copy link
Contributor

commented Feb 14, 2017

You shouldn't directly copy+paste all the thread/sinew/fiber/yarn entries but should use the "using" syntax instead. Check out how is it done with "adhesive": it is defined in data/json/requirements/materials.json, then used in, for example, data/json/recipes/armor/head.json.

ZhilkinSerg added some commits Feb 14, 2017

Introduce missing yarn into recipes
(, \[ "plant_fibre", (\d+) \])
$1, \[ "yarn", $2 \]
@ZhilkinSerg

This comment has been minimized.

Copy link
Contributor Author

commented Feb 14, 2017

@Coolthulhu, there was some reason I didn't use "using", though "filament" material requirement was added.

@ZhilkinSerg

This comment has been minimized.

Copy link
Contributor Author

commented Feb 14, 2017

Thanks, @kevingranade!

It seems I have issue number one in here - I didn't use fetch and rebase, but did pull.

Do I need to cancel this pull request and create a new one after fetching and rebasing (probably with single commit for all of the changes)?

@ZhilkinSerg ZhilkinSerg changed the title [WIP] Crafting Overhaul: Tailoring [CR] Crafting Overhaul: Tailoring Feb 14, 2017

@Chezzo

This comment has been minimized.

Copy link
Contributor

commented Feb 15, 2017

Plant fibers work as thread too.

@ZhilkinSerg

This comment has been minimized.

Copy link
Contributor Author

commented Feb 15, 2017

Plant fibre were originally in recipes together with threads and sinews.

Anyway I've changed all recipes to use filament material requirement, so one can easily add more thread variants without need to change all of the recipes in the future.

@Coolthulhu Coolthulhu self-assigned this Feb 15, 2017

@@ -6847,6 +6852,36 @@
]
},
{
"id": "sewing_machine",

This comment has been minimized.

Copy link
@Coolthulhu

Coolthulhu Feb 16, 2017

Contributor

The sewing machine should be a separate PR. It needs to be cleaned up a bit before merging, while the filament change could go in now.
Problems include:

  • Doesn't use power
  • Isn't any faster than manual sewing
  • Uses more materials for same repairs (this one could be an explicit disadvantage, but probably isn't intended)
  • Has to be wielded to be used, which is tedious. Should probably wait until it can be used from the floor.

This comment has been minimized.

Copy link
@ZhilkinSerg

ZhilkinSerg Feb 16, 2017

Author Contributor

Thanks, I will remove sewing_machine from current PR.

"tools": [ [ [ "sewing_kit", 1 ], [ "tailors_kit", 1 ], [ "needle_bone", 1 ], [ "needle_wood", 1 ], [ "sewing_machine", 1 ] ] ]
},
{
"id": "knitting_standard",

This comment has been minimized.

Copy link
@Coolthulhu

Coolthulhu Feb 16, 2017

Contributor

You don't use that anywhere because you defined sewing quality instead.
You should either use the quality or the using, not both for the same thing.
Qualities are simpler, while usings are easier to extend (for example, if you ever wanted to add an electric knitter that uses power, qualities would not work).

This comment has been minimized.

Copy link
@ZhilkinSerg

ZhilkinSerg Feb 16, 2017

Author Contributor

I only added sewing_machine to sewing_standard, but it is already used for repairing of vehicle roof_cloth (see data/json/vehicle_parts.json).

knitting_standard is not used anywhere, so I will remove it.

This comment has been minimized.

Copy link
@ZhilkinSerg

ZhilkinSerg Feb 16, 2017

Author Contributor

I just realized it is possible to replace tools with qualities in sewing_standard:

before:

{
    "id": "sewing_standard",
    "type": "requirement",
    "//": "Crafting or repair of fabric items",
    "tools": [ [ [ "sewing_kit", 1 ], [ "tailors_kit", 1 ], [ "needle_bone", 1 ], [ "needle_wood", 1 ] ] ]
},

after:

{
    "id": "sewing_standard",
    "type": "requirement",
    "//": "Crafting or repair of fabric items",
    "qualities": [ { "id": "SEW", "level": 1 } ],
},

That way we won't need to add new tools (like sewing_machine or electric_knitter) to toolsets.

@Coolthulhu Coolthulhu removed their assignment Feb 16, 2017

@ZhilkinSerg ZhilkinSerg changed the title [CR] Crafting Overhaul: Tailoring [RDY] Crafting Overhaul: Tailoring Feb 16, 2017

@ZhilkinSerg

This comment has been minimized.

Copy link
Contributor Author

commented Feb 16, 2017

Sorry for the strange double commits - I really should learn using git...

@WizardOfOoo

This comment has been minimized.

Copy link
Contributor

commented Feb 19, 2017

Did you resolve the repair/reinforce issue you had? Or do you still need to load the tool to do repairs?

@ZhilkinSerg

This comment has been minimized.

Copy link
Contributor Author

commented Feb 19, 2017

Nope, repairing/reinforcing still needs loaded tools, but that will be changed eventually.

@Coolthulhu Coolthulhu self-assigned this Feb 23, 2017

@Coolthulhu Coolthulhu merged commit ce2f173 into CleverRaven:master Feb 23, 2017

1 check passed

default This has been rescheduled for testing as the 'master' branch has been updated.

@ZhilkinSerg ZhilkinSerg referenced this pull request Mar 17, 2017

Closed

[WIP] Tailoring without thread charges #20569

1 of 2 tasks complete

@ZhilkinSerg ZhilkinSerg deleted the ZhilkinSerg:crafting-overhaul-tailoring branch Aug 29, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.