minetest-mods / skinsdb

Player skin mod, supporting unified_inventory, sfinv and smart_inventory
28 stars 41 forks source link

Clean up skin listing #100

Closed SmallJoker closed 1 month ago

SmallJoker commented 3 months ago

Supersedes the 'fsep' setting by automatically detecting the texture name in both, the skin list and the updater scripts. New, automatically fetched skins will now always use the '.' delimiter to avoid player name issues.

How to test

  1. Create textures according to the documented naming in "textures/readme.txt"
  2. Create legacy named textures
  3. Player-specific skins must still be listed
SmallJoker commented 2 months ago

@Niklp09 is it later yet?

blaboing commented 2 months ago

did a quick test and it failed for playername "test_1" with

player_test_1.png
player_test_1_1.png

The idea behind fsep was to gently force existing servers to abonden the _ seperator completly to avoid the playername problem. So imo it would be better to deprecate _ and change to . as default first before removing _ completly later. But either way I don´t think its a good idea to remove all information regarding _ as seperator especially if you still want to support it.

SmallJoker commented 2 months ago

@blaboing As long the default is _, the servers will never change because it "generally works". skinsdb should at least be capable to deal with both formats automatically to allow a seamless transition. Perhaps that particular issue could be solved by adding the skin twice:

What do you think about that?

blaboing commented 2 months ago

Adding a mixed seperator mode will make it even more unlikely that servers will switch. I would force them to switch to . by making it default and spam debug.txt with a warning if fsep is still set to _ for every single skin and/or even bug the players on login that they should tell the admin that their skin uses an deprecated filename pattern😄 After a while or a couple releases/updates remove _ completly except for a fsep deprecation warning.

Adding the same player_ skin to 2 different playernames would also allow servers to just keep ignoring it while at the same time going against that personal per player skin idea if anyone could just register as playername_1 and then use that skin.

SmallJoker commented 2 months ago

Adding a mixed seperator mode will make it even more unlikely that servers will switch.

The trick here is that the transition will happen automatically. Newly installed skins (e.g. new installations) will use the dot delimiter. Over time, the old naming scheme will disappear without much interaction needed by admins. If we want to speed things up, a warning could still be added.

SmallJoker commented 1 month ago

Now a warning message is logged in case of an ambiguous skin name. It will still be assigned to the shorter player name for simplicity. I think this is an acceptable compromise.

SmallJoker commented 1 month ago

Kept this PR idle for two weeks to collect further inputs. If there are objections in the next few days, I will ahead to merge this PR.