Various fixes

[reyqn]

Adds compatibility for more than 3 players by stacking cabins. ~Right now only supports the base farm map, and needs to set StartingCabins at 1 in config.json.~

[ObjectManagerManager]

Thanks for the great work! And thanks for making use of Harmony; I haven't played around with it, but it seems like a neat tool.

I won't have time to test this until this weekend at the earliest. And since I'm unfamiliar with Harmony, I can only guess how exactly this works. A few questions:

1. The mod already hotfixes the host's chatbox to add cabin-build commands via private messaging (I apologize for the lack of documentation ATM). What features does this PR add? From my understanding, the cabin-building in this PR implementation is automatic and the cabins "stack" (i.e., they all sit on top of each other). Are there any other differences?
2. Are there any issues with overlapping mailboxes due to stacked cabins?
3. Are there any rendering issues with the stacked cabins in general?
4. What's special about vector location <50, 14>, where the cabins are auto-generated?
5. My guess is that this implementation makes it impossible for players to enter each others' cabins (even if they're non-overlapping). Can you confirm?
6. Why does StartingCabins have to be set to 1?
7. From what I've seen, generating buildings usually involves requesting a buildLock first (see, e.g., here). This implementation doesn't seem to do this. Are there race conditions we need to be careful with?

Perhaps we should also consider adding a mod configuration option for cabin generation mechanics. Some sort of "auto-stacked" configuration would use this implementation (and ignore StartingCabins, forcing it to 1). Some sort of "manual" configuration would disable this feature altogether, relying on the mod's existing chat-based build commands.

[reyqn]

You're welcome. And nothing is urgent, I just wanted to put the code out there in case someone wanted to host a server with lots of stacked cabins.

1. Yeah I saw that you could add more cabins from the chat commands, the issue is that when playing with many people, lots of cabins takes up a lot of place, and then you don't have much to grow stuff. This is just a way to stack cabins so players can still have place to grow their farm.

2. No, when a farmhand uses a mailbox, the game checks where the mailbox is supposed to be, so even if mailboxes are stacked everyone can still use them correctly.

3. Maybe, I'm not sure about which cabin type is built on the first spot, but anyway if there is an issue, I'll fix it when adapting the PR to work with other farms and starting cabin layouts. However, there will probably be rendering issues if someone upgrades his cabin, and other players do not.

4. That's just the location of the first cabin, it will be fixed too when I adapt the PR to be more robust with cabin locations.

5. Yes, you can't visit other people's cabins. I could use the farmhouse instead of the cabins to warp people inside their cabins, so players couldn't enter the main farmhouse but they could visit other players' cabins. Maybe chat commands could also allow players to visit stacked cabins.

6. Again, this will be fixed when I adapt the PR, but that's just so the area under the cabin is cleared by the game when the farm is generated (no stones, grass or sticks under the cabin)

7. I saw you did that, then I forgot about it when implementing, but I didn't encounter any issues. I can add it though.

And yes, mod configuration is a nice idea. This was mostly done to fit my use case starting from another mod, and quickly adapted to your mod, but I get that everyone doesn't want this exact configuration. For now I'll work on making it compatible with all farm types and cabin layouts, fixing possible rendering issues, and hardcoded cabin locations, I'll just quickly add a parameter in the config to allow or disallow harmony patching

[ObjectManagerManager]

Great, no surprises then. This sounds like a great PR; I'm happy to accommodate it.

Re 5: Absolutely. I can implement chat commands for this in a later PR to save you some time, if you'd like.

Re 7: I'm not familiar enough with the internal mechanics to know when certain locks need to be acquired, exactly. I just know that the buildLock is usually acquired before calling Farm::buildStructure(). Your implementation is just adding a building to the netlist and then loading it, so I'm not exactly sure whether race conditions are possible here. But I'd rather be safe than sorry---let's acquire the lock.

Keep me posted as you update this PR. I'll do some iterative testing and review as it progresses.

[reyqn]

Ok so this is mostly done, remains 7.

I'm not sure why, cabins are not building when I try using the buildlock, maybe I'm just not using it correctly.

[reyqn]

I also added a command to visit other players. The limitation would be if two players have the same name, I warp to the first I find.

Also you should only warp when already in a cabin, because the only way I found of easily warping players without mods client side is to use the pass out logic, and if you pass out outside you will lose some money.

As a side effect, It also allows you to warp home from anywhere, but I guess if people want to cheat why stop them, also it's paid.

[reyqn]

So I played with some friends, and we all got warped twice at 8pm. I have no idea why, so I changed how the server decides to warp clients. I'm not really happy about how I did it though. Also since we started playing at the same time, we noticed that the automatic cabin creation didn't work for multiple players at the same time, so I fixed it. There is also an issue with cabins' shadows being visually too solid due to stacking.

[reyqn]

I noticed I was often warping from outside when I just wanted to enter a friend's cabin to get resources, and doing so cost money for nothing, so in the end I added a check.

[reyqn]

The new warp mechanism fixes lots of things (notably the passing out warp to get home and glitches if some cabins are upgraded and other aren't), but it breaks warp commands (clients crash when warping). I'll see if that's fixable.

[reyqn]

So I finally worked out how to do what I wanted:

Cabins are created automatically whenever a new player joins and no cabins are available. Cabins are created out of bounds, so clients don't see them. Clients can check their mail at the farmhouse mailbox. Clients enter their cabins by running into the farmhouse door (if you click you will enter the real farmhouse) Clients get out of their cabins from the farmhouse door.

In the end, all clients use the farmhouse like if they were the main player, and all of them have their own "instantiated" cabin inside. You can still use warp command to enter a friend's cabin

[ObjectManagerManager]

This is looking great. And thanks for adding the fishing rod behavior link, and the code to unlock the community center. I haven't had time to do any full / integration tests, so I'm sure there are still missing features here and there.

Out of curiosity, why does the bot's mining level up passively?

Either way, the features seem to be pretty much fully implemented, from my perspective. I'm not seeing a config option to disable the harmony patching yet. That's especially important due to harmony's unpredictable compatibility with other mods. Let me know when you've added that config option and finished your local testing.

[ObjectManagerManager]

A couple more questions:

1. Why do we need to use harmony to patch the construction of the cabin NetFields before sending it to the client? Why can't we just access them and modify them via reflection later on, after construction? Maybe I don't understand Netcode objects correctly, but my understanding is that server-side updates to Netcode objects will propagate to all clients. Is there a concern that by directly replacing the NetFields array components with newly construct net objects, the new components will no longer reference the building object's direct members? For instance, normally Building.NetFields.fields[3] is a reference to Building.tilesWide, as assigned in Building.initNetFields(). By replacing the array component in ForgeCabin, I believe these become separate Netcode objects. Can that lead to bugs anywhere? e.g., when a cabin is moved, and the door tile changes?
2. What prevents players from moving their cabins? They're constructed off-map, but can't a player visit Robin to move them somewhere else?

[reyqn]

1. Modifying the netfields before sending to the client makes it sure that we actually send what me want. Plus since we have to edit the farm clone we send anyway to add the warp, we have to use harmony anyway. About bugs, I'm not sure I totally understand how the netcode works, but I didn't encounter any. Also it means the cabin isn't altered server side, which is nice, because

2. Nothing, but since the cabin wasn't altered server side, next time the server sends the cabin to the client, it won't edit the netfields, so the cabin will work as a regular cabin. I've not tried it, and I don't think it works right now, but this could work with minimal addictions.

[reyqn]

I didn't see the first message. There is actually a config option which defaults to false (stackcabins) and disable the harmony patch altogether.

About mining XP, the little monsters that drop coal on the ice levels in the mine sometime explode and break rocks. Those rocks surprisingly count as XP for each player in the game.

[reyqn]

In the end it was way easier to implement moving with the cabins really modified on the host in order to be able to take advantage of the broadcastlocationdelta method.

Now you can move your cabin inside the map to use it like a regular cabin, or move it back out of bounds and use the farmhouse as your cabin. The mailbox works as intended and updates when you move your cabin.

The only caveat is the farmhouse door. It could send you in the farmhouse or any cabin if you click on it, depending on which cabins here moved or not, so you have to run into it without clicking to get home.

[reyqn]

I'm wondering if I shouldn't make this a standalone mod, so that players can stack cabins in multiplayer and play as the host. Obviously it would still be compatible with this, and I would create a new PR for the fixes I added to the bot here.

[reyqn]

So I've done it, my mod is fully compatible with the server and available here

The fixes included in the PR now are:

• Unlock Willy
• Unlock community center
• Unlock sewer
• "hides" the bot behind the farmhouse
• Skip levelup menu, sets bot mining to lvl 10
• Check mailbox to unlock events

[reyqn]

Also I should let you know I think there is a bug in the cropsaver for crops that first grow and then produce stuff every x days. They don't seem to die at the end of the seasons.

[ObjectManagerManager]

Before you move on any further, I figured I should bring something to your attention: This mod is licensed under the MIT license. I fully intend on allowing anyone and everyone use it for any purposes (within the scope of SDV's terms of use, of course). If you want your contributions held under a difference license, they'll all have to go in your own fork.

In any case, I greatly appreciate your thorough testing and contributions :)

So I've done it, my mod is fully compatible with the server and available here

That's wonderful! I was going to suggest making this into its own mod anyways. If you'd like, I can include a pointer to it in the README, since other users will likely find it helpful as well.

Also I should let you know I think there is a bug in the cropsaver for crops that first grow and then produce stuff every x days. They don't seem to die at the end of the seasons.

The intention was the following:

• If the crop only produces a single harvest, it simply does not die at the end of the season. It will just die after the first harvest. Exception: If the crop was planted too late, so that the crop goes "out of season" before it has finished growing, then it still dies as usual.
• If the crop produces multiple harvests, then it dies once it has both gone out of season and has been harvested at least once. Exception: Again, if it's planted too late, then it just dies when it goes out of season.

The idea is to allow players to always get at least one harvest out of their crops in case they have to log off while their friends keep playing. Knowing this, does the mod work for you as expected? Or does there still seem to be a bug? If there still seems to be a bug, could you file an issue with steps to reproduce it?

I wouldn't be surprised if the crop saver was still a bit buggy. It is a bit hacky; it has to persist its own state and applies some hacky logic to determine which crops were planted by a player and need to be modified (as opposed to autogenerated crops). We could probably use harmony to monkey-patch the crop planting message receiver to clean things up a bit, but, for the sake of compatibility, I'd rather not rely on harmony for things that can be solved without it.

[reyqn]

Before you move on any further, I figured I should bring something to your attention: This mod is licensed under the MIT license.

I don't have any issues with this.

That's wonderful! I was going to suggest making this into its own mod anyways. If you'd like, I can include a pointer to it in the README, since other users will likely find it helpful as well.

Yeah in the end stacking cabins can also be useful without using a server, and the code doesn't have a lot on common with yours, so it's probably for the best. I started working on it inside your project because I saw some comments on your code saying it would be nice if it worked. You can mention it in your README, I think I would be happy finding it there if I was searching for a stardew valley server solution and ended up here, but it's not urgent ~(also I'm not so sure about the stability I just made some huge changes)~

Knowing this, does the mod work for you as expected? Or does there still seem to be a bug? If there still seems to be a bug, could you file an issue with steps to reproduce it?

Yeah I think there is still a bug, I'll try to reproduce it and file a real issue. Right now we disabled the cropsaver since we all manage the same crops.

About this PR, I could break it down in multiple ones if you feel that's too big, or things aren't really related.

[ObjectManagerManager]

I've finished testing most of it. I'm happy with all of the features, so far. The only feature I haven't tested all that much is the EndCommunityCenter behavior because it's a bit hard to test, but I think it works properly.

I'm merging this, but I do have two concerns that I'll resolve afterward:

1. The new behavior links (unlock community center, end community center, get fishing pole) all call processNext() unconditionally. processNext() should generally only be called when a behavior link is ignored (i.e., its trigger conditions are not satisfied) unless the behavior link is just modifying the state and passing it along the command chain. Following the other behavior links, processNext() should probably just be wrapped in an "else". This would help avoid bugs in the future by keeping these behaviors mutually exclusive, making them less likely to conflict with each other (e.g., the host trying to do multiple things in a single tick might not always play well with the game loop).
2. The beginning-of-day features including checking the mail, unlocking the sewers, and upgrading the farmhouse should not be in the ReadyCheckHelper. They should be moved somewhere else. They're mostly unrelated to one another, so maybe they should each get their own class. The ReadyCheckHelper class is just a big reflection class, providing an interface for working with the team's private ReadyCheck objects, updating references each day after they're reconstructed.

[ObjectManagerManager]

@reyqn

PSA: I've located, diagnosed, and fixed the crop saver bug. Refer to issue #8. Let me know if you see any further issues with it.

One small remaining "bug" (though intentional for simplicity) is that if a crop finishes growing on the first day of the wrong season (e.g., strawberries finish growing / become reharvestable on Summer 1), they aren't killed immediately. In theory, they should be since they weren't reharvestable in spring.

I think it can be fixed, but the logic will be a bit messy. I'll get it figured out in the near future.

[reyqn]

Thanks, I'll try it and report back