MrRar / edit_skin

Advanced skin editor mod for Minetest
MIT License
7 stars 5 forks source link

Fix sfinv support to retain sfinv menus. #10

Open Skaapdev opened 5 months ago

Skaapdev commented 5 months ago

@MrRar Hi MrRar,

I have been looking at using edit_skin and my mod soup uses sfinv at this point in time. It is a small annoyance but when a player clicks on the edit_skin menu it doesn't actually use the sfinv formspec. So as a consequence there is no way to click back to other menus such as armour/craft etc. Well I happen to just figure out how to integrate some other mod "properly" with sfinv and while I was doing that I had a go at doing the same for edit_skin.

My changes are here: https://github.com/Skaapdev/edit_skin/commit/82458b17f83fd741ec1fdd18a5510ce9dfaa683a

That diff is quite hard to read, I am not sure I believe it. Better to just look at the raw file or do your own diff from that.

Unfortunately I am working from a fork of a fork, I suspect a PR would be inappropriate. Alas my understanding of both sfinv and edit_skins is suspect at the very least. However this is what I am going to use for the time being.

Thanks, Skaap

MrRar commented 5 months ago

I honestly didn't know sfinv had a size parameter. I thought it was just fixed size. I will look into this.

MrRar commented 5 months ago

I tried it out: https://github.com/MrRar/edit_skin/commit/0179946a30dc461a955e911daf30b0c3b06111b7

I'm running into two issues: The hack that I used to get the color sliders to work doesn't work very well with sfinv. It's probably possible but not super simple. Other issue is that it's impossible to know when an sfinv formspec is closed so the player skin has to be saved after every little change. I guess that's not so bad.

Skaapdev commented 5 months ago

@MrRar Hi MrRar,

Thank you for looking at this. I tried your branch, and it works well for me.

  1. the color sliders to work doesn't work very well with sfinv.

Could you be more specific about what part doesn't work? I tried both the sfinv and /skin form specs and could not find anything wrong with the color sliders.

Not related but, is it complicated to add the color sliders for the eyes?

  1. Other issue is that it's impossible to know when an sfinv formspec is closed so the player skin has to be saved after every little change. I guess that's not so bad.

What about just saving the skin on player logout? If that is not possible I would suggest some kind of cool down wrapper queue to saving.

Something like this pseudo code below:

skin_saved_cooldown = {}
skin_last_cache = {} -- Perhaps this isn't even needed?
...
--Before use:
if skin_saved_cooldown[player_name] == true then
    skin_last_cache[player_name] = `the data about the current skin`
    return 
end
skin_saved_cooldown[player_name] = true
minetest.after(5, function()
    edit_skin.save(player,  skin_last_cache[player_name])
    skin_saved_cooldown[player_name] = nil
end)

Kind Regards, Skaap

MrRar commented 5 months ago

Could you be more specific about what part doesn't work? I tried both the sfinv and /skin form specs and could not find anything wrong with the color sliders.

Maybe the fact that moving them does nothing? Anyway I added a new commit that got them working again. I had to do some hacky stuff to get them working. I also added some code in the latest commit that fixes the bug where if you update the player using /skin the changes don't show on the sfinv page. I feel like these changes are just too hacky to add such a minor feature so I probably wont add it to the master branch right now.

Not related but, is it complicated to add the color sliders for the eyes?

Not really. I made some things not colorable because I thought it wouldn't mater that much but some people care about it. Particularly the shoes and the eyes. I might add that some time.

adikalon commented 1 month ago

@MrRar what about implementation in unified_inventory?

MrRar commented 1 month ago

@MrRar what about implementation in unified_inventory?

I don't think it would really be possible. The area Unified Inventory provides for custom formspecs is way to small to fit Edit Skin. Plus there's already a button to get to Edit Skin from Unified Inventory.