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

Migration support to convert charges to copies #54376

Closed
a-chancey opened this issue Jan 13, 2022 · 14 comments · Fixed by #54843
Closed

Migration support to convert charges to copies #54376

a-chancey opened this issue Jan 13, 2022 · 14 comments · Fixed by #54843
Labels
[C++] Changes (can be) made in C++. Previously named `Code` Items / Item Actions / Item Qualities Items and how they work and interact

Comments

@a-chancey
Copy link
Contributor

Is your feature request related to a problem? Please describe.

Working down the chain of tasks on the tailoring audit #53436, it's been decided that the fabrics should migrate from ammo to generic. I was attempting this in #54308, and the work on genericizing them is done, but the migration is blocking approval for the merge. When the item types are changed to generic, their charges are removed, so cotton sheet (1500) becomes 1 cotton sheet.

I'd do it myself, but after two days of trying to figure this out, I'm going to admit that this is beyond my skillset and knowledge of C++.

Solution you would like.

a 1:1 charge to item conversion

savegame_json.cpp could possibly be modified to catch this in the io function. This if block already checks for items with charges that shouldn't have them, and will throw an error if the items isn't in the charge obsoletion file, before setting charges to zero.

Adding an else to the end of that check on 2901 would be a good place to have the charge removed and then spawn in copies equal to the number of charges, but I'm not exactly clear on how to handle that part.

Describe alternatives you have considered.

a more elegant solution would be being able to specify the conversion ratio in an obsoletion file

Additional context

No response

@mqrause
Copy link
Contributor

mqrause commented Jan 13, 2022

This if block already checks for items with charges that shouldn't have them, and will throw an error if the items isn't in the charge obsoletion file, before setting charges to zero.

Adding an else to the end of that check on 2901 would be a good place to have the charge removed and then spawn in copies equal to the number of charges, but I'm not exactly clear on how to handle that part.

As far as I understand that, it isn't possible to handle it there because that Archive can only be a single item, see item::deserialize. It's only able to do sanity checks of item properties, like remove charges if it can't have any.

@a-chancey
Copy link
Contributor Author

good to know. I'm a C# guy by day so reading CPP is like trying to read Dutch as an American. It looks familiar but wait what the hell does that mean?

@kevingranade
Copy link
Member

I just had a kind of crazy idea, at the point you identified, you could change the item being handled from a rag to a "bundle of rags", and insert the appropriate number of items into it based on the charges.

@PatrikLundell
Copy link
Contributor

Not ideal to force the player to manually dissemble the bundles generated, but certainly better than crushing stacks down to a single item. It would probably be confusing for players to find that they don't have any tailoring material after a migration, as all of it is suddenly locked up in bundles. Bonus points for creative thinking, but I think a proper migration is needed.

@anothersimulacrum
Copy link
Member

I think these 'bundles' would be bags of rags/patches, not the proper bundle items, because migrating to the bundle item won't fix this (bundle items are of fixed size, we need to migrate to arbitrary size)

@a-chancey
Copy link
Contributor Author

This should not be closed yet as this fix only applies to what is in the player's carried inventory.

Items on the ground still revert to just one single item.

@mqrause
Copy link
Contributor

mqrause commented Jan 28, 2022

They should also have copies in their migration pocket, but you need to pick them up for them to fall out.

Edit: When I try it with leather patches, it says leather patch > x items in every menu and in look around mode, so it should be pretty clear there's stuff, too?

@a-chancey
Copy link
Contributor Author

Oh yeah, I see that now, my bad. I missed the migration blacklist step initially.

did notice an issue or two so far - I migrated all the fabric scraps, and cotton scraps (50) gave me cotton scrap > 49 cotton scraps as expected. I then tried to pick up 1 (pressing 1 then l to select by quantity) and the game hung. Picking up the bundle broke it out into separate items, so that seems to work fine

@mqrause
Copy link
Contributor

mqrause commented Jan 28, 2022

I then tried to pick up 1 (pressing 1 then l to select by quantity) and the game hung

I'm not sure what you mean there? I did notice menus being slow to open with lots of items (~1000 in my test) in migration pockets, but after that everything went as usual. Items in the migration pocket shouldn't be directly accessible anyway.

@a-chancey
Copy link
Contributor Author

the game actually crashed completely and closed without giving an error message. i'd need to fire it up via visual studio to see what the error was.

@a-chancey
Copy link
Contributor Author

Ok other than the crash when trying to access individual items from the bundle, this works. Not sure how to deal with that, but yeah, picking them up breaks them apart and the ones in my inventory broke apart in one turn. I’ll get the fabric genericizing done today so I can get the rest of the tailoring stuff done.

Thanks for your hard work!

@mqrause
Copy link
Contributor

mqrause commented Jan 28, 2022

I'll check the crash out later. That really shouldn't happen.

@mqrause
Copy link
Contributor

mqrause commented Jan 29, 2022

I can't reproduce your crash. Pressing 1 (or any number) and then select just selects the one item here, because the ones in the pocket aren't accessible.
grafik

@a-chancey
Copy link
Contributor Author

Alright I’ll see if I can reproduce it later. It could have just been this weird inventory item selection bug I only have when running it in debug.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[C++] Changes (can be) made in C++. Previously named `Code` Items / Item Actions / Item Qualities Items and how they work and interact
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants