Skip to content
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

Add migration code to player skins using '.' delimiters #105

Merged
merged 4 commits into from
Jun 15, 2024

Conversation

SmallJoker
Copy link
Member

@SmallJoker SmallJoker commented Jun 9, 2024

Previously, the players would have their selected skin reset. See 'skins.__fuzzy_match_skin_name' for a detailed explanation.

This also fixes an issue where "player.[name].[number].png" skins were not recognized.

Fixes #101

How to test: modify the mod storage file to use "character_123" or "player_singleplayer_343" skin names but have files named "character.123.png" and "player.singleplayer.343.png".

@Bastrabun Please confirm whether this fixes your issues.

Previously, the players would have their selected skin reset.
See 'skins.__fuzzy_match_skin_name' for a detailed explanation.

This also fixes an issue where player.[name].[number].png skins
were not recognized.
@SmallJoker SmallJoker force-pushed the pr_105_delim_migration_code branch from da973e7 to 00e5696 Compare June 10, 2024 16:31
@SmallJoker
Copy link
Member Author

@Bastrabun While writing some unittests, I realized that I was going into the wrong direction. Reconstructing the old file name based on the new format turns out to be much more reliable for the cases that I've tested so far. Please let me know which specific unittests you'd like to add.

@@ -2,7 +2,8 @@ local dbgprint = false and print or function() end

--- @param path Path to the "textures" directory, without tailing slash.
--- @param filename Current file name, such as "player.groot.17.png".
local function process_skin_texture(path, filename)
--- @return On error: false, error message. On success: true, skin key
function skins.register_skin(path, filename)
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Will be added to the API documentation in a later PR.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  1. The migration seems to work, I could not find a case where it would not. OK
  2. register_skin is now public and correctly adds the skin from a different mod (meta not tested) OK
  3. Private skins show up in the list OK

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I assume list point "3." is OK too? 🙃

In either case - thank you for testing. If there's no further issues found, I'll go ahead merging this PR in about 24 hours.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not entirely certain about point 3 - that's the hand key ambiguity. I looked at the key again and found it still ambiguous, but I thought that's because this issue wasn't marked addressed in this PR.

My goal is to externalize skins from this mod, so we can go "full upstream", while maintaining our skins in a worldmods folder #31 and having a way to retrofit skins into a running server (currently blocked by the problem that the hand is a new block, which cannot be registered during runtime)

@SmallJoker SmallJoker merged commit 11bb5ba into master Jun 15, 2024
2 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Previously selected player skins are now reset to default
2 participants