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

[WIP] Sealed cans are not open contaners, and can be carried; empty cans are good for boiling #40742

Closed
wants to merge 2 commits into from

Conversation

wapcaplet
Copy link
Contributor

@wapcaplet wapcaplet commented May 22, 2020

Summary

SUMMARY: Bugfixes "Allow sealed cans to be picked up and carried, and empty cans to be used for boiling"

Purpose of change

To make cans containing beverages and food (pale ale, chicken, etc.) possible to stash in the inventory without spilling. Also to make the empty cans have boiling quality, like the old version (with separate empty can item).

Fix #40729
Fix #40658
Fix #40127

Describe the solution

I added the CONTAIN and BOIL quality to the cans, allowing them to once again be used for boiling water once emptied.

I also removed the explicit "open_container": true flag from the can_drink, can_food, and can_medium items, so they act like jars and bottles - as though they had lids (even after they have been opened). This is not a realistic behavior, but is a step toward a proper fix, and mitigates player annoyance in the meantime.

Describe alternatives you've considered

My previous attempt at fixing this #40465 was another band-aid that worked without being correct.

I'm happy to consider proposals for better solutions, but have already spent so much time code-diving this issue, I do not plan to actively pursue it any further.

Testing

Teleport into kitchens and raid the fridges

Additional context

This issue appears to only affect cans that are spawned by mapgen. Cans spawned through the debug menu are correctly sealed.

@jbytheway
Copy link
Contributor

This doesn't seem right. Isn't the sealed-ness of a pocket a thing that can change over time, but by changing the pocket_data means they'll never count as open_container, whereas they should once unsealed.

It seems like after this change an opened can will (incorrectly) not spill; is that so, or is there other code which overrides that property for opened containers?

@wapcaplet wapcaplet changed the title Sealed cans are not open contaners, and can be carried; empty cans are good for boiling [WIP] Sealed cans are not open contaners, and can be carried; empty cans are good for boiling May 22, 2020
@wapcaplet wapcaplet force-pushed the my-lids-are-sealed branch from 7e31a54 to 6f3c289 Compare May 22, 2020 02:01
@wapcaplet
Copy link
Contributor Author

wapcaplet commented May 22, 2020

It seems like after this change an opened can will (incorrectly) not spill; is that so, or is there other code which overrides that property for opened containers?

Indeed you are right. I have reverted the change, and am still seeking a correct solution.

@wapcaplet wapcaplet force-pushed the my-lids-are-sealed branch from 83dac95 to 26fa1dd Compare May 22, 2020 02:46
@Elpyfo
Copy link

Elpyfo commented May 23, 2020

So, is the issue here that opened and closed cans are not two distinct items with their own set of features? I'm trying to understand, but not making much headway.

Also, quite new to this, but what does it mean that "all checks have passed" but the title says WIP?

@wapcaplet
Copy link
Contributor Author

So, is the issue here that opened and closed cans are not two distinct items with their own set of features? I'm trying to understand, but not making much headway.

Yes, cans used to be this way - but not anymore, with nested containers. The separate items still exist in the JSON, but as I understand it they are no longer used/reachable by the code, which is why this change is needed.

Also, quite new to this, but what does it mean that "all checks have passed" but the title says WIP?

The "WIP" in the pull request title is my own indicator that work is not complete, and I don't consider this solution to be final. The "all checks have passed" part is a series of automated tests that are run on all code submissions, and just says they could not detect any problems.

@Elpyfo
Copy link

Elpyfo commented May 24, 2020

So, aoae23 found this doesn't happen with cans of food/soup, just with cans of drink. Perhaps what needs to happen is for the can of beer to have the same properties as the can of soup? Other than the sealing mechanism being different, I don't really see the difference between these anyhow.

@RoyBerube
Copy link
Contributor

The changes suggested are not a good idea imo. They alleviate some symptoms while introducing different but still incorrect behavior. It will just make it more difficult to sort out the fix for the problem in the future.

Container problem affects more than just cans. For example, a 3L jar of jam doesn't lose its sealed state after eating some of the jam. Consuming the item in the pocket of the container is not propogating any changes up to the container.

@Elpyfo
Copy link

Elpyfo commented May 24, 2020

The changes suggested are not a good idea imo. They alleviate some symptoms while introducing different but still incorrect behavior. It will just make it more difficult to sort out the fix for the problem in the future.

Container problem affects more than just cans. For example, a 3L jar of jam doesn't lose its sealed state after eating some of the jam. Consuming the item in the pocket of the container is not propogating any changes up to the container.

The rabbit hole goes deeper. Maybe we should get @KorGgenT 's input?

@KorGgenT
Copy link
Member

i've been talking about this bug on the discord - basically what's going on right now is i can't get a reliable reproduction case so i can't pinpoint the error

@Elpyfo
Copy link

Elpyfo commented May 24, 2020

i've been talking about this bug on the discord - basically what's going on right now is i can't get a reliable reproduction case so i can't pinpoint the error

Issue 40658 contains a save file where the error occurs and if you can tell me what exactly it is you'd want I can create a couple more. Can't code, but I can definitely test stuff. Apologies if I'm completely missing the point, I'm new to this! Cheers

@wapcaplet
Copy link
Contributor Author

wapcaplet commented May 25, 2020

i've been talking about this bug on the discord - basically what's going on right now is i can't get a reliable reproduction case so i can't pinpoint the error

Issue 40658 contains a save file where the error occurs and if you can tell me what exactly it is you'd want I can create a couple more.

I can still reproduce the problem with any beverage can (most easily found in a refrigerator). I can also reproduce the issue by loading the save game on #40658

The changes suggested are not a good idea imo. They alleviate some symptoms while introducing different but still incorrect behavior. It will just make it more difficult to sort out the fix for the problem in the future.

I'm still open to alternatives. My proposed change simply makes cans behave the same as bottles and jars (instead of making them always prone to spilling), so I think it's an improvement, but if you have an idea for a better way to solve the problem, I'll gladly consider it.

@anothersimulacrum anothersimulacrum added <Bugfix> This is a fix for a bug (or closes open issue) [JSON] Changes (can be) made in JSON labels May 29, 2020
wapcaplet added 2 commits June 2, 2020 14:53
This makes cans behave like other food/beverage containers (water
bottles, glass bottles and jars etc.),  as if the cans had lids.  Not
very realistic, but perhaps more credible than all factory-sealed cans
leaking as soon as they are picked up.

In other words this allows refilling empty cans, and still be able to
carry them without spilling, since the "spillable" status of an empty
can is no longer present.
@wapcaplet
Copy link
Contributor Author

I have still not found any solutions, and do not plan to seek any. Canceling the PR.

@wapcaplet wapcaplet closed this Jun 23, 2020
@wapcaplet wapcaplet deleted the my-lids-are-sealed branch July 2, 2020 00:13
@wapcaplet wapcaplet restored the my-lids-are-sealed branch July 2, 2020 00:13
@wapcaplet wapcaplet deleted the my-lids-are-sealed branch July 14, 2020 21:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
<Bugfix> This is a fix for a bug (or closes open issue) [JSON] Changes (can be) made in JSON
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Tin can bug Cannot stash cans Canned items are spawning opened again
6 participants