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

New jumping behavior controlled by physics override #9980

Closed
wants to merge 11 commits into from

Conversation

oilboi
Copy link
Contributor

@oilboi oilboi commented Jun 2, 2020

Goal of the PR

  1. To create a new physics override that allows players and modders to select if they want the new consistent jumping code or the old floaty one.

  2. To fix the bug that allows players to jump up nodes irregularly and essentially negate having to have stairs by making "staircases" of nodes.

  3. To fix the glitch that players cannot jump on bouncy nodes unless coming to a halt.

How does the PR work?

I've added in a new physics override new_jump.
It is on by default, but players and modders can select between the new and old code using:

player:set_physics_override({new_jump=boolean})

Here is a video: https://youtu.be/Jt7QFyiINbM

Status

Ready for Review.

  • Make new_jump a physics_override.
  • Fix jumping to be consistent. (landing on node completely)
  • Fix not being able to keep bounce-jumping on bouncy node.

How to test

Create a mod with this in the init.lua:

local test = true
local function swap_physics()
	for _,player in ipairs(minetest.get_connected_players()) do
		player:set_physics_override({new_jump=test})
		minetest.chat_send_all("new_jump: "..tostring(test))
	end
	test = not test
	minetest.after(10,function()
		swap_physics()
	end)
end

swap_physics()

Then bounce and jump on nodes.

EDIT by SmallJoker: Format the post.

@TheTermos
Copy link
Contributor

A new jump option for the new move option.

The root cause is player can't jump a full node, instead a step up event is triggered each time slingshotting them upwards.
Imo the universal solution would be to increase jump velocity and turn off stepheight while in air (or only reduce significantly if we want to allow walking over gaps)

doc/lua_api.txt Outdated
@@ -6092,6 +6092,9 @@ object you are working with still exists.
(default: `false`)
* `new_move`: use new move/sneak code. When `false` the exact old code
is used for the specific old sneak behaviour (default: `true`)
* `new_jump`: use the new jump/bouncy jump code. When `false` the exact
old code is used for jumping and bouncy jumping behavior.
(default: `true`)
Copy link
Member

Choose a reason for hiding this comment

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

shouldn't this be false by default?

Copy link
Contributor

Choose a reason for hiding this comment

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

shouldn't this rather be a float for jump "floatiness"?

Copy link
Member

Choose a reason for hiding this comment

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

That sounds like too much configurability that most people won't need.

Copy link

Choose a reason for hiding this comment

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

I'm actually kinda weirded out why such a thing would be optional, that's one of the first annoying things I found about Minetest and it's the way jumping works.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated to make it false by default

src/client/content_cao.cpp Outdated Show resolved Hide resolved
@oilboi
Copy link
Contributor Author

oilboi commented Jun 2, 2020

This needs to be squashed when commited

src/script/lua_api/l_localplayer.cpp Outdated Show resolved Hide resolved
@@ -444,8 +444,9 @@ void LocalPlayer::move(f32 dtime, Environment *env, f32 pos_max_d,
m_can_jump = ((touching_ground && !is_climbing) || sneak_can_jump) && !m_disable_jump;

// Jump key pressed while jumping off from a bouncy block
if (m_can_jump && control.jump && itemgroup_get(f.groups, "bouncy") &&
m_speed.Y >= -0.5f * BS) {
if ((!physics_override_new_jump && m_can_jump && control.jump && itemgroup_get(f.groups, "bouncy") && m_speed.Y >= -0.5f * BS)||
Copy link
Member

Choose a reason for hiding this comment

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

The options common to both new & old jump should be moved into a variable for readability.

if (m_can_jump && control.jump && itemgroup_get(f.groups, "bouncy") &&
m_speed.Y >= -0.5f * BS) {
if ((!physics_override_new_jump && m_can_jump && control.jump && itemgroup_get(f.groups, "bouncy") && m_speed.Y >= -0.5f * BS)||
(physics_override_new_jump && control.jump && itemgroup_get(f.groups, "bouncy") &&
Copy link
Member

Choose a reason for hiding this comment

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

I noticed m_can_jump is missing here, which probably breaks some feature. Is this intentional?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh it's very intentional, it's because m_can_jump is broken with bouncy nodes, and this creates a sane way to calculate if the player can jump based on their Y speed

src/client/localplayer.cpp Outdated Show resolved Hide resolved
@sfan5 sfan5 added Feature ✨ PRs that add or enhance a feature and removed Bugfix 🐛 PRs that fix a bug labels Jun 2, 2020
@sfan5 sfan5 changed the title Jumping fix New jumping behavior controlled by physics override Jun 2, 2020
@paramat
Copy link
Contributor

paramat commented Jun 2, 2020

👎 I consider a slightly different jumping behaviour not nearly important enough to add a whole new physics override (sent over the network), with the associated complexity, maintenance and potential bugs.
You can see that we only have a very few physics overrides and for very fundamental things.
We should not start a precedent of casually adding large numbers of physics overrides for trivial and unnecessary changes of behaviour.

2.) To fix the bug that allows players to jump up nodes irregularly and essentially negate having to have stairs by making "staircases" of nodes.

There is no bug and this statement is not true.

Players can only ascend a slope of nodes if all player jumps are of 1 node, only if all nearby nodes rise in 1-node steps, in which case the slope is already essentially 'stairs'. So 'having to have stairs' is not 'negated'. The player cannot ascend if any jump is 2 nodes.

The current agility on slopes is intentional and has been part of MT for 8 years.
Celeron55 has expressed in the past that the ease and speed of jumping up slopes should not be reduced.
I cannot see any sense in suddenly reducing intentional agility and speed on slopes after 8 years, and when so many players are used to that and like it, and when it is harmless, optional or not.

However, i do not criticise you for making this contribution, you found something you consider an improvement and have good intentions. Sorry i have to object strongly.

@oilboi
Copy link
Contributor Author

oilboi commented Jun 3, 2020

Well that's why it's an option and off by default, you will not even notice it is in there :)

@ghost
Copy link

ghost commented Jun 3, 2020

I'm interested in this being an option, especially considering I think this player just discovered wall jumping

https://www.youtube.com/watch?v=kwp-HT8704o&feature=youtu.be&t=2777

@@ -6092,6 +6092,9 @@ object you are working with still exists.
(default: `false`)
* `new_move`: use new move/sneak code. When `false` the exact old code
is used for the specific old sneak behaviour (default: `true`)
* `new_jump`: use the new jump/bouncy jump code. When `false` the exact

Choose a reason for hiding this comment

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

jump/bouncy jump? I don't think this was intended?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It was, you can now continuously jump on bouncy nodes

@paramat
Copy link
Contributor

paramat commented Jun 7, 2020

Screenshot_2020-06-07 New jumping behavior controlled by physics override by oilboi · Pull Request #9980 · minetest minetest

This is concerning because the conditions for being able to jump have been changed from requiring 'm_can_jump' to also requiring 'touching_ground'.
It is likely that 'm_can_jump' is what should decide this, which is why it was named that way.
So i suspect 'new jump' will break movement in some ways, this needs a lot of testing.

Looking back through code history, the conditions for being able to jump have been this way for 8 years, the rest of MT movement code will be built around that, so this is a very risky change.

@paramat
Copy link
Contributor

paramat commented Jun 7, 2020

To create a new physics override that allows players and modders to select if they want the new consistent jumping code or the old floaty one.

Maybe no-one has explained this yet but, the actual reason this has to be a physics override, instead of a clientside option, is this:

This has to be a physics override so that the server can, if it needs to, have complete control over the 'new jump' option of every player.
If this was a clientside option, different players would have differing agility on slopes, which would cause unfair advantages for some players on any kind of competitive server. The end result would be everyone being pressurised into using 'old jump' out of fear of being at a disadvantage.

@paramat
Copy link
Contributor

paramat commented Jul 14, 2020

I just tested this applied to master branch.

When 'new jump' is enabled, it results in the undesirable and now fixed behaviour of #10136, described in detail in #10136 (comment)

I tested on a pyramid type diagonal slope, which also contains a full-node staircase.
Climbing is slow, approximately half previous speed, very unpleasant and awkward, with the player 'sticking' to the surface of the nodes, moving vertically up then horizontally forward in a zig-zag movement. There is no 'jumping' movement at all, just sliding along node surfaces. This is far worse than the current fast and agile movement.

I accept that you may have 'fixed a code error' and made the code more 'consistent', but that alone does not justify merging this, the resulting behaviour has to be considered.

So i cannot see any justification in adding a new physics override to enable a change in movement behaviour that results in slow, unpleasant and awkward climbing. The resulting movement is exactly the same as issue #10136 that was recently fixed.
So i think it is obvious this PR is unacceptable and should be closed, i ask other core devs to consider disapproving this.

But, there seems to be a fix for bouncy nodes? That may be welcome, please could you split that into a new PR?

@sfan5
Copy link
Member

sfan5 commented Jul 14, 2020

So i cannot see any justification in adding a new physics override to enable a change in movement behaviour that results in slow, unpleasant and awkward climbing

The point of making this a physics override is exactly that people who don't like it don't need to use it.
Just because you consider it undesirable is no grounds for rejection.

@LoneWolfHT
Copy link
Contributor

At this point it might be worth it to port the old_sneak and new_jump options into a server-side setting, I can see we're going to have a lot more situations like this one.
Maybe even figure out some way to allow servers to provide their own custom jump code

@oilboi oilboi closed this Jul 14, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

8 participants