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

Fix glitch that jolts players up edges #13975

Closed
wants to merge 4 commits into from

Conversation

jordan4ibanez
Copy link
Contributor

@jordan4ibanez jordan4ibanez commented Nov 9, 2023

  • Goal of the PR
    Fixes a glitch that jolts players up edges of nodes. "The Boink™"
  • How does the PR work?
    Makes sure that the thing is firmly on the ground before committing to step up.
  • Does it resolve any reported issue?
    No idea

To do

This PR is Ready for Review.

How to test

Try to make "The Boink™" happen with this change.

Here's a youtube video showing it in action for some reason. https://youtu.be/38UbIPhHimk?feature=shared

Copy link
Member

@SmallJoker SmallJoker left a comment

Choose a reason for hiding this comment

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

This may look like it causes some strange side-effect in gameplay but I found that there's no difference in the maximal node climbing height with or without the sneak glitch being active.

@Desour
Copy link
Member

Desour commented Nov 25, 2023

Shouldn't there be a physics override or so for this? I can imagine players complaining about pvp or parkour disadvantages with the new version.

@SmallJoker
Copy link
Member

SmallJoker commented Nov 25, 2023

@Desour Would you please be so nice to provide an example where this change breaks a specific movement? If the node is too high to jump onto, the code will be executed when the player is about to fall down. I can imagine that the hiccup (that is already present in master) would only be delayed by a few milliseconds, thus not break anything. The player (or entity for the matter) would still be put on top of the node.

Unless there's a clear negative difference in gameplay, I think that customizing this behaviour introduces unnecessary code and maintenance bloat.

@Desour
Copy link
Member

Desour commented Nov 25, 2023

Example:

  • Build a staircase (with normal cube nodes).
  • Hold jump and forward, to jump upwards.
  • => You're faster in master. (Haven't measured, but it definitely feels faster.)

@SmallJoker
Copy link
Member

SmallJoker commented Nov 25, 2023

Right. Teleporting up (what step-up does) effectively makes you move faster. That is a logical consequence of getting rid of the hiccup.

@sfan5 sfan5 added this to the 5.9.0 milestone Nov 26, 2023
@jordan4ibanez
Copy link
Contributor Author

I just came back to say hi again

@sfan5 sfan5 added the Action / change needed Code still needs changes (PR) / more information requested (Issues) label Dec 25, 2023
@Zughy
Copy link
Member

Zughy commented Jan 13, 2024

So is OP expected to add a physics override option (due to the label) or is it fine and we can proceed?

As someone who follows speedruns, it happens many times that new games get patched and speedrunners have to find a new way (shorter or longer) for a new record. I think this is no big deal, especially if we consider that it means more code burden for us - and it's not like there aren't many movement bug issues already. It takes no time to a server admin to remove one sec from the scoreboard to everyone (and frankly I don't know servers that feature time trial parkours anyway)

@jordan4ibanez
Copy link
Contributor Author

So is OP expected to add a physics override option (due to the label) or is it fine and we can proceed?

As someone who follows speedruns, it happens many times that new games get patched and speedrunners have to find a new way (shorter or longer) for a new record. I think this is no big deal, especially if we consider that it means more code burden for us - and it's not like there aren't many movement bug issues already. It takes no time to a server admin to remove one sec from the scoreboard to everyone (and frankly I don't know servers that feature time trial parkours anyway)

Please let me just merge this in so we can stop dealing with bugs and not harbor them anymore

@Desour
Copy link
Member

Desour commented Jan 15, 2024

I don't remember having seen any speedrunning in minetest either. But as said, this gives a pvp disadvantage. For some users it will be a reason to not upgrade.
We already have similar physics override flags, I don't see why there shouldn't be a new one for this case.

@Warr1024
Copy link
Contributor

For parkour games, the big impact won't be stair climbing times, but jumps that used to be barely possible due to the "ledge grab" no longer being possible and requiring entire map areas to be reworked.

If an override is available for this, I would enable it and keep the legacy behavior at least in NodeCore (though I would try the new physics first in my other games), since hyper realistic movement is not a design goal and excess realism is likely to distract from the game. If no override is available then I will probably just have to tell players that it's something outside of my control and they need to either adapt to it, or file an issue here.

@sfan5
Copy link
Member

sfan5 commented Jan 15, 2024

There have been enough uproars about the engine changing existing behaviour or "pulling the rug".
Proposing to just stop dealing with bugs and merge a breaking change anyway is not productive.

@sfan5 sfan5 added the Breaking Change Changes an existing API behavior in an incompatible way label Jan 15, 2024
@appgurueu
Copy link
Contributor

I think a physics override is insufficient: The code modified by Jordan applies to entities and players alike; this patch would hence also apply to entities with a stepheight > 0. We'd need an object property.

@Zughy
Copy link
Member

Zughy commented Feb 21, 2024

@jordan4ibanez still want to work on this? @minetest/engine thoughts about what appguru has said?

@jordan4ibanez
Copy link
Contributor Author

@jordan4ibanez still want to work on this? @minetest/engine thoughts about what appguru has said?

You can steal it if you want

@Zughy
Copy link
Member

Zughy commented Feb 21, 2024

No no, just wanted to know if I had to close it

@jordan4ibanez
Copy link
Contributor Author

Oh yeah go for it

@Zughy Zughy added the Adoption needed The pull request needs someone to adopt it. Adoption welcomed! label Feb 25, 2024
@Zughy Zughy closed this Feb 25, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Action / change needed Code still needs changes (PR) / more information requested (Issues) Adoption needed The pull request needs someone to adopt it. Adoption welcomed! Breaking Change Changes an existing API behavior in an incompatible way Bugfix 🐛 PRs that fix a bug @ Client / Controls / Input One approval ✅ ◻️
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

8 participants