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

Dual wielding (revival) #14427

Open
wants to merge 6 commits into
base: master
Choose a base branch
from

Conversation

Andrey2470T
Copy link
Contributor

@Andrey2470T Andrey2470T commented Mar 2, 2024

As it is been more than a month since the original PR (#11016) was closed for a reason of the long inactivity of the author and it has not been opened so far, I've decided to take over it. The original commit history and authors priority was taken into consideration.

To do

  • Repair set/get_wielded_item() when used inside on_place/on_secondary_use callbacks.
  • Set offhand list on default.
  • Add is_offhand parameter to end of the callbacks to distinguish the passed itemstacks from the main and offhand.
  • Implement some sfan5's suggestions from the last review of the closed PR.

How to test

Test the following code:

-- Creating 'offhand' list after joining
minetest.register_on_joinplayer(function(player)
	player:get_inventory():set_size("offhand", 1)
end)

-- The registration of the command implementing swapping itemstacks in two hands
minetest.register_chatcommand("swap", {
	help = "Swap hands",
	func = function(name)
		local player = minetest.get_player_by_name(name)

		if not player then
			return false, "Need to be online"
		end

		local inv = player:get_inventory()

		local main = player:get_wielded_item()
		local offhand = inv:get_stack("offhand", 1)

		inv:set_stack("offhand", 1, main)
		player:set_wielded_item(offhand)
	end,
})

@appgurueu appgurueu added @ Server / Client / Env. Feature ✨ PRs that add or enhance a feature Concept approved Approved by a core dev: PRs welcomed! labels Mar 2, 2024
@Zughy Zughy marked this pull request as draft March 2, 2024 23:05
@cx384
Copy link
Contributor

cx384 commented Mar 3, 2024

In my opinion, hardcoded dual wielding is too inflexible. The engine could support all kinds of different hands, and number of hands.

Wouldn't it be way better to have a wieldhand/wielditem Lua HUD element?
It could support changing the following things:

  • Position
  • Direction / Mirrored
  • Animation definition
  • Flags for showing the animation on leftclick/rightclick/shift+click ...
  • Image/Mesh Model/Inventory Location
  • z_index to draw other HUD element above/below
  • ...

Imagine for example a game in which you control a spaceship that can shoot.
The space gun could then be such a HUD element which is centered at the bottom of the screen, and shows a nice animation when you click to shoot.

Another example would be playing as a creature which has 4 or more hands that all show an animation when digging something.

(If necessary, dual wielding could then be easily implemented in builtin.)

@sfan5
Copy link
Member

sfan5 commented Mar 3, 2024

In my opinion, hardcoded dual wielding is too inflexible. The engine could support all kinds of different hands, and number of hands.

At the time we (coredevs) decided that this was nonsense and we'd rather have the incremental improvement of two hands than some theoretical many hands feature that isn't realistic to happen.

Also, much of this PR concerns itself with how a second hand works for item interactions. A HUD element does not address that.

@cx384
Copy link
Contributor

cx384 commented Mar 3, 2024

Also, much of this PR concerns itself with how a second hand works for item interactions. A HUD element does not address that.

Of course, you would need additional functions to handle how placing and digging works, e.g.

-- Ether "wield_index" for using the wield index or a static position.
player:set_dig_source("main", "wield_index")
player:set_place_source("offhand", 1)

(This would also be useful for the hotbar, and I think there are already 2 issues which can be solved by this.)

Also, I found at least 5 other issues which could be solved by a wieldhand HUD element.

(I don't want to link all the issues here.)

Maybe I should make a PR myself to show that it is "realistic to happen", but first someone should review my hotbar HUD element PR.

@lhofhansl
Copy link
Contributor

In my opinion, hardcoded dual wielding is too inflexible. The engine could support all kinds of different hands, and number of hands.

Let's not have this discussion again. While I agree in principle, I take a less flexible solution that works and - crucially - exists over a pie-in-sky perfect solution that we will likely never get, any day!

We can make sure that we do add anything that cannot be extended in the future.

@Andrey2470T
Copy link
Contributor Author

In my opinion, hardcoded dual wielding is too inflexible. The engine could support all kinds of different hands, and number of hands.

"Multi-handedness" is too specific feature for me. Where would you want to use it? Do you want your character to be a octopus? Note besides the obscure usecases of it, that would also significantly harden the interaction mechanism with the world through the callbacks on_place/on_secondary_use and cause more breakages than two hands can provide.

Wouldn't it be way better to have a wieldhand/wielditem Lua HUD element? It could support changing the following things:

  • Position
  • Direction / Mirrored
  • Animation definition
  • Flags for showing the animation on leftclick/rightclick/shift+click ...
  • Image/Mesh Model/Inventory Location
  • z_index to draw other HUD element above/below
  • ...

Imagine for example a game in which you control a spaceship that can shoot. The space gun could then be such a HUD element which is centered at the bottom of the screen, and shows a nice animation when you click to shoot.

Another example would be playing as a creature which has 4 or more hands that all show an animation when digging something.

(If necessary, dual wielding could then be easily implemented in builtin.)

Actually we made the same conclusion yesterday in Discord during the discussion on this PR. As Herowl suggested, it is necessary to add a new HUD type, say "mesh", allowing to render a mesh on the screen with some manipulation like screen-space positioning, rotatiting, scaling, affecting the lighting, animating and etc. And then dual wielding could be based off that. Also, as it was stated yesterday, it is necessary to do refactoring in the creating/handling wield nodes part.

@cx384
Copy link
Contributor

cx384 commented Mar 4, 2024

I really don't want to continue this discussion here, but also don't want to keep your questions unresponded.
So I hope this clarifies things and settles the discussion.

"Multi-handedness" is too specific feature for me.

Dual wielding is also a kind of "Multi-handedness" as you call it, and yes it is specific. Especially since IIRC the behavior of its current implementation is mainly designed to be applicable for a Minecraft clone (MineClone2).
(I don't expect anything else.)

Where would you want to use it? Do you want your character to be a octopus?

I already gave two examples. Another example would be playing a machine which has 3 "hands", e.g. 2 drills and one gripper. An octopus would fit into the creatures example, and playing with 8 tentacles which show an animation when you dig something would probably look quite nice.

Note besides the obscure usecases of it, that would also significantly harden the interaction mechanism with the world through the callbacks on_place/on_secondary_use and cause more breakages than two hands can provide.

Breakages wouldn't be much different from dual wielding. The main problem is how the current wield hand is determined.
So also for your PR, I suggest to make a clear distinction between:

  • wieldhand which is the hand that a player last used. (changed after every click to place/dig.)
  • and hand which is just any hand.

This way you can keep get_wield_list(), get_wield_index(), get_wielded_item() and set_wielded_item(item)
without introducing much breakage. (You will need new functions to access a specific hand.)

Then callbacks e.g. on_place would always use the current wieldhand which caused the callback to be executed.

NOTE:
This only causes breakage, if a mod does not use get_wield_list() to determine the wield list before setting the item.
(For the index it's not a problem, since in previous versions it can change anytime too.)
However, this problem is not restricted to dual wielding!
When #8297, #11723 or similar will be solved, which is very likely to happen eventually, causing this breakage is inevitable.

Actually we made the same conclusion yesterday in Discord during the discussion on this PR.

Too bad you can't link the discussion here such that anyone who isn't singed up for Discord can read it.
(This is not a suggestion for you to try to do this now.)

Contradictory to what I previously wrote, I think for a HUD element for wield hands, it would probably be better to just have an inventory source and visuals which only/mostly depend on the itemstack. So it would be better to make the animation a part of the item definition. A separate element type "mesh" could then still be added.

To conclude, just do what the cordevs say (not me) and pay attention to what has been discussed in the two previous PRs and the Issue. Also, it would be good if you try to not introduce too much hardcoded things that will be difficult to generalize at a later point.

@appgurueu
Copy link
Contributor

My two cents:

  • A proper HUD wield mesh element is not "some theoretical many hands feature"; it is not restricted to hands at all. It is just a feature to give modders more control about what to display on the screen.
  • Such a feature is not unrealistic. We already have two related features: (1) the model[] formspec element and (2) first-person attachments. The problem is just that (1) requires a formspec and (2) is in-world (not always on top), and visible to all players (though the observability PR could change that). Also related: @v-rob's HUD / formspec replacement. HUDs and formspecs have significant overlaps - everything that is visual-only (images, text, layouting) applies to both. And even on-screen interactive elements like buttons make sense on touch displays (and are a soft prerequisite for exposing controls to modders, along with SSCSM to be able to actually smoothly make use of the new controls outside of singleplayer, but I'm getting off track and into long-term plans here).
  • Such a feature is indeed not equivalent to a "second hand", but a second hand (and the first hand) could be implemented well and de-hardcoded in terms of such a feature, which would likely be cleaner and more flexible.

With that in mind, we might still opt for merging the "dual wielding" feature first, incurring some technical debt for when / if the HUD wield mesh feature is to be implemented, because (1) the dual wielding code is largely already there (2) we probably don't want to delay this "too much".

@lhofhansl
Copy link
Contributor

Thanks @appgurueu

With that in mind, we might still opt for merging the "dual wielding" feature first, incurring some technical debt for when / if the HUD wield mesh feature is to be implemented, because (1) the dual wielding code is largely already there (2) we probably don't want to delay this "too much".

+1 on that.

@Andrey2470T
Copy link
Contributor Author

So from the whole discussion that was here and on Discord I can make the following conclusive points:

  • A new HUD type like "mesh" should be added for displaying on the screen some arbitrary mesh or an item that would look like any wielded item currently from the main hand. However, not to leave this PR open and unfinished at an unknown period due to a need of refactoring the wielded hand code and adding a new type, as appgurueu suggested, it is better to defer it in a exchange for a tech debt.
  • At this moment, it is necessary to solve the backwards compatibility. I see the most rational solution for that is refactoring player:set_wielded_item() that would be aware of a current used hand and would set a new itemstack for that hand. on_place/on_secondary_use should be left as they are. They will accept itemstack parameter based on the current context.
  • It is worth to add also a new parameter for those callbacks like 'offhand = true/false' determining whether this callback is called from the itemstack of the offhand or not. Necessary for distinguishing the hands from each other.

@Andrey2470T Andrey2470T marked this pull request as ready for review March 9, 2024 17:27
@Andrey2470T
Copy link
Contributor Author

A following code testing the correct work of set_wielded_item() inside the on_place and on_secondary_use callbacks:

-- Note: the offhand list is already created on default,
-- so this code is commented as it is actually unnecessary
--minetest.register_on_joinplayer(function(player)
--	player:get_inventory():set_size("offhand", 1)
--end)

-- Note: add textures with these names or your own ones in the textures folder!
-- For fast swapping items in hands, in this PR you can press 'F' key.
minetest.register_craftitem("second_hand:swap_item_aspen", {
	description = "Swap Item Aspen",
	inventory_image = "multidecor_aspen_wood.png",
	on_place = function(itemstack, placer, pointed_thing) --on_secondary_use = function(itemstack, user, pointed_thing)
                -- Use both variants for testing (1) and (2) in the mainhand and offhand.
                -- The results should be same.
		user:set_wielded_item(ItemStack("second_hand:swap_item_laminate")) -- (1)

		--return ItemStack("second_hand:swap_item_laminate") -- (2)
	end
})

minetest.register_craftitem("second_hand:swap_item_laminate", {
	description = "Swap Item Laminate",
	inventory_image = "multidecor_laminate.png",
	on_place = function(itemstack, placer, pointed_thing) --on_secondary_use = function(itemstack, user, pointed_thing)
                -- Use both variants for testing (1) and (2) in the mainhand and offhand.
                -- The results should be same.
		user:set_wielded_item(ItemStack("second_hand:swap_item_aspen")) -- (1)

		--return ItemStack("second_hand:swap_item_aspen") -- (2)
	end
})
`

@sfan5 sfan5 added the Rebase needed The PR needs to be rebased by its author. label Mar 20, 2024
@lhofhansl
Copy link
Contributor

Do we need to revive the revival? :)

@Andrey2470T
Copy link
Contributor Author

Do we need to revive the revival? :)

Yes. That has been revived also now.

@sfan5 sfan5 removed the Rebase needed The PR needs to be rebased by its author. label Mar 23, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Concept approved Approved by a core dev: PRs welcomed! Feature ✨ PRs that add or enhance a feature @ Server / Client / Env.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants