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

Allow overriding the hand #2827

Closed
wants to merge 1 commit into from

Conversation

Projects
None yet
@TeTpaAka
Copy link
Contributor

commented Jun 25, 2015

You can set a custom hand by assigning an item to the "hand" inventory list.
This can be used for different appearance and for different player classes.
Where should I document this behaviour?
Fixes #1368

@est31

View changes

src/content_sao.cpp Outdated
}
bool PlayerSAO::setWieldedItem(const ItemStack &item)
{
if(Inventory *inv = getInventory()) {

This comment has been minimized.

Copy link
@est31

est31 Jun 25, 2015

Contributor

Space :)

This comment has been minimized.

Copy link
@TeTpaAka

TeTpaAka Jun 25, 2015

Author Contributor

Fixed.

@est31

View changes

src/content_sao.cpp Outdated
return true;
}
}
mlist->changeItem(getWieldIndex(), item);

This comment has been minimized.

Copy link
@est31

est31 Jun 25, 2015

Contributor

This isn't tab indented, following line too

This comment has been minimized.

Copy link
@TeTpaAka

TeTpaAka Jun 25, 2015

Author Contributor

Fixed.

@TeTpaAka TeTpaAka force-pushed the TeTpaAka:hand_replacement branch 2 times, most recently Jun 25, 2015

@TeTpaAka

This comment has been minimized.

Copy link
Contributor Author

commented Jun 25, 2015

Copying turned some tabs to spaces.

@est31

View changes

src/content_sao.cpp Outdated
}
return ret;
}
bool PlayerSAO::setWieldedItem(const ItemStack &item)

This comment has been minimized.

Copy link
@est31

est31 Jun 25, 2015

Contributor

Why do we need a setter? Its not used anywhere.

This comment has been minimized.

This comment has been minimized.

Copy link
@est31

est31 Jun 25, 2015

Contributor

I see. Pretty weird place to put it.

This comment has been minimized.

Copy link
@TeTpaAka

TeTpaAka Jun 25, 2015

Author Contributor

No. setWieldedItem is used for the wielded item, not only for the hand. I need to override it, because else, you might get a replacement for the hand in your inventory, if you use a hand that changes when used.

This comment has been minimized.

Copy link
@est31

est31 Jun 25, 2015

Contributor

but mobs have no wielded item??
Its just weird that the abstract ServerActiveObject class has support for this. (Outside of the scope of this PR)

This comment has been minimized.

Copy link
@TeTpaAka

TeTpaAka Jun 27, 2015

Author Contributor

Yeah, true. And they shouldn't have a wielded list, too. "" is the only thing it returns for non-players.
But that should be fixed by someone who knows what he is doing.

@est31

This comment has been minimized.

Copy link
Contributor

commented Jun 25, 2015

Documentation... good question. I guess this needs its own section, somewhere around here. Perhaps its enough to simply name them in brackets "(with main and hand inventory lists)"

@TeTpaAka

This comment has been minimized.

Copy link
Contributor Author

commented Jun 25, 2015

craft and craftpreview aren't documented either. Maybe we need a new category for the different inventory lists of players?

@est31

This comment has been minimized.

Copy link
Contributor

commented Jun 25, 2015

Yea

@est31 est31 added the enhancement label Jun 25, 2015

@TeTpaAka TeTpaAka force-pushed the TeTpaAka:hand_replacement branch 2 times, most recently Jun 25, 2015

@MirceaKitsune

This comment has been minimized.

Copy link
Contributor

commented Jun 28, 2015

Thank you for creating this pull request! This fixes a limitation which heavily affects my Creatures mod, and I strongly welcome the change. As users might know, Creatures allows players to become various species (from mice to sheep to monsters to humans), and not being able to customize the wielded hand per player in realtime has been an obvious visual problem since the existence of the mod.

Apart from my mod, this is also important for skin mods. Some skins might want to customize the wield hand... for example, if a character skin has gloves you want the wield hand to reflect that.

@TeTpaAka

This comment has been minimized.

Copy link
Contributor Author

commented Jun 28, 2015

I don't know how good this would be for individual skins. The way it is done, is, it sets a tool as an override for the hand. This way, not only the appearance may differ but also the abilities. But you need to define a new tool for every new appearance.

@est31

This comment has been minimized.

Copy link
Contributor

commented Jun 28, 2015

Yes this is a problem, but having this is better than nothing.

@MirceaKitsune

This comment has been minimized.

Copy link
Contributor

commented Jun 28, 2015

@TeTpaAka Actually, the hand also having different abilities is an additional feature if anything. You can't customize the damage you do with the wielded hand per player either, and this solves that limitation as well.

@paramat

This comment has been minimized.

Copy link
Member

commented Jun 29, 2015

I'm not qualified to comment on implementation, but a very useful feature, for example spacesuit glove in a space/moon mod.

@est31 est31 force-pushed the minetest:master branch to a890c66 Aug 2, 2015

@FreeLikeGNU

This comment has been minimized.

Copy link

commented Aug 26, 2015

👍 for this to have gloves, play as mobs, maybe even emotes (point, wave, etc)

@sfan5

View changes

src/content_sao.cpp Outdated
ret = hlist->getItem(0);
}
}
return ret;

This comment has been minimized.

Copy link
@sfan5

sfan5 Dec 3, 2015

Member

Isn't ret used uninitalized if inv is NULL?

This comment has been minimized.

Copy link
@TeTpaAka

TeTpaAka Dec 3, 2015

Author Contributor

I think, a PlayerSAO has always an Inventory. At least, this is how it was used before. So I should probably remove the if condition.

This comment has been minimized.

Copy link
@HybridDog

HybridDog Dec 4, 2015

Contributor

You could also write return hlist->getItem(0); instead of setting ret to it and returning it later, can't you?

This comment has been minimized.

Copy link
@TeTpaAka

TeTpaAka Dec 4, 2015

Author Contributor

No. ret is only set to hlist->getItem(0) if the name of the current selected item of the hotbar is "".
See #2827 (diff)

This comment has been minimized.

Copy link
@HybridDog

HybridDog Dec 4, 2015

Contributor

l don't mean removing the if things.

+   const Inventory *inv = getInventory();
+   ItemStack ret;
+   if (inv) {
+       const InventoryList *mlist = inv->getList(getWieldList());
+       if (mlist && (getWieldIndex() < (s32)mlist->getSize()))
+           return mlist->getItem(getWieldIndex());
+       if (ret.name == "") {
+           const InventoryList *hlist = inv->getList("hand");
+           if (hlist)
+               return hlist->getItem(0);
+       }
+   }
+   return ret;

This comment has been minimized.

Copy link
@TeTpaAka

TeTpaAka Dec 4, 2015

Author Contributor

Yeah, I could. But both versions are equally good readable and the change wouldn't improve anything.

This comment has been minimized.

Copy link
@HybridDog

HybridDog Dec 4, 2015

Contributor

ok

This comment has been minimized.

Copy link
@TeTpaAka

TeTpaAka Dec 4, 2015

Author Contributor

Fixed.

@TeTpaAka TeTpaAka force-pushed the TeTpaAka:hand_replacement branch 2 times, most recently Dec 4, 2015

@C1ffisme

This comment has been minimized.

Copy link

commented Feb 15, 2016

👍 If it works as @paramat said.

@nerzhul

View changes

src/content_sao.cpp Outdated
if (Inventory *inv = getInventory()) {
if (InventoryList *mlist = inv->getList(getWieldList())) {
ItemStack olditem = mlist->getItem(getWieldIndex());
if (olditem.name == "") {

This comment has been minimized.

Copy link
@nerzhul

nerzhul Feb 16, 2016

Member

name.empty()

@nerzhul

View changes

src/game.cpp Outdated
@@ -3282,6 +3282,12 @@ void Game::updateCamera(VolatileRunFlags *flags, u32 busy_time,
if (mlist && client->getPlayerItem() < mlist->getSize())
playeritem = mlist->getItem(client->getPlayerItem());
}
if (playeritem.getDefinition(itemdef_manager).name == "") { // override the hand

This comment has been minimized.

Copy link
@nerzhul

nerzhul Feb 16, 2016

Member

name.empty()

@nerzhul

View changes

src/game.cpp Outdated
@@ -3282,6 +3282,12 @@ void Game::updateCamera(VolatileRunFlags *flags, u32 busy_time,
if (mlist && client->getPlayerItem() < mlist->getSize())
playeritem = mlist->getItem(client->getPlayerItem());
}
if (playeritem.getDefinition(itemdef_manager).name == "") { // override the hand
InventoryList *hlist = local_inventory->getList("hand");

This comment has been minimized.

Copy link
@nerzhul

nerzhul Feb 16, 2016

Member

you can add this to the next condition

@TeTpaAka TeTpaAka force-pushed the TeTpaAka:hand_replacement branch 2 times, most recently Feb 17, 2016

@TeTpaAka

This comment has been minimized.

Copy link
Contributor Author

commented Feb 17, 2016

Fixed.

@Zeno-

This comment has been minimized.

Copy link
Contributor

commented Nov 24, 2016

Once the minor style issues (declaration inside expression of if statements) have been addressed 👍

@TeTpaAka TeTpaAka force-pushed the TeTpaAka:hand_replacement branch Nov 24, 2016

@TeTpaAka

This comment has been minimized.

Copy link
Contributor Author

commented Nov 24, 2016

@paramat was right: It didn't work out of the box. I added the "hand" inventory list manually in Lua, which worked, but forgot to mention it in the documentation. Now I add the "hand" inventory list directly in C++, so it should work now.

I tried to fix the style issues, hope I didn't miss any.
If everything is alright, I'll squash the commits.

@Zeno-

This comment has been minimized.

Copy link
Contributor

commented Nov 24, 2016

I'm ok with this now. Official 👍

@TeTpaAka TeTpaAka force-pushed the TeTpaAka:hand_replacement branch to bedec87 Nov 24, 2016

@TeTpaAka

This comment has been minimized.

Copy link
Contributor Author

commented Nov 24, 2016

Squashed.

@MirceaKitsune

This comment has been minimized.

Copy link
Contributor

commented Nov 24, 2016

I didn't check the technical aspects yet, but I fully support this feature, and in fact have been needing it for one of my mods for years now! Definitely a reinforcement of my 👍

@paramat

This comment has been minimized.

Copy link
Member

commented Nov 24, 2016

Will test again and hopefully +1.

@paramat

This comment has been minimized.

Copy link
Member

commented Nov 24, 2016

@TeTpaAka
It works, but i have a request.
This code is my test for changing the hand in-game, it randomly switches the hand bwtween sand and stone:

minetest.register_item("hands:sand_hand", {
    type = "none",
    wield_image = "default_sand.png",
    wield_scale = {x = 1, y = 1, z = 5},
	stack_max = 1,
    tool_capabilities = {
        full_punch_interval = 0.9,
        max_drop_level = 0,
        groupcaps = {
            crumbly = {times={[2]=3.00, [3]=0.70}, uses=0, maxlevel=1},
            snappy = {times={[3]=0.40}, uses=0, maxlevel=1},
            oddly_breakable_by_hand = {times={[1]=3.50,[2]=2.00,[3]=0.70}, uses=0}
        },
        damage_groups = {fleshy=1},
    }
})

minetest.register_item("hands:stone_hand", {
    type = "none",
    wield_image = "default_stone.png",
    wield_scale = {x = 1, y = 1, z = 5},
	stack_max = 1,
    tool_capabilities = {
        full_punch_interval = 0.9,
        max_drop_level = 0,
        groupcaps = {
            crumbly = {times={[2]=3.00, [3]=0.70}, uses=0, maxlevel=1},
            snappy = {times={[3]=0.40}, uses=0, maxlevel=1},
            oddly_breakable_by_hand = {times={[1]=3.50,[2]=2.00,[3]=0.70}, uses=0}
        },
        damage_groups = {fleshy=1},
    }
})

minetest.register_globalstep(function(dtime)
	if math.random() < 0.05 then
		for _, player in ipairs(minetest.get_connected_players()) do
			if math.random() < 0.5 then
				player:get_inventory():remove_item("hand", "hands:stone_hand")
				player:get_inventory():add_item("hand", "hands:sand_hand")
			else
				player:get_inventory():remove_item("hand", "hands:sand_hand")
				player:get_inventory():add_item("hand", "hands:stone_hand")
			end
		end
	end
end)

Altering the hand once is easy because the 'hand' inventory is empty.

Altering the hand many times in-game is more difficult because you must first remove what is already in the 'hand' inventory, and to do that you need to work out what is already there, is there an easy way to clear the 'hand' inventory without knowing what is there?
It is also easy to clutter up the 'hand' inventory with multiple identical items, so it is necessary to set stack_max = 1.
Is there a way your code can automatically clear the 'hand' inventory when a new item is assigned? This would move some work from Lua to C++ and make the changeover simpler to mod. Then all we need to do is 'add item'.

@TeTpaAka

This comment has been minimized.

Copy link
Contributor Author

commented Nov 24, 2016

What I did to test it was:
player:get_inventory():set_stack("hand", 1, ItemStack("handtest:testhand"))
So, there is already a built in method to switch a certain spot in an inventory with a different item.

Edit:
You can directly insert the string in the code without ItemStack(...), I just misread the documentation of the function.

@paramat

This comment has been minimized.

Copy link
Member

commented Nov 25, 2016

Thanks works well.

@paramat

This comment has been minimized.

Copy link
Member

commented Nov 25, 2016

I guess to return to the normal hand we do:
player:get_inventory():set_stack("hand", 1, "")

@TeTpaAka

This comment has been minimized.

Copy link
Contributor Author

commented Nov 25, 2016

Yes. This would clear the hand slot and thus return to the current behavior.
The code first checks the inventory for the wielditem. If there is none, or the item can't interact with the environment, it checks the hand slot. If the hand slot is empty, it uses the default hand. (Notice, if there is an item in the hand slot, it never falls back to the default hand.)

@paramat

This comment has been minimized.

Copy link
Member

commented Nov 26, 2016

👍 I think amongst all who have looked at this we have enough approval now.

@garywhite207

This comment has been minimized.

Copy link

commented Nov 26, 2016

+1 I think it'd be cool to have 2 switchable hands in Minetest

@paramat

This comment has been minimized.

Copy link
Member

commented Nov 26, 2016

785a9a6
Thanks so much for this, long overdue.

@micheal65536

This comment has been minimized.

Copy link

commented Aug 20, 2017

Attempting to change the "hand" inventory slot causes the client (not the server) to segfault for me when the player switches to an empty slot on their hotbar. Minetest 0.4.16

@paramat

This comment has been minimized.

Copy link
Member

commented Aug 20, 2017

Please open an issue about this in https://github.com/minetest/minetest/issues

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.