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

Fix #76414: Can't activate item - character instead tries to eat it. #76823

Merged
merged 2 commits into from
Oct 10, 2024

Conversation

CavanStone
Copy link
Contributor

@CavanStone CavanStone commented Oct 5, 2024

Summary

Bugfixes "Fixes #76414, Can't activate item - character instead tries to eat it."

Purpose of change

Fixes #76414 .

Steps to reproduce:

  1. Try to make some lard in a batch.
  2. Stop the action.
  3. Attempt to resume it
  4. Fail bc your character tries to eat it.

Can be duplicated by trying to resume any comestible craft. Also works with trying to activate pet foods such as cattle fodder.

Describe the solution

Revert commit e0650e5 . This commit changed the priority order by which item methods gets called, specifically it puts the consume action ahead of the use action in the priority queue. By reverting this commit the priority queue returns to the original order of use then consume.

A consequence / trade-off of this PR is that Comestible items that also have a use action defined to them such as delayed transform will no longer be able to be consumed. Having reviewed the comestible items that also have a use action, I beleive that the consumption of delayed transformation items are a rare edge case that most players would never try. These are disgusting items barely fit for consumption akin to dirty pool water or food soaked in brine. I think a higher priority demand for the players is to enable the main line use of these items which is to ferment or to feed pets and that sacrificing our ability to drink dirty pool water is worth it to make sure that Fido doesn't starve. The functionality to drink dirty pool water can be restored at a later lower priority PR.

Describe alternatives you've considered

Altering the delayed transform code to give the player the choice on comestibles as to whether to activate the transformation or to consume the item. The level of effort for this would be much higher and meanwhile my Fido is starving.

Testing

  1. Build and launch the game
  2. Open the debug menu and spawn Cattle Fodder
  3. Confirm that the pet food dialog box opens up.
  4. Spawn purifying water in the debug menu
  5. Activate the purifying water.
  6. Verifying the message, "The chlorine released by the tablets is still working. You still need to wait a bit before you can drink it." appears in the log.
  7. Run Unit Test Suite

Additional context

The Master Branch is also giving unit test failures in Russian Translation.

image

image

image

@github-actions github-actions bot added [C++] Changes (can be) made in C++. Previously named `Code` <Bugfix> This is a fix for a bug (or closes open issue) new contributor json-styled JSON lint passed, label assigned by github actions astyled astyled PR, label is assigned by github actions labels Oct 5, 2024
@CavanStone CavanStone marked this pull request as ready for review October 5, 2024 05:35
@PatrikLundell
Copy link
Contributor

I'm not too keen on this fudge. The issue is that you're not given a choice of action when there are more than one possibility. An acceptable solution to the sub problem of in-progress and in-process items would be to not make them edible in that state, but the main problem would still have to be tackled.

And you can keep Fido fed in the mean time by placing dog food at his tile (at least this worked with cows when I tested it).

@epebe
Copy link
Contributor

epebe commented Oct 5, 2024

The issue isn't just keeping Fido fed, its also being able to tame Fido at all. As it stands, trying to tame a dog results in me eating dog food, cows, cow food. Eating in progress crafts is also an issue because now I have to manually drop it onto a workbench, interact with the bench, and then select the craft to continue.

Like Cavan said, I don't think being able to eat dirty pool water is more important than these issues. Frankly, I haven't even engaged with the new water purification mechanics. There's plenty clean water in water heaters, and when I run out, I craft with the water purifier appliance.Sure there's a better solution, but promising to work on it later doesn't address the issue right now that locks me out of animal husbandry and makes crafting a keypressing slog.

@CavanStone
Copy link
Contributor Author

I'm not too keen on this fudge. The issue is that you're not given a choice of action when there are more than one possibility. An acceptable solution to the sub problem of in-progress and in-process items would be to not make them edible in that state, but the main problem would still have to be tackled.

And you can keep Fido fed in the mean time by placing dog food at his tile (at least this worked with cows when I tested it).

Let's not make the perfect become the enemy of the unbreaking of the game. We can divided impacts of this PR into the 3 aspects: Gameplay, Code Maintainability, and Social

Gameplay

On the net this PR represents a significant improvement in the player experience over the current state of EXPERIMENTAL.
This PR fixes a very heavily reported issue that is impacting multiple users on core gameplay features. #76414 , #76423 , #76425 . Some of these issues such as the inability to tame animals via pet food activation have no known work around. The one feature that would be disabled by this PR is the very very edge case of players deciding to consume the item undergoing delayed transformation, which would confer a significant amount of negative gameplay effects. In fact, many of the delayed transformation items put into the game long before this issue came up already disable the consumption action by having the GENERIC type instead of COMESTIBLE even though their category is food or drink.

Code Maintenance

As for your fudge comment, all I did was a git revert so if it is fudge, it is one long accepted by the project administration. This PR in fact eliminates a conditional check and returns the order of operations for using items back to the long standing convention that a large part of the code base axiomatically assumes. On the net accepting this PR would eliminate a conditional check in addition to unbreaking the game

Social

While I agree with you on where the end state should be. I disagree with how to get there. Developer bandwidth is limited. A lot of people's games are broken right now and we should not keep them waiting just to perfect a feature that nobody is going to use.

Furthermore the contributor guide advises to keep the scope of PRs small and I think going with your suggestion we will very quickly fall down the slippery slope of scope creep.

We should also keep this scope limited for the standard agile reasons.

  • Enhanced Clarity: Smaller changes are easier to comprehend, allowing reviewers to provide more effective feedback.
  • Faster Feedback Cycles: Focused, bite-sized changes can be reviewed and merged more quickly, accelerating development.
  • Simplified Bug Detection: Isolated changes make it easier to identify and address bugs.
  • Reduced Merge Conflicts: Incremental updates lower the risk of encountering merge conflicts.

@PatrikLundell
Copy link
Contributor

You're not fixing the issue, just hiding it by shuffling it into affecting more niche cases (unless, of course, you're breaking something else by shifting what gets the single top spot, in which case it doesn't even do that). That negates all of your arguments about scope etc.
Thus, the only thing this does is to make a hack to make some functionality sort of work again. The argument that your hack is a reverse of a previous hack that didn't fix the underlying issue isn't a particularly strong one.

@CavanStone
Copy link
Contributor Author

You're not fixing the issue, just hiding it by shuffling it into affecting more niche cases (unless, of course, you're breaking something else by shifting what gets the single top spot, in which case it doesn't even do that). That negates all of your arguments about scope etc. Thus, the only thing this does is to make a hack to make some functionality sort of work again. The argument that your hack is a reverse of a previous hack that didn't fix the underlying issue isn't a particularly strong one.

Please explain how anything I am doing is a net detriment to the player experience or the developer experience and I will fix it.

@github-actions github-actions bot added the BasicBuildPassed This PR builds correctly, label assigned by github actions label Oct 5, 2024
Copy link
Member

@RenechCDDA RenechCDDA left a comment

Choose a reason for hiding this comment

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

Playing musical chairs with what's broken is not a solution, it's just making it someone else's problem. Worse it complicates understanding the issue because someone who does want to fix it has to understand the timeline of behavior stretching across multiple versions of changing what's broken. That is supremely unhelpful.

2024-10-05.12-56-38.mp4

@johannes-graner
Copy link
Contributor

Is it not better to make sure the completely broken systems (animal husbandry) are working while a proper fix (e.g. a pop-up menu to select whether to consume or activate) is being developed?

Especially since the consumption of in-process items was not available before the change that broke animal husbandry, and was not a major part of that change (water purification tablets taking time to work).

As it seems to be quite important for some people that in-process items can be consumed, the proper fix would probably be prioritized, so hot fixing the game in the mean time should hopefully not delay that functionality too much.

@RenechCDDA
Copy link
Member

As it seems to be quite important for some people that in-process items can be consumed [...]

Just to be clear, 'master' is the experimental branch. Really the only thing that matters here is ease of development and applying that development. 'People want [thing]' always takes backseat to development. If this bug bothers people, they shouldn't be playing experimental. They should be playing the stable version, which we specifically spend great time and effort preparing to avoid these situations.

I can take another look at what the behavior was before #71971 but this is exactly what I'm talking about when I say this PR is like playing musical chairs. If it was broken before #71971, and it was fixed after, we don't want to re-break it!

@CavanStone
Copy link
Contributor Author

CavanStone commented Oct 5, 2024

If this bug bothers people, they shouldn't be playing experimental. They should be playing the stable version, which we specifically spend great time and effort preparing to avoid these situations.

Yeah you shouldn't really be blaming the players here for surfacing this to our attention nor should you blame them for communicating urgency. This not only tells us what work to prioritize with your limited time but the only effective way to have progress in stable is to have a healthy population playing experimental. Without those beta testers willing to put up with some disruption and variance bugs like this would easily make their way into stable.

Also you really shouldn't be using the Experimental branch as an excuse for waterfall development.

@RenechCDDA
Copy link
Member

30a378d (this PR): Unable to eat slab of raw curing porkbelly through the 'E'at menu

a26f6ea (master): Unable to eat 'slab of raw curing porkbelly' through the 'E'at menu

6341fd2 (last master commit before #76326): Unable to eat 'slab of raw curing porkbelly' through the 'E'at menu

So basically what CavanStone said in the first place is wrong, and reverting the commit in question does not actually regress any behavior. This isn't "re-breaking" anything. I still don't like the solution, largely in line with what PatrikLundell has said, but sure that can be another PR, even someone else's PR.

@CavanStone
Copy link
Contributor Author

So basically what CavanStone said in the first place is wrong, and reverting the commit in question does not actually regress any behavior.

Sure I'll take the hit on that.

@esotericist
Copy link
Contributor

Let's not make the perfect become the enemy of the unbreaking of the game. We can divided impacts of this PR into the 3 aspects: Gameplay, Code Maintainability, and Social

@CavanStone
speaking as one of the maintainers for the project, i feel the need to inform you that:

a) renech was not blaming players for surfacing the issue, and phrasing it as such adds an unnecessarily hostile tone to your feedback
b) renech is correct about the issue of urgency; we have a tendency to jump to it a lot faster when data loss is involved, but at the end of the day, that's still a courtesy, not an obligation, when experimental is involved.
c) our development policy is not actually crowd sourced. our team has quite a lot of professional experience in it, and we've landed at processes that work for the needs of the project, as well as serving as best we can the aggregate health of our maintainers and contributors.
d) presuming that the team in aggregate doesn't understand development models isn't a good starting point for convincing us of anything
e) in accordance with the above, unprompted persuasive essays of this sort aren't necessary or helpful, especially when you haven't built meaningful credibility with the team as of yet.

@trowt
Copy link

trowt commented Oct 6, 2024

Let's not make the perfect become the enemy of the unbreaking of the game.

You're not fixing the issue, just hiding it

As a brand new player, I spent a stupid amount of time trying to figure out what I was doing, because I tried to make some tallow, got the warning that I was in smoke, and selected to stop crafting so that I could move.

I have 3 separate in-progress crafts that got interrupted by smoke, zombies, world events, that I cannot resume.

Yes, I'm a newb. Yes, most of the tutorials, videos, docs are obsolete; however, this relatively new commit, especially compared to the help available, has broken the ability to resume any interrupted craft if this type. Maybe the game doesn't have enough newcomers to care that they can't resume things like making tallow, but it sounds like the commit broke more than it fixed.

@PatrikLundell
Copy link
Contributor

I've said my piece on this PR, so I'm only going to comment on @trowt's issue.

  • You're supposedly able to continue crafts lying on a table (or, ideally, workbench) by interacting with that furniture. I haven't had reason to try that, so I can't verify it's correct.
  • If you have companions, you can order them to resume crafting rather than start new crafts when ordering them to craft. That would also require that they have the necessary skills and access to resources to perform the task.

Neither of the work arounds above fix any issues, but can help you continue on without having to wait for any fixes. I.e. they're work arounds, not fixes.

@harakka
Copy link
Member

harakka commented Oct 6, 2024

As a new player you should be playing stable, not experimental where we can and do break anything on a day to day basis. New player experience on experimental is not a relevant argument for anything.

@worm-girl
Copy link
Contributor

  • You're supposedly able to continue crafts lying on a table (or, ideally, workbench) by interacting with that furniture. I haven't had reason to try that, so I can't verify it's correct.

This option is not available to Innawood players or characters who are stuck in a location where they do not have access to furniture, IE a character holed up in a cave or a car trying to craft stuff while they wait for a broken leg to heal.

@HadeanLake
Copy link
Contributor

HadeanLake commented Oct 6, 2024

holed up in a cave

They can use crafting spot. Cost nothing to build and it allows to continue crafts as if it is a workbench
Also, Innawoods characters can craft something that works as workbench early - fiber mat

@Maleclypse Maleclypse merged commit b62308e into CleverRaven:master Oct 10, 2024
40 of 48 checks passed
@Zireael07
Copy link
Contributor

Fixed at last, this was a pretty game-stopping bug (the workarounds suffer from poor discoverability and ux)

@CavanStone CavanStone deleted the Can't-activate-item branch October 11, 2024 22:21
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 <Bugfix> This is a fix for a bug (or closes open issue) [C++] Changes (can be) made in C++. Previously named `Code` json-styled JSON lint passed, label assigned by github actions new contributor
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Can't activate item - character instead tries to eat it.