searica / MoreVanillaBuildPrefabs

GNU General Public License v3.0
2 stars 1 forks source link

Latest update seems to break on lots of previously existing prefabs (eg cliff_mistlands1) #6

Closed dreamwraith closed 10 months ago

dreamwraith commented 11 months ago

Our configuration file is intact, but tons of the items we had previously are no longer showing up in the build menu, nor in config manager ui, even though they are still in the configuration file.

searica commented 11 months ago

Those prefabs are not broken, they have been disabled. I changed the filtering requirements to prevent adding prefabs that spawn anything with the MineRock5 component when destroyed. There are a few reasons for doing so, but the main one is that if you placed any of those prefabs and then hit them a pickaxe or other weapon they could spawn an unremovable piece that sometimes matched the original prefab and sometimes was much much larger than the original prefab. This can cause some significant issues for players that damage those prefabs in any way after placing them, hence my disabling them. If you regenerate the cfg file you would find that none of those prefabs are in it anymore.

This change removed ~30 prefabs, the majority of which (if not all of them) were not enabled by default. For reference, as a general rule if a prefab is not enabled by default then it may be subject to significant changes since it may be a prefab that I haven't got to patch/test yet.

Hopefully that makes sense and clears things up. Let me know if you have any other questions or would like more clarity on a particular aspect.

If disabling those prefabs is a deal-breaker for you or your players and you can let me know the specific prefabs you'd like to still have I may be able to add a config option to re-enable them (would be disabled by default though). That said, due to the issues with the MineRock5 component they are semi-unsafe for use in building but as long as you're aware of that and still want them I'm open to a compromise.

dreamwraith commented 11 months ago

What you describe is a non issue for me personally. I use all the mountain/mistland rocks to design my own islands. I understand the concern, it's just suddenly having them disappear when the mods were updated in r2 makes it look as though its a bug occurring.

I don't want to tell you how to do free work for others though :) If they remain disabled I'll probably use one of the other mods thats less user friendly to get it done.

As an aside, I have noted another issue when using this on a dedicated server - the config file does not seem to save properly when modifying the configuration via config manager. I don't have this issue with other mods, just this one. My workaround for now is to just load a single player world, modify the config, save it, then upload to my server.

searica commented 10 months ago

Designing islands is a neat use case, I like that.

I did mention that I changed the prefab filtering in the changelog for the last update though I didn't explicitly mention which prefabs got removed in the process. That said, I do understand being surprised to find prefabs you used a lot missing though.

I'll look into possibly disabling the spawning of the MineRock5 component when deconstructing player-built instances as an alternative solution and if that doesn't work out I'll just add the config option to re-enable those prefabs anyways. Still achieves the goal of disabling them to prevent issues for most users while giving me a way to allow people to make a more informed choice and opt-in to those prefabs (since I can warn about the issues via the config option description).

Regarding your aside, could you describe the exact sequence of steps you follow when it doesn't seem to save properly on a dedicated server? I have not had that issue during any of my testing so it'll be hard to look into without the steps to reproduce it.

searica commented 10 months ago

Ah, I just figured out the issue with the config file on the server. Currently the config file is only saved when the game shuts down (kills performance otherwise). That meant that if you changed the config in-game on the server and then shut down said server it would keep your changes (which I often do during testing cause I'm making changes to the mod). Issue will be fixed in next update. Thanks for letting me know!

searica commented 10 months ago

All right, I just released version 0.4.6 on Thunderstore. The issue with config changes not persisting on the server is fixed and I have re-enabled the prefabs that spawn MineRock5 components when destroyed but a warning is now automatically added to the description of those prefabs so people are aware of the possible issues with them.

I did test out disabling the spawning of the MineRock5 prefabs for player-built pieces but that meant the whole cliff would just go poof if it was damaged at all. So that was a non-option as well since it would make building off of those pieces super risky. So I think the warning is the best option and it was less work than adding another config option. Let me know what you think after you get a chance to test the update.

dreamwraith commented 10 months ago

Oh wow you're awesome! I was gonna try and capture logs at home today for the config. I'll be sure to check things out and let you know the results.

Re: minerock5 issue - would it be possible to just make them invulnerable by default (for these problematic ones) or is that not possible?

Thanks so much for your quick response and willingness to work some things in for me.

dreamwraith commented 10 months ago

Hi,

Was able to test, everything works well now, thanks. Just one unfortunate bug.

The fix for the config file save seems to result in an infinite loop, breaking the server. Basically get the below spammed in log as soon as someone logs off and the server dies.

[Info :MoreVanillaBuildPrefabs] Reloading config file 10/28/2023 18:22:18: Console: [Info : Unity Log] 10/28/2023 18:22:18: Console: [Info :MoreVanillaBuildPrefabs] Reloading config file 10/28/2023 18:22:18: Console: [Info :MoreVanillaBuildPrefabs] Reloading config file [Info :MoreVanillaBuildPrefabs] Reloading config file 10/28/2023 18:22:18: Console: [Info : Unity Log] 10/28/2023 18:22:18: Console: [Info :MoreVanillaBuildPrefabs] Reloading config file 10/28/2023 18:22:18: Console: [Info :MoreVanillaBuildPrefabs] Reloading config file

The above is repeated over and over.

dreamwraith commented 10 months ago

I think this is occurring because when you call PluginConfig.save() it saves the config file, but any modification of the config file triggers a reload of the config file, thus again triggering the PluuginConfig.save() since you are calling the save() during Plugin Re-Initialization.

You may need to disable the auto-reload on modification while performing the save().

searica commented 10 months ago

Haha damn. All right I'll try to find a different solution to the server config issue sometime tonight. That wasn't something I could test by myself unfortunately. With only a single player it doesn't trigger the loop. Calling the re-init functions also only has an effect if settings have changed so I'm not sure exactly what the cause is.

Re: minerock5 issue - making them invulnerable is technically possible but it would cause issues for people without the mod, so it's not really an option that I would go for given that the mod is designed to work as a client-side only mod if needed.

dreamwraith commented 10 months ago

Cool, understood. Let me know if there is anything you'd like me to test. As a workaround, I disabled the mod server side and am just manually syncing configs to players.

searica commented 10 months ago

Glad you've got a workaround. Still wish that it wasn't an issue. I've made a couple tweaks but I don't know if fixed it, would you be able to test out the version I just pushed to the repo? https://github.com/searica/MoreVanillaBuildPrefabs/blob/main/Publish/ThunderStore/MoreVanillaBuildPrefabs.zip

You'll need to manually install it (can do so using the import local mod feature in r2modman or thunderstore mod manager).

That version changes the way that re-inits are triggered to always be based on reloading the file, so it just saves the file after changes are made using the in-game manager now. I've also reached out in the JVL discord to see if anyone there has other ideas.

searica commented 10 months ago

Also you're very welcome regarding re-implementing the prefabs that got removed. One of the things I've enjoyed about making this mod is finding out the different ways people are using it that I didn't expect or think of myself. So long as how they want to use it still allows me to keep the mod compatible with vanilla players I'm pretty willing to make adjustments, and it gives me new ideas too. Hadn't thought of making my own islands but I totally am going to now!

dreamwraith commented 10 months ago

Hello,

Thanks for the quick attempt. Unfortunately the file linked does not appear to have fixed anything. I am assuming you didn't increment the version #, but regardless, with the new package on server and clients, I saw no behavior change. The config file still fails to save, and the server still gets an endless loop of choking.

I have spotted a few other issues as I slowly enable some of the pickable prefabs to use for decoration in the ocean biome where I am making islands, I will open a separate issue for those when I get them better documented in a couple days though.

searica commented 10 months ago

Darn, okay. I've disabled the patch that saves the config when you log out and pushed a new change. Now it saves when you change the config in-game and the reloads it to trigger the re-initialization. The result is there's a bit more of a noticeable lag when you change config settings. The change should almost certainly resolve the infinite loop, though I don't think it resolves the server sync issue. Let me know if the new version (incremented version number this time, but it's the same link) fixes the issue.

All right, let me know about the pickables later. I've tested most of the crops/plants but if you're enabling ones that aren't enabled by default there is a good chance of finding new bugs.

dreamwraith commented 10 months ago

Headed to bed tonight, will test this in my morning tomorrow and let you know. Aside: Here is a bit of a screenshot of our islands we have been building. View distance chops it up a bit, but you get the idea. Valheim 10_29_2023 12_39_51 AM

searica commented 10 months ago

Oh that looks amazing! I'm really glad you pointed out the possibility of making islands. (P.S. if you take some screenshots you're happy with I'd love to include them on the mod page as an example of what you can do with the mod).

I chatted a bit with the Jotunn devs and it looks like some changes might need to be made to get the way configuration settings are synced on the server itself. For now I've updated the readme (which required a new version on thunderstore) to mention the known issue. Gonna see if I can figure out the changes myself and make a PR so that Margmas doesn't have to add more to their plate.

For now, the current version on Thunderstore is the best I can do. Configuration changes on the dedicated server made in-game now persist after logging out and re-connecting but they aren't written to disk until you shut the server down. So if you edit the server configuration file manually it will overwrite the in-game changes you made.

dreamwraith commented 10 months ago

Thanks, can confirm the updated behavior with one exception: I don't see changes saved to disk on shutdown. This could come down to how I am running the server. I am running on a linux host. The server is being sent a kill -2 signal for shutdown, but it doesn't seem to be triggering a safe shutdown perhaps. I will do some looking on that, but if you are seeing it save in other environments (eg windows server) then It's most likely a me-problem.

Nonetheless, I appreciate your working with me on this and the other issue. As a software dev myself by day, I understand how solving problems like this can be super tough, especially when you are trying to meet the needs of many different users with different wants and needs. I try not to do too much software work in my leisure time, so I am always appreciative of others who are willing to share their work with others like this. It does inspire me a bit to perhaps dig in a bit more myself, if only to help solve some bugs here and there. You're more than welcome to use any screenshots I send, and I will send some more your way later this evening :)

I've also sent you a little thank you as well, just in appreciation of how courteous and accommodating you've been.

dreamwraith commented 10 months ago

I can confirm the save issue is related to my pterodactyl egg/yolk not properly sending a SIGINT to the valheim server process when trying to shutdown. I will likely migrate off of using pterodactyl in the next few months and just run my own docker containers to avoid issues like this. Pterodactyl is nice, but overkill for most people running game servers, and troublesome when you run into issues like this.

With that said, this issue has ended up morphing into more of a server config issue rather than the original issue/feature request. We can probably close this issue now, and i will post new ones when they come up (eg the issue with recipes on some "not enabled by default" prefabs.

searica commented 10 months ago

Oh, thank you! Glad that I could help and thanks for updating me about the server issue. I've finished writing the code for the pull request to Jotunn (just need some more thorough testing) and if the PR gets accepted and built into the next version of Jotunn then the server configuration file will save whenever it is changed rather than only on shutdown (no need to update MVBP once that happens though thanks to Jotunn being an external dependency).

I'll close this issue now but I'm looking forward to finding out about the different pickables and their issues.