-
Notifications
You must be signed in to change notification settings - Fork 1.1k
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
Crops persistence #2137
base: master
Are you sure you want to change the base?
Crops persistence #2137
Conversation
tornac1234
commented
May 9, 2024
•
edited
Loading
edited
- Add crops persistence (grow progress and exact slot)
- Make sure crops sync works
- Sync trash can behaviour (related because you often need to throw items when playing with crops, ig)
- Sync deletion of giver objects (which give you some item when you click on them like melons)
- Sync fruit growing (from grown plants) and harvesting (picking up)
NitroxPatcher/Patches/Dynamic/PickPrefab_AddToContainerAsync_Patch.cs
Outdated
Show resolved
Hide resolved
New commit can be reviewed by itself |
NitroxModel/DataStructures/GameLogic/Entities/Metadata/FruitPlantMetadata.cs
Outdated
Show resolved
Hide resolved
NitroxClient/GameLogic/Spawning/Metadata/Processor/FruitPlantMetadataProcessor.cs
Outdated
Show resolved
Hide resolved
NitroxClient/GameLogic/Spawning/Metadata/Processor/FruitPlantMetadataProcessor.cs
Show resolved
Hide resolved
NitroxClient/GameLogic/Spawning/Metadata/Processor/PlantableMetadataProcessor.cs
Outdated
Show resolved
Hide resolved
public static void Postfix(FruitPlant __instance, (float, NitroxId, Plantable) __state) | ||
{ | ||
// If no change was made | ||
if (__state.Item1 == __instance.timeNextFruit) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Direct float equality could be problematic here, no?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
float a = __instance.timeNextFruit;
// bla bla bla
float b = __instance.timeNextFruit;
is there a reason why "a" could be different than "b" if __instance.timeNextFruit didn't change in the meantime ?
Note to fix something in StayAtLeashPositionMetadataProcessor (from leviathans PR) |
I merged this into the master branch on my local computer, and seems to work perfectly. Even restarting the server preserved the plant states, even the grow state. This seems to be OK. |
I followed the same process as kovadam69 to test this (delete ItemData.cs, remove referencing Method in
|
76dbd05
to
58f570f
Compare
I'll be correcting this after all that happened since last year |
…oring, fix plants death not broadcasted, fix trashcan sync, forcefully add and remove plants from containers (safer)
Corrected everything that was necessary, can I get more reviews and tests please ? |
This is really good, fixed both issues I was having before.
game[Nike2]-20250225.log |
fastgrow command is not synced and will just break any source of syncing the crops |
Do you have any example of seed to replant fast ? |
That is probably why the two players were different. |
Marblemelons and AcidShrooms |
I'm unfortunately unable to reproduce this bug... It might have been because of fastgrowth ? I'll try to sync the command but otherwise I'll just disable it |
Just retested and reproduced. It seems to be easiest to trigger right after a rejoin, and coincides with these errors in the server console:
server[crops2]-20250225.log |
That is a pretty bad error indeed. Can you explain to me the exact procedure you did to obtain these ? (please mention which commands you used to get the items, and when you picked up something/planted something, with the number of players) |
possibly relevant client log slice:
failed because it already changed to 'Coral_reef_purple_mushrooms_01_02(Clone)' at (-171.4, -30.8, 92.6), used to be 'AcidMushroomSpore(Clone)' at (-171.4, -30.8, 92.6) |
@NikeLaosClericus Thanks, I was able to reproduce the issues and fix them and I even synced both multiplayer-relevant fast commands (fastgrow and fasthatch) |
The problem is still there for me, but I'm going to restate it because I learned more: (instead of depending on harvesting a fully grown plant first) **Note: there are instances when, seemingly at the end of the variable 30-60 seconds, that planting will become desynced between players, and the planting player will see a filled grow bed slot, while the other sees it as empty. This can be seen here, as the top player planted a deep mushroom right at the end of the variable time, but this planting was not synced to bottom player. Bottom player then planted an acid mushroom in the same slot. both are independently harvestable by their respective player. Another instance here, same circumstances: The fast commands sync seems to be good. game[Nike]-20250226.log |
The bug I fixed requires you to use new mushrooms items tho, it's about how metadata is set onto them when you pick them up. Can you retry with a new world ? |
…anter, and a bug where the plant age wouldn't be calculated properly, and some refactoring
Thanks a lot for the bug reports @NikeLaosClericus, I could fix both of them and fortunately, no one will have to come across those after release :) |
@tornac1234 Thank YOU for fixing these, it's been bugging my friends that they couldn't farm anything. |
Yeah, I had been using a new world, I do with each commit you push. |
It's much better, the 30-60 second window seems to be gone, no more adult shrooms immediately growing from seeds. Now though, I've noticed ghost plants have (pretty rarely) come back.
The second player didn't have this issue, and could plant there safely.
(I'd like to just say that these are very small issues compared to what we came from, and so much has been fixed, that I would be thrilled with this getting merged even in this state.) launcher-20250227.log |
I am currently unable to reproduce the "ghost shroom" bug but I suspect it happens because of lag... And the second one happens because we never update the state of a plant when it finally grows up. I could do it but will give it a bit more thinking |
NitroxClient/GameLogic/Items.cs
Outdated
@@ -24,6 +24,11 @@ public class Items | |||
public static GameObject PickingUpObject { get; private set; } | |||
private readonly EntityMetadataManager entityMetadataManager; | |||
|
|||
/// <summary> | |||
/// Whether or not <see cref="Inventory.Pickup"/> is running. It's useful to discriminate between Inventory.Pickup from a regular |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't really understand this comment. What is regular?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I cut the comment again 💀 "a regular Pickupable.Pickup"
NitroxClient/GameLogic/InitialSync/PlayerInitialSyncProcessor.cs
Outdated
Show resolved
Hide resolved
NitroxPatcher/Patches/Dynamic/NoCostConsoleCommand_OnConsoleCommand_bobthebuilder_Patch.cs
Outdated
Show resolved
Hide resolved
Co-authored-by: Measurity <[email protected]>
I have to fix one more thing |
Two last commits needs review |
|
||
if (parent.GetComponent<Constructable>() || parent.GetComponent<IBaseModule>().AliveOrNull()) | ||
// This check must happen before the GetComponent<IBaseModule> which will be triggered (wrong result) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm unclear what this is trying to say. Is it saying that if the GetComponent call is done first it will produce an unwanted outcome? If so I think the end of the comment should be reworded it's a little unclear if being triggered is wrong or if this check prevents that
{ | ||
return; | ||
} | ||
// Error logging is done in the function |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Minor: Slight reword maybe, In the try function?
return; | ||
} | ||
// For planters, we'll always forcefully recreate the entity to ensure there's no desync | ||
if (container.containerType == ItemsContainerType.LandPlants || container.containerType == ItemsContainerType.WaterPlants) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is it worth us having Plants as a type then inherit? Then we can only have one check here and work for all plants
} | ||
|
||
public void AddItem(GameObject item, NitroxId containerId) | ||
// calls from Inventory.Pickup are managed by Items.PickedUp |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
V minor
// calls from Inventory.Pickup are managed by Items.PickedUp | |
// Calls from Inventory.Pickup are managed by Items.PickedUp |
return; | ||
} | ||
|
||
NitroxId planterId = waterParkId.Increment(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't have the increment code in front of me but I wanted to check this gives not the same as the other waterpark increment call.