Add disable_jump to liquids and ladders #7688

SmallJoker · 2018-09-01T16:34:13Z

Implements feature requested in #4494

Jumping is now no longer possible when the group disable_jump = 1 is set. Jumping at the bottom of liquids may look a bit weird, but is actually because the node below can be used to jump.

Testing code

-- river water for comparison
core.override_item("default:water_source", {
	groups = {
		disable_jump = 1,
		water = 3, liquid = 3, puts_out_fire = 1, cools_lava = 1
	}
})

core.override_item("default:water_flowing", {
	groups = {
		disable_jump = 1,
		water = 3, liquid = 3, puts_out_fire = 1,
		not_in_creative_inventory = 1, cools_lava = 1
	}
})

core.override_item("default:leaves", {
	groups = {
		disable_jump = 1,
		cracky = 3
	}
})
-- steel ladder for comparison
core.override_item("default:ladder_wood", {
	groups = {
		disable_jump = 1,
		cracky = 3
	}
})

Wuzzy2 · 2018-10-14T11:47:58Z

I have tested this. Nice!

Works fine for ladders. A ladder that you can only climb down, but not up? Funny. :-) This will be interesting for puzzle games, like @sofar's Inside The Box. :D

Liquids behave a bit weird. Yes, you cannot rise in liquids, this part works fine. But if you are at the bottom of the water (in your test mod), you will rise a tiny bit. Not enough to climb a node, but it sure looks weird, I think it should be fixed. Otherwise fine.

Please update lua_api.txt about the special meaning of disable_jump=1 for ladders and liquids.

Maybe instead of disable_jump=1, this should use disable_ascent=1 for a more fitting group name. You don't really “jump” in liquids and ladders.

One idea (optional):
I feel there should be a group that works like this, but for the downwards direction as well. I.e. disable_descent=1. For ladders, this woud prevent climbing down, for liquids, this would prevent manual (!) sinking. The reason for this is consistency. If you can prevent going up, there should also be a way to prevent going down.

You don't have to do this for me to approve, I just think that with a disable_descent, this would feel more “complete”. :)

SmallJoker · 2018-10-14T13:51:12Z

But if you are at the bottom of the water (in your test mod), you will rise a tiny bit. Not enough to climb a node, but it sure looks weird, I think it should be fixed.

Post above:

Jumping at the bottom of liquids may look a bit weird, but is actually because the node below can be used to jump.

The player uses the node at its feet to repel from the ground, the liquid viscosity then causes the player to slow down quickly. Intended behaviour.

I agree that disable_ascent and disable_descent would be more fitting group names. I will add the required compatibility code to the item registration and override part soon.

Wuzzy2 · 2018-10-14T16:00:04Z

Thanks, awesome!

The minijump on sea floor is not a big deal and I can accept it as-is. It's not really wrong anyway.

paramat · 2018-10-14T19:02:59Z

What's the usecase of no ascent in climbable nodes? Why is that included here? Just wondering.

Wuzzy2 · 2018-10-14T19:15:53Z

One-way climbable blocks for puzzle games. But I didn't have any concrete use case in mind. It's really more about symmetry than anything else.

FaceDeer · 2018-10-22T20:22:57Z

A use case for this that I have in mind: I'd like to implement a heavier-than-air asphyxiant gas to add danger to certain caves. A liquid with no viscosity and no swimming seems like it'd be ideal for something like this, allowing the gas to "swirl" and "flow" with a faintly visible surface that looks nicer than just a bunch of translucent hard-edged cubes.

src/localplayer.cpp

paramat · 2018-10-23T06:07:28Z

Looking at the code, also disabling climbing just uses an already calculated bool, so does no harm.

I'm uncomfortable with the changes to 'old_move' in this PR, they seem to possibly change behaviour.
Since this is a new feature and not high priority it doesn't necessarily have to be in old_move().

src/localplayer.cpp

paramat · 2018-10-23T06:23:29Z

There are a few things here that seem wrong and seem changes to exisitng behaviour, i possibly misunderstand though :)
However we certainly shouldn't risk changing old_move() behaviour so i will disapprove changes to that if i'm in any doubt.
So for now -1 but only because there seem to be errors and due to the danger of changing old_move().

Wuzzy2 · 2018-10-23T17:02:02Z

I agree, legacy movement code should not be touched for this. Can you just implement this feature without touching old_move at all? As far I understand, old_move is legacy and does not need touching anyway.

SmallJoker · 2018-10-23T19:09:05Z

@Wuzzy2 Well sure, that's doable. But then I would also note in the Lua API that this group's full functionality is only available to new_move users.

src/localplayer.cpp

paramat · 2018-10-25T10:45:08Z

I'm ok with the standing node stuff now, misunderstood.
2nd commit seems good.

My only remaining concern is the changes to old_move(), ~~i don't think it's worth the risk, adding features and bugfixing have a large chance of changing behaviour. So i'm -1 unless it's left untouched.~~
The feature request had no core dev support for 2 years so is low priority, i'm a little bemused by the scale of changes being made for such a specialist feature :)

SmallJoker · 2018-10-26T17:03:53Z

i'm a little bemused by the scale of changes being made for such a specialist feature :)

After all it was you who requested these changes indirectly. I'm just coding. If you want fewer changed lines, then please adopt this PR to show me that it's possible. I could also leave the old move code untouched if you tell me so. The feature will then be limited to the new move code only.

paramat · 2018-10-27T11:38:17Z

the scale of changes being made

I think there is a misunderstanding, by that i'm referring to the initial PR, i'm not complaining about coding you have done since to attend to my concerns, which i appreciate of course.

After all it was you who requested these changes indirectly

If you are referring to the initial PR, i have never requested this feature be coded, i was -1 at first and then neutral, see #4494 No other core dev supported the feature over 2 years, but i assume you support the feature now?
Maybe there has been a misunderstanding, did you think i supported the feature?

~~For old_move(), i feel the feature is not important enough to be worth the risk of changing the behaviour.~~

paramat · 2018-10-27T12:41:00Z

Maybe it's because i commented: "Any core dev support?", when i do that i'm not expressing my support for a request.

SmallJoker · 2018-10-27T13:53:33Z

@paramat Ah, I now see the reason for this situation!
PRs are usually a positive indicator since they bring the project along, so I also expected neutral and hoped for some positive support (at least from those who showed interest in the original issue). I think you can safely assume that the creator of the PR actually wants the feature to be in the game (otherwise, why the effort?).

Of course, not everybody might like movement code changes, thus I tried to keep those changes as little as possible. According to your code comments I then corrected the issues, which apparently still touch the old movement code too much. For now I've only seen disapprovals on these lines, so I'd like to know whether @Wuzzy2 and @paramat are happier to leave the old movement code untouched.

I was wrong in assuming you would find interest in this feature; but am glad that you're neutral about it right now. I'm open for suggestions to bring this PR along, and would be happy about feedbacks from other parties as well.

ghost · 2018-10-27T16:16:58Z

Important feature +1

…

On Sat, Oct 27, 2018, 9:53 AM SmallJoker ***@***.***> wrote: @paramat <https://github.com/paramat> Ah, I now see the reason for this situation! PRs are usually a positive indicator since they bring the project along, so I also expected neutral and hoped for some positive support (at least from those who showed interest in the original issue). I think you can safely assume that the creator of the PR actually wants the feature to be in the game (otherwise, why the effort?). Of course, not everybody might like movement code changes, thus I tried to keep those changes as little as possible. According to your code comments I then corrected the issues, which apparently still touch the old movement code too much. For now I've only seen disapprovals on these lines, so I'd like to know whether @Wuzzy2 <https://github.com/Wuzzy2> and @paramat <https://github.com/paramat> are happier to leave the old movement code untouched. I was wrong in assuming you would find interest in this feature; but am glad that you're neutral about it right now. I'm open for suggestions to bring this PR along, and would be happy about feedbacks from other parties as well. — You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub <#7688 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AWXKYgqo1uurE7Rd22eFpgoSYVR-IStaks5upGVigaJpZM4WWSNs> .

Wuzzy2 · 2018-10-28T14:34:40Z

I agree that old_move should not be touched if there's not a strong reason to touch it. This PR is not a strong reason to touch it. Therefore old_move should not be touched in this PR. Personally, I don't really care about old_move because I don't need it in my code, but I agree with the rationale given by paramat.

To remove any doubt: I am in favor of the core feature itself as it will directly help MineClone 2. It can improve cobwebs which are implemented as fake liquids (to slow down movement), but the annoying thing is that you can climb them up, which should not be possible. With this feature I can fix this. But I believe it will also be useful for games that like to add quicksand and other fun stuff.

Finally, @paramat: The “no core dev support” argument is a bit silly if there is actually working code, whether coded by a core dev or not. :P

paramat · 2018-10-28T15:40:52Z

Yes i have now changed labels on the issue since a core dev supports.

~~To clarify i am -1 if old_move() code is touched, otherwise neutral.~~

Ezhh · 2018-10-29T17:10:11Z

So this feature won't work if someone is using old move?

SmallJoker · 2018-10-29T18:40:26Z

@Ezhh from how it looks right now, yes. From what I've seen here, the old_move code should be left in peace, so the feature will be for the new movement code only (as soon as I push the a new commit).

Ezhh · 2018-10-29T19:39:00Z

Well that's a pity. I wasn't aware using old move code would mean missing out on new (and desired) features. Also on servers that use both and permit switching of move code to their players, this will lead to inconsistent behaviour.

Instead of the automatic "oh no we can't change old move at all ever"... in what ways would it be changed? Is new move code not also being changed by this?

Can we please not add more movement related issues through inconsistent behaviour?

paramat · 2018-10-29T21:09:20Z

If i was certain the change to old_move made no difference i wouldn't have a problem, but i'm not certain, as the standing node point has been altered, and am being cautious, understandably.
The feature here is low priority so my caution currently outweighs the benefit of the feature. I'm also neutral about the feature itself.

However i do see the inconsistency problem with not having the feature in old_move, but that would be less damaging than a change to old_move. It's a dilemma because it seems a lose/lose situation.
So this needs very careful testing, which i'm not interested in doing because i don't support the feature (neutral). If others carefully test and find no change to old_move i'll probably remove my -1.

So, more testing needed.

paramat · 2018-10-29T21:29:48Z

The fact is that if features and bugfixes are added to old_move it is highly likely it's behaviour will change. After all, it was a bugfix that caused the whole controversy in the first place.
So, it is difficult to provide new features and bugfix old_move as well as guaranteeing absolutely no change to it's behaviour.

Anyway, i would like to avoid a feature inconsistency between old and new move.
Honestly and personally i feel it's best to drop this feature completely and avoid all this grief, it just doesn't seem worth it (however i'm not going to -1 for this reason).

Ezhh · 2018-10-29T21:37:31Z

After all, it was a bugfix that caused the whole controversy in the first place.

Untrue. It was the things accompanying the bugfix.

You should not exclude old move from new features without being able to say in what way it would change old move. What about if it changes new move? There's some people who are probably pretty attached to exactly how that works as well by now. The answer in both cases is to test, and without a PR being tested, it shouldn't be merged anyway.

paramat · 2018-10-30T01:08:24Z

I mean, the bugfix was only possible by changing some things that then changed sneak behaviour. So it was the bugfix that changed sneak behaviour.

For your second point i've just realised you are correct. I've removed my -1 because it is quite possible that old-move behaviour hasn't been changed in a problematic way. After all i don't fully understand the changes here. However it does need very careful testing before being merged.
So i'm neutral.

paramat · 2018-10-30T02:00:40Z

To be clear, i'm no longer requesting old_move be left untouched.

Wuzzy2 · 2018-11-01T14:53:00Z

I have done the testing on my part for the “new move” code, as written above.
But not for the “old move” code because I do not understand how it works, or more importantly, how it is supposed to work.

Wuzzy2 · 2019-03-09T07:33:34Z

Any news?

SmallJoker · 2019-03-09T08:44:41Z

@Wuzzy2 Now that 5.0.0 is out, reviews might come soon here. The PR is ready.

rubenwardy

All good apart from this

rubenwardy · 2019-04-11T19:23:16Z

src/client/localplayer.cpp

-		// Node below the feet, update each ClientEnvironment::step()
-		m_standing_node = floatToInt(m_position, BS) - v3s16(0, 1, 0);
+		// Node at feet position, update each ClientEnvironment::step()
+		m_standing_node = floatToInt(m_position, BS);


why did this not cause problems before?

Because

It's overwritten after the first of many move() calls inside step() as soon there's a collision

It doesn't matter in fly mode

However, now it has to be this way to properly detect nodes when the player is floating.

@rubenwardy ok for you?

paramat · 2019-06-06T18:44:52Z

I trust SmallJoker's testing so i have no objections and no testing requests.

SmallJoker added @ Script API Feature ✨ PRs that add or enhance a feature @ Client / Controls / Input labels Sep 1, 2018

SmallJoker mentioned this pull request Sep 23, 2018

Stepheight issues with touching ground in liquid #7742

Open

paramat reviewed Oct 23, 2018

View reviewed changes