mt-mods / travelnet

Network of teleporter-boxes that allows easy travelling to other boxes on the same network. (Mod for Minetest)
https://content.minetest.net/packages/mt-mods/travelnet/
GNU General Public License v3.0
7 stars 12 forks source link

Attempt to repair the station each time the primary formspec is opened #54

Closed oversword closed 3 months ago

oversword commented 2 years ago

This PR runs the repair action each time the form is opened, except when it has not yet been configured.

I also fixed the repair logic and added in a condition to stop the max network count being exceeded, or a duplicate network being added. Any error when re-attaching the station will be reported via chat, and the primary formspec shown anyway so the player can remove the station if needed.

In my opinion https://github.com/mt-mods/travelnet/pull/52 (adding a repair button) is much preferred because:

All these things improve the user experience of re-attaching the network

S-S-X commented 2 years ago

Only someone with permissions can run the repair logic

Use travelnet.allow_attach. API function is currently in bit bad shape as it requires knowledge of internal implementation but check itself is very simple, bit surprised why it is not already here.

The repair logic is run intentionally, not all the time

Should be run when network is broken. Best situation would be if networks would not be broken from beginning, automatic repair simply makes it look like that until there's conflict (you're handling conflicts right?).

Formspec error messages can be used, instead of a mix of formspec & chat

If wanted this can be redone to use formspec error messages.

oversword commented 2 years ago

Use travelnet.allow_attach. API function is currently in bit bad shape as it requires knowledge of internal implementation but check itself is very simple, bit surprised why it is not already here.

That's not the original behaviour, I thought you wanted me to re-implement the original behaviour exactly?

Should be run when network is broken. Best situation would be if networks would not be broken from beginning, automatic repair simply makes it look like that until there's conflict (you're handling conflicts right?).

There is a condition that checks if it's broken, but that's not the point. What if you want to leave it (temporarily) broken so you can go and fix the network elsewhere? Doing it automatically for the player could cause confusion in the event of data loss. Fixing it automatically on interaction will not make it look like networks aren't broken, if more than one station is lost then when you interact with a particular station it will look like the other stations are simply gone without knowing why - "THIS station looks fine (because it repaired itself), so why are the others missing?" If instead we informed the player and got them to manually fix the problem, they would know they have to go and fix it for other stations too.

If wanted this can be redone to use formspec error messages.

No, it can't. As I mentioned on the other issue this could cause some travelnets to become inoperable

https://github.com/mt-mods/travelnet/pull/52

doing this on every formspec open could cause the travelnet to become inoperable: if you had a network that lost a station somehow, then added more stations to reach the limit, then tried to repair a station while opening the formspec you would instead get an error about the max station count, meaning you could never remove the station because you can't open the formspec to do so.

S-S-X commented 2 years ago

I guess I'll have to do implementation to explain what my proposal is, I can try to find time for that. Probably easier to understand than trying to explain what behavior I'm thinking. Carefully reading comments on #52 should however give basic idea.

BuckarooBanzay commented 1 year ago

bump! is this relevant? with the move to modstorage as backend i don't think this is needed :thinking:

S-S-X commented 1 year ago

If everything for server side network lookup and box lookup is in the db then this shouldn't be needed, it was anyway more about temporary workaround to make things seem fine and hopefully restore after certain problems with data storages.

This also isn't really a good way for client side stuff / restoring from crippled / partial backup. For that I'm pretty sure either configurable LBM or asking for actual DB would probably be best options. Both can be completely isolated and allow complete db reconstruction in special cases where it is needed.

I guess there's already solution for tn-box removal that skips normal callbacks and db writes or was it added back after refactoring things? If it wasn't yet restored then that part could be actually useful now.

BuckarooBanzay commented 1 year ago

I guess there's already solution for tn-box removal that skips normal callbacks and db writes or was it added back after refactoring things? If it wasn't yet restored then that part could be actually useful now.

i don't think we have such a thing yet :thinking:

Niklp09 commented 3 months ago

Closing since this is outdated and marked as invalid. As always, re-open if needed.