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

Snow Cones For Alcoholics #74269

Merged

Conversation

TheMurderUnicorn
Copy link
Contributor

Summary

Content "Add Alcoholic Snow Cones"

Purpose of change

I added snow cones in #73916 and said I would add alcoholic versions of snow cones, so I did that.

Describe the solution

I made an alcoholic snow cone item, which is weak alcohol but made from strong alcohol (because it makes 3, so it would be diluted so to speak). I added a recipe for this snow cone, allowing for a variety of juices/sodas to be used as flavoring and various hard alcohols to be used for the alcoholic component.

Describe alternatives you've considered

Allowing beer, but my reasoning was as follows: I did not add beers/ales/similar brewed alcohols to the list because one, it would be disgusting to make a beer strawberry snowcone or whatever. But also two, that the alcohol content in a beer is much lower than liquor/the other alcohols, and so the resulting alcoholic snowcone would end up basically tripling your drunkenness potential despite watering down the booze with ice and other drinks and making 3 snowcones from one portion of booze. This is also why mead doesn’t appear, despite being sweet, the weak alcohol part sort of messes things up.

Testing

I tested the names extensively. Some of them are not perfect but our control over the order is extremely limited. Additionally, there's no way to search ALL components of ALL ingredients for a specific id, it instead searches for anything containing that string. IE: "apple" can be specified if its the core ingredient. BUT you can't search ALL components for apple, because instead of using anything with the id of apple, it would instead accept anything that contains the word apple, so it would catch pineapple, crabapple, etc, despite them not having the exact string. That might not make sense in text... but it just doesn't work the way you want it to, basically x_x

Anyway, point is, some of the names are wonky, specifically apples, I mentioned this as well in that original PR of #73916 as snow cones have the same issue. Most of the names print perfectly, some get a bit complicated with long ingredient names (looking at you solomon's seal triple sec snow cones) and blah blah. But it's not anything problematic in my opinion.

On another testing note, I finished this like a week ago but didn't submit it, and although I did a final testing pass, it's possible I missed something because I walked away and forgot something. Let me know if you see any glaring issues.

Additional context

I guess I sort of added most of the additional context in the above sections. Again, check the original PR for additional information.

On another note: I did not add fruit wine to possible list of alcohols, due to this being player-crafted from various fruits, this will break the contextual naming and cause duplicate fruit names to appear. IE: it would output something like grape grape wine snow cone.

"Why wasn't [insert alcohol or mixed drink] added to the list?" - Basically anything that's weak alcohol is not viable, as mentioned above, because it would let you take one portion of weak alcohol, and convert it to something that's 3 portions of weak alcohol. As we only have two flags for alcohol, there's not much I can do about this other than exclude those things from possible components.

Screenshots:
image
image

@github-actions github-actions bot added [JSON] Changes (can be) made in JSON json-styled JSON lint passed, label assigned by github actions astyled astyled PR, label is assigned by github actions BasicBuildPassed This PR builds correctly, label assigned by github actions labels Jun 2, 2024
@Maddremor
Copy link
Contributor

Why are not requirement groups used here? (Applies to the original snow cone PR too, but I was not present for that.)

@TheMurderUnicorn
Copy link
Contributor Author

TheMurderUnicorn commented Jun 3, 2024

Why are not requirement groups used here? (Applies to the original snow cone PR too, but I was not present for that.)

I'm not sure what you mean by requirement groups, can you elaborate?

EDIT: After talking with Karol who explained what they are, I don't feel that having a group for these is all that necessary. The only overlap are the sugary drink components, which would admittedly save like a dozen lines, but I can't think of any future uses for such a group. If you meant a tool group for snow machines or something like that, it's currently just the one machine, if I ever got around to added the higher tier commercial machine I would make some sort of tool group for them (if that works how I think it does)

@TheMurderUnicorn
Copy link
Contributor Author

Also as mentioned I had to hand pick/avoid certain liquors, so I do think actually having a liquor group would be applicable to other recipes/useful, in this case certain alcohols had to be left out. IE: I can't have a group that contains all wine, because this recipe can't use the player-crafted wine without breaking the contextual naming. So even though having a wine or liquor group would be useful for the project, this recipe should have them manually chosen.

@TheMurderUnicorn
Copy link
Contributor Author

TheMurderUnicorn commented Jun 4, 2024

Updating this to use sweet_juice has now opened the door for the naming convention being broken if someone were to add other things to that list in the future. I also left kompot out for a reason, I thought, though I don't recall why now. I do not understand this change being made when I had offered a rebuttal on why I didn't change it and it effectively would only save a few lines in the json. EDIT: I did not see that you also converted to use a soda group, that is my mistake. EDIT 2: I guess I didn't offer much rebuttal previously, so okay. I do think this opens the door to breaking the naming to basically save a few lines, which to me isn't worth it but idk

@Maddremor
Copy link
Contributor

IMO, it's less about saving a few lines than it's about not having to track down this specific recipe and update it if more sodas were added.

@TheMurderUnicorn
Copy link
Contributor Author

That's fair. And I guess it's like, well, if it breaks someone can fix it. I'll make a PR to update the basic snow cone when I have some time

@Maddremor
Copy link
Contributor

Eh, I'm just a lowly Discord moderator. This is just my opinion.

@Maleclypse
Copy link
Member

Maleclypse commented Jun 6, 2024

Updating this to use sweet_juice has now opened the door for the naming convention being broken if someone were to add other things to that list in the future. I also left kompot out for a reason, I thought, though I don't recall why now. I do not understand this change being made when I had offered a rebuttal on why I didn't change it and it effectively would only save a few lines in the json. EDIT: I did not see that you also converted to use a soda group, that is my mistake. EDIT 2: I guess I didn't offer much rebuttal previously, so okay. I do think this opens the door to breaking the naming to basically save a few lines, which to me isn't worth it but idk

Are you sure you left out kompot? I may have miscounted but I literally only did those two lists and not the alcohol lists because I was handing matching every item in the list as already existing in the recipe. One second I can go check the commit history.

edit: yes. You had already manually defined this list to include kompot.

Edit2: My thoughts are that including lists that make sense and already have full coverage in the recipe ingredients is always the right choice and that if something breaks the inherited name (which should be turned into a c++ field anyway in my opinion and I'm going to request that right now) that's fine as it will only break for the new ingredient and be very simple to identify as wrong and then fix. :) I like this recipe. Thank you for making it!

@Maleclypse Maleclypse merged commit 0c962bb into CleverRaven:master Jun 6, 2024
24 checks passed
feinorgh pushed a commit to feinorgh/Cataclysm-DDA that referenced this pull request Jun 8, 2024
* Add Alcoholic Snow Cones

* Alcoholic Snow Cone Recipe Added

* Update frozen.json

---------

Co-authored-by: Maleclypse <[email protected]>
@TheMurderUnicorn TheMurderUnicorn deleted the SnowConesforAlcoholics branch July 4, 2024 00:05
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 [JSON] Changes (can be) made in JSON json-styled JSON lint passed, label assigned by github actions
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants