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

Genericize fabric #54308

Merged
merged 9 commits into from
Jan 30, 2022
Merged

Conversation

a-chancey
Copy link
Contributor

@a-chancey a-chancey commented Jan 11, 2022

Summary

Balance "Genericize Fabric"

Purpose of change

This removes the AMMO flag from the fabric resources. Using the new charge to item migration, it will make the items go from {item}(###) to {item}># of {items}. When placed into inventory, the bundle disassembles itself and drops the inserted items into the ground.

Additionally, I bumped up the SUS group spawn quantities a good bit to compensate. The craft store I visited had at most 3 of any one kind of sheet, so I've bumped them up as the recipe usage will be shifting around very soon as I continue working on #54256 this week. I'll revisit craft store spawns with the next part of the tailoring overhaul.

Describe the solution

This removes the AMMO type from the various tailoring fabric materials - I left thread alone for now.
Added their IDs to the charge removal obsoletion file
increased some fabric spawn amounts by a fairly large amount in clothing store groups to bring them closer in line with the new usage amounts. I'll revisit craft stores later.

Describe alternatives you've considered

Not doing this, but there was desire on the discord to move the materials out of the ammo category, and honestly, it not only makes sense in the context of the tailoring overhaul, but also in how the materials themselves stack.

Testing

Generated a new world and character, visited various clothing stores to check that fabrics were spawning in reasonable quantities.
Loaded an existing world, every bundle wound up opening to the exact number of items as i previously had charges. it's a lot of items.

Crafted and disassembled a few items to make sure I get the expected results.

Additional context

This converts all fabric from ammo to generic and will likely upset some people, but i need to do it to continue the standardization of fabrics.
Clothing store SUS group spawns were a bit low given the upcoming changes and the removal of charges. I'm going to review other spawns as I go forward with the rest of the tailoring changes as well, but this seems to be a good start based on my testing, since they don't spawn with a bunch of charges any more and are used individually.
@a-chancey a-chancey requested a review from I-am-Erk as a code owner January 11, 2022 21:03
@github-actions github-actions bot added json-styled JSON lint passed, label assigned by github actions astyled astyled PR, label is assigned by github actions labels Jan 11, 2022
@kevingranade
Copy link
Member

Since this is going to impact players migrating from stable to stable as well, we need to spend some effort looking for a migration path that doesn't evaporate everyone's precious rag piles.

@a-chancey
Copy link
Contributor Author

Well I guess I know what I'll be trying to figure out tonight then, since I need to get this in place to finish up the rest of the updates to fabrics. If it only affected scraps it wouldn't really matter, but hitting sheets and patchwork clothing parts will hurt. I'll look into the code and see what I can do.

As for conversion, one charge to one item seems fair for right now, unless there’s objections. An obsoletion file for charge conversion might be a worthwhile idea, but that's beyond the scope of what I can commit to doing right now.

A bunch of things are gonna be shifting around - Sheets are going from the smallest of most fabrics to the largest square footage for each, so they’re gonna get bigger. The patchwork clothing parts are actually IDed as {material}_patchwork_sheet for every material except cotton. IIRC (not at my pc), cotton actually happens to be the only fabric with a patchwork sheet, so the rest of the materials will need a new {material}_patchwork item to cover the item named "patchwork clothing part" that become patchwork sheets. This won't be a huge deal as a patchwork sheet will break down to two patchwork clothing parts, but it could be a bit jarring at first load.

The suggestion given to me on discord was that it would be better to change the name and stats to match an ID than it would be to change the ID to match the name, and then create a new item to take the place of the old one with a more appropriate ID.

@github-actions github-actions bot added the BasicBuildPassed This PR builds correctly, label assigned by github actions label Jan 12, 2022
@github-actions github-actions bot removed the BasicBuildPassed This PR builds correctly, label assigned by github actions label Jan 12, 2022
@kevingranade
Copy link
Member

The suggestion given to me on discord was that it would be better to change the name and stats to match an ID than it would be to change the ID to match the name, and then create a new item to take the place of the old one with a more appropriate ID.

If a more transparent solution isn't apparent this would work, but I'm a little worried that would churn the heck out of the crafting recipes and mapgen. If I'm wrong about that then it might be the best option.

@a-chancey
Copy link
Contributor Author

The suggestion given to me on discord was that it would be better to change the name and stats to match an ID than it would be to change the ID to match the name, and then create a new item to take the place of the old one with a more appropriate ID.

If a more transparent solution isn't apparent this would work, but I'm a little worried that would churn the heck out of the crafting recipes and mapgen. If I'm wrong about that then it might be the best option.

Honestly, the patchwork clothing parts don't spawn anywhere as far as I can tell, as they're only acquired from disassembly/butchering currently.

I'm already auditing the recipes as part of this, and the only difference in the ID naming convention is that the current patchwork clothing parts will go from sheet_{material}_patchwork to {material}_patchwork to be the new clothing parts. Since most of that is actually handled within the tailoring requirements file, it should be minimal work to rename the existing to patchwork sheets, add the new clothing parts and a recipe to disassemble the existing (renamed) patchwork sheets back into the clothing parts.

I was considering migrating the patches to a better ID as well, as they're just IDed as {material} right now, but that just seems like unnecessary pedantry right now. At least making the patchwork sheets not only makes sense in the terms of the ID, but it's part of standardizing the fabrics, so that item was coming down the line one way or another - with the already existing ID.

@Maleclypse Maleclypse added the Items / Item Actions / Item Qualities Items and how they work and interact label Jan 14, 2022
@github-actions github-actions bot added the BasicBuildPassed This PR builds correctly, label assigned by github actions label Jan 28, 2022
@a-chancey
Copy link
Contributor Author

@mqrause did some fantastic work with #54843, I'll wrap up testing tonight.

Both new worlds and existing saves have the updated fabrics, but they spawn a bit oddly currently. Gonna make sure I review a few spawn points for counts, and check that I didn't break crafting, repairing or modifying before pushing up changes.

After that I'll get the tailoring overhaul/review going and hopefully have the first pass done by early next week.

Got all the fabric items that were AMMO moved to the charge migration blacklist.
Fixed my placement of faux fur and felt patch in the main list.
@kevingranade
Copy link
Member

Woo! Sorry to complicate this, but we really needed it.

@a-chancey
Copy link
Contributor Author

Failed some unrelated graphics tests so I just merged from upstream to see if it passes this time

@kevingranade kevingranade merged commit 7275d44 into CleverRaven:master Jan 30, 2022
@a-chancey a-chancey deleted the genericize_fabric branch January 31, 2022 15:31
@moll
Copy link

moll commented Feb 11, 2022

Hey,

I'm not sure if it's worthy of a new issue or it's somehow related to this ongoing migration/redesign, but I'm seeing some peculiar behavior with regards to space calculations. This is on 0d23870.

For example, see the image below where moving felt sheet to the cargo space fails:

Screenshot from 2022-02-11 21-01-51

I seem to have both the old style and new style items (without >, right?) on the group. It's as if the new style items seem to take up far more space than the inventory portrays — for some reason the 500L cargo space is full at only 44L. Any idea? Thanks!

@a-chancey
Copy link
Contributor Author

a-chancey commented Feb 11, 2022

There's a 4096 item limit in one space, so that's probably why you can't put more into it while it's not full on volume. I don't know if that's in place due to technical restrictions but it's a limit nonetheless, unfortunately.

I'm working on resizing the items, but I also have to review and update every single recipe that uses the impacted fabrics to match the new item weights. So it's taking quite a while to go through and audit hundreds of recipes.

Once the recipes are to a place where they make sense, then the fabrics spawns will be adjusted down quite a bit.

The bundles and having thousands of sheets are really only an artifact from upgrading an existing save - once I'm done, you'll likely never have over 100 sheets or a few hundred patches of any particular fabric without putting some effort into it, but you also probably won't need that much fabric ever.

@mqrause
Copy link
Contributor

mqrause commented Feb 11, 2022

Please note that the item limit is only for top level items. Items contained in other items don't count toward that. In that regard a felt sheet is the same as a felt sheet > 699 items, since the latter still is just a single felt sheet codewise, except it (temporarily until picked up) contains 699 other felt sheets.

So if you want to put all that on a single tile, put it inside some large bags and then the bags in the cargo space.

@moll
Copy link

moll commented Feb 12, 2022

Ah, that's interesting. Thanks for revealing this! First of all, this definitely needs better error messages as I suspect it's not actually unlikely to want to store more than 4k in a single tile. It's very easy to obtain both sugar and salt in high quantities (in addition to fabrics) and given there are no gameplay negatives in storing either without containers, I presume it's very likely to end up with 4k of items on a single tile. It is a little peculiar that the 4k limit applies to stacked items, too. I would've assumed stacks count as a single item in some tile's vector — one that contains a pair of an item id and its count — and the 4k limit is for unique items, ones that actually take up a row when shown in the inventory. That's not the case?

So if you want to put all that on a single tile, put it inside some large bags and then the bags in the cargo space.

Umm, containers aren't limited by 4k? That's even weirder, right, as whatever potential (technical) overflow problem the 4k limit on a tile may avoid, it's surely to come up in having a container with 4k items. Again, I've amassed a few thousand units of sugar in-game already purely by looting homes. What about paper? I've had over 4k sheets of paper on a real-world desk. A stack of 500 is just 10cm high after all. :P

I found #20483 and #35398 as related, incl. an old PR for deleting the limit outright (#35411). I'm fine with having a limit, too, as we are dealing with finite memory/bufers, but 4k itself seems a little low.

except it (temporarily until picked up) contains 699 other felt sheets.

Actually, I tried picking it up and all, but it doesn't dissolve into individual sheet-items. From this issue or an unrelated one I read that it's supposed to. Any special requirement for that to happen?

@mqrause
Copy link
Contributor

mqrause commented Feb 12, 2022

Currently food and ammo items are acting as a single item, even if you have thousands of them (sugar (1000) is one item while 1000 felt sheets is 1000 items), although that behavior is likely to go after the next stable. What kinds of changes are needed to make this not awkward ingame is a discussion for its own issue, though.

Actually, I tried picking it up and all, but it doesn't dissolve into individual sheet-items. From this issue or an unrelated one I read that it's supposed to. Any special requirement for that to happen?

Yeah, I can reproduce that. Simply wielding it doesn't trigger the inventory sanity check that would cause them to drop it seems. You have to put something inside a worn pocket or drop an item from your inventory so it actually happens.

@a-chancey
Copy link
Contributor Author

There's two types of stacking with items, individual units and "charges". Sugar, flour, etc are stored as charges, and ammo is also stored in a similar fashion. As such, sugar (8000) is 8000 units of sugar, but it's not considered 8000 individual items - it's 8000 "charges" of sugar.

And it makes sense in the instance of ammo or a powder, where an individual unit can mix back in to the total group. If I didn't need the extra tablespoon of sugar, I dump it back in the bag. No big deal.

Fabrics used to be this way, and then someone posted on Reddit about acquiring Kevlar thread for nomad gear for introduced by the Exodii. I thought "surely you can get Kevlar thread somewhere."

It turns out you can - by disassembling either combat or motorcycle boots. That's it. 6 thread charges each pair, because inexplicably, you can recover thread from boots as you take them apart. But that's not what bugged me - what bugged me was that a pair of combat boots took 66 "charges" of Kevlar sheets. Due to the way ammo/charge items are counted with the definitions, this meant it was 66% of a "Kevlar sheet". That came out to, based on actual weight of Kevlar fabric, a few square centimeters total of fabric. Split between two boots that were supposed to be 1/3 Kevlar.

Oh and disassembling the boots returned all 66 charges of Kevlar sheets, which just magically merged into the stack of 140 charges you had and suddenly you have two full "sheets" worth of charges of fabric.

This would imply either that fabric is liquid, that tailoring was a non-destructive process for resources, or that we didn't care that any type of patchwork Kevlar would lose its ballistic resistance IRL because Kevlar's protective capability is derived from a combination of its material strength (per weight unit, it's stronger than steel), and the tight continuous weave.

Could I have changed the deconstruct recipe and moved on? Sure, but that wouldn't solve the overarching issue that we've had multiple abandoned attempts at times overhauling the system. I decided I'd rather overhaul the system to completion, even if it meant some growing pains during the transition and taking on an insane amount of volunteer work.

that behavior is likely to go after the next stable. What kinds of changes are needed to make this not awkward ingame is a discussion for its own issue, though.

Honestly, the bundling is largely an issue for upgrading saves, and then it goes away. This is, imo, the most sensible solution, and beats my suggestion of "just let me nuke people's fabric sheets stockpiles".

I've been working through the recipe audit now so we won't require anywhere near the current quantity of fabrics in the future.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
astyled astyled PR, label is assigned by github actions BasicBuildPassed This PR builds correctly, label assigned by github actions Items / Item Actions / Item Qualities Items and how they work and interact json-styled JSON lint passed, label assigned by github actions
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants