unitystation / unitystation

The original unitystation
https://unitystation.org
GNU Affero General Public License v3.0
704 stars 648 forks source link

A new crafting system and a method for learning new recipes #7024

Closed Lizurt closed 2 years ago

Lizurt commented 2 years ago

Purpose

fixes #6158

Notes:

It was tested on a headless server and works fine.

Thanks .ᚲᚨᚲᛖ᛫ᛁᛋ᛫ᚡᛖᚱᛃ᛫ᛏᛁᚱᛖᛞ᛬ for the weapon category icon.

Changelog:

CL: [New] The new crafting system and its user interface.

Lizurt commented 2 years ago

I think that's all. The only thing that may change is the UI design, but this change shouldn't lead to changes in the code. And it can also be done in a separate PR.

I can't do anything with the unit tests because they don't say what's wrong, but they fail.

I hope I made a clear enough code, because there are quite a lot of code changes. Good luck with reviewing, bois.

Akbadain29 commented 2 years ago

The unit tests say the following:

SpawnableListTest

_Base_RecipeBook (UnityEngine.GameObject) needs to be in the spawnPrefabs list and has been added. Since the list has been updated you NEED to commit the changed NetworkManager Prefab file _Base_RecipeBook (UnityEngine.GameObject) needs to be in the allSpawnablePrefabs list and has been added. Since the list has been updated you NEED to commit the changed NetworkManager Prefab file BakingRecipeBook (UnityEngine.GameObject) needs to be in the spawnPrefabs list and has been added. Since the list has been updated you NEED to commit the changed NetworkManager Prefab file BakingRecipeBook (UnityEngine.GameObject) needs to be in the allSpawnablePrefabs list and has been added. Since the list has been updated you NEED to commit the changed NetworkManager Prefab file _BaseCategoryButton (UnityEngine.GameObject) needs to be in the allSpawnablePrefabs list and has been added. Since the list has been updated you NEED to commit the changed NetworkManager Prefab file Food (UnityEngine.GameObject) needs to be in the allSpawnablePrefabs list and has been added. Since the list has been updated you NEED to commit the changed NetworkManager Prefab file Misc (UnityEngine.GameObject) needs to be in the allSpawnablePrefabs list and has been added. Since the list has been updated you NEED to commit the changed NetworkManager Prefab file Weapons (UnityEngine.GameObject) needs to be in the allSpawnablePrefabs list and has been added. Since the list has been updated you NEED to commit the changed NetworkManager Prefab file CraftingMenu (UnityEngine.GameObject) needs to be in the allSpawnablePrefabs list and has been added. Since the list has been updated you NEED to commit the changed NetworkManager Prefab file RecipeButton (UnityEngine.GameObject) needs to be in the allSpawnablePrefabs list and has been added. Since the list has been updated you NEED to commit the changed NetworkManager Prefab fileat Tests.Asset.NetworkManagerTests.SpawnableListTest () [0x00172] in /github/workspace/UnityProject/Assets/Tests/Asset/NetworkManagerTests.cs:68SendMessage cannot be called during Awake, CheckConsistency, or OnValidate SendMessage cannot be called during Awake, CheckConsistency, or OnValidate

TestScriptableObjects

Can't load asset ScriptableObjects. Maybe linked ScriptableObject script is missing? Expected: But was: < "ScriptableObjects" >at Tests.MissingAssetReferences.CheckMissingScriptableObjects (System.String path) [0x000dd] in /github/workspace/UnityProject/Assets/Tests/Asset/MissingAssetReferences.cs:131 at Tests.MissingAssetReferences.TestScriptableObjects () [0x00000] in /github/workspace/UnityProject/Assets/Tests/Asset/MissingAssetReferences.cs:176Can't load asset ScriptableObjects. Maybe linked ScriptableObject script is missing?

TestSingletonScriptableObjects

Missing reference found in scriptable object CraftingRecipeSingleton field Element 5 Expected: But was: < (CraftingRecipeSingleton, Element 5) >at Tests.MissingAssetReferences.CheckMissingReferenceFieldsScriptableObjects (System.String path, System.Boolean checkEmpty) [0x00131] in /github/workspace/UnityProject/Assets/Tests/Asset/MissingAssetReferences.cs:170 at Tests.MissingAssetReferences.TestSingletonScriptableObjects () [0x0000b] in /github/workspace/UnityProject/Assets/Tests/Asset/MissingAssetReferences.cs:184Missing reference found in scriptable object CraftingRecipeSingleton field Element 5

You can view the result of unit tests here by clicking on the commit, select the "Checks" tab, click "tests", scroll down to "Artifacts" and click "Test results".

Lizurt commented 2 years ago

You can view the result of unit tests here by clicking on the commit, select the "Checks" tab, click "tests", scroll down to "Artifacts" and click "Test results".

Thanks, now I see.

Unit test failures were fixed.

corp-0 commented 2 years ago

how should I test this in-game?

Lizurt commented 2 years ago

how should I test this in-game?

You can open a crafting menu using small action buttons in the main interface.

I added some recipes and categories as examples for testing, so you can test like a regular player.

Ingredients or reagents should be in front of you. Tools should be in your hands.

A flat dough is an examples of simple recipes and also an example of recipes that require tools.

A dough is an example of recipes that require reagents.

Other recipes are boring and require ingredients only.

Baking recipe book is an examples of recipe books. The book contains a dough and a flat dough recipes. Just read it to add new recipes to your crafting menu. You may use the spawn menu to spawn this book.

A refresh button(top left corner) may help you to test "gold" borders for recipe icons. These borders indicate that you can already craft something.

Unfortunately there are no examples for result handlers and recipe forgetting - I removed them in my last commits because they were useless(for a player). But if you want to test these things yourself, I may add them, just tell me

Lizurt commented 2 years ago

I'm intrigued to see how the read only fields are set because I didn't get it just from reading.

Some of them are fully automated through a custom editor, some of them require pressing special buttons. For example, before deleting a recipe, you need to click the "Prepare for deletion" button, and when creating a new recipe, you need to click "Add to the singleton if necessary".

ReadOnly attributes only prohibit manual editing, but they don't prohibit edits through code(in my custom recipe editor)

corp-0 commented 2 years ago

image I'm clicking on craft but nothing happens

corp-0 commented 2 years ago

same here image

Lizurt commented 2 years ago

image I'm clicking on craft but nothing happens

Ingredients should be in front of you, not in your hands

https://user-images.githubusercontent.com/42589308/127985484-b5bbdd15-6df1-4ef0-b016-dda886bc1fe2.mp4

I guess I need to add a help button to the menu

corp-0 commented 2 years ago

Issue says reachable tiles, pockets and hands. It would also be good to have some feedback when you click the button but can’t craft the thing.

Lizurt commented 2 years ago

Issue says reachable tiles, pockets and hands

I can't reduce the complexity of the algorithms, but I try to at least reduce a size of input data. I'll discuss this with the author of bounty.

It would also be good to have some feedback when you click the button but can’t craft the thing.

Yep, I agree. Will do.

I also noticed a bug of a rejoined client when I was recording the video above. I advise you not to test PR until you see a commit with a bug fix.

Lizurt commented 2 years ago

Done, ready for review again (if the unit tests and codacy will be successfull of course).

Tools still should be in hands, but ingredients may be on adjacent tiles, in hands or pockets.

ThatDan123 commented 2 years ago

Was posted on discord first, just putting here for easy access

Currently in this PR all objects are sent to the client from the server, this is unneeded as the client already knows what objects are around it and in their inventory. The current system also relies on the reagent container contents being sync'd which it is not so would currently not work in a full client-headless setup.

System will need to be reworked into something like this:

image

-This way the server only needs to send a list of reagents (and their volume) which are nearby, client does all the recipe validation before sending a crafting request

-Server checks that twice, and would stop crafting when something is missing

-Would also probably add a check to only ask for the surrounding reagents if the Passed recipes actually require any, stops unneeded requests to the server

-There still a spam security issue with asking for the reagents, by spam opening and closing when you know a recipe has passed the object checks and requires reagents, would add a cooldown for that and maybe a cooldown for when you start a craft just in case they stop the craft and start it repeatedly causing the server to do the first checks over and over. That'll be a server and client side cooldown see #7201 for how to do it

Lizurt commented 2 years ago

Ready for review again.

I made something similliar to the dan's plan.

(also please remove the Status: Merge Conflicts tag)

ThatDan123 commented 2 years ago

I'll try to review it all tomorrow if i have time. If anyone has time to build test as well please do.

PetMudstone commented 2 years ago

Crafting items when you have more than the necessary amount of ingredients available doesn't remove the correct amount of items upon crafting completion. For example, batons need 15 wooden planks to craft. However, when you craft a baton when there are 16 wooden planks available you only use 1 wooden plank. Crafting while 20 wooden planks were on hand only used 5 wooden planks. Meanwhile, crafting batons when 34 wooden planks are available uses 19 planks instead of just 15. Notably in all these cases the amount of planks was reduced to 15. It seems like if you have more than the necessary amount of ingredients then crafting always reduces the amount of ingredients to the necessary amount.

PetMudstone commented 2 years ago

On a side note, I find it a bit annoying that the crafted items aren't put into your hands when they are empty.

Lizurt commented 2 years ago

Crafting items when you have more than the necessary amount of ingredients available doesn't remove the correct amount of items upon crafting completion. For example, batons need 15 wooden planks to craft. However, when you craft a baton when there are 16 wooden planks available you only use 1 wooden plank. Crafting while 20 wooden planks were on hand only used 5 wooden planks. Meanwhile, crafting batons when 34 wooden planks are available uses 19 planks instead of just 15. Notably in all these cases the amount of planks was reduced to 15. It seems like if you have more than the necessary amount of ingredients then crafting always reduces the amount of ingredients to the necessary amount.

Alright, seems it's fixed now.

On a side note, I find it a bit annoying that the crafted items aren't put into your hands when they are empty.

Sure why not. Fixed.

ThatDan123 commented 2 years ago

Other than IResultHandler cant see any code issues. Some of the netmessages could be Commands/TargetRPC's instead, but having them in separate files as netmessages is fine.

If this is working fine for headless and two connected clients, then i think this is good to be merged. Good work!

If you have time could you also make a docs page for making new recipes? https://unitystation.github.io/unitystation/

Lizurt commented 2 years ago

If you have time could you also make a docs page for making new recipes? https://unitystation.github.io/unitystation/

Yeah I was planning this. I'll create docs after a while as the changes come into the game.

ThatDan123 commented 2 years ago

Just did a quick build test, all seems good :)

waffielz commented 2 years ago

now we got mining AND crafting