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 sliding player clipping through floor #2838

Merged
merged 2 commits into from
Apr 1, 2024

Conversation

weluvgoatz
Copy link
Member

For whatever reason that is beyond me, the PR #2795 appears to have intentionally introduced a bug where Tux would clip through the floor if he stopped sliding but was still moving (meaning, the player stopped pressing the down key). In areas where Tux ended sliding in a 1-block hole, he would clip into the floor. This clipping was also noticeable under normally-tall terrain gaps, but it resolves itself immediately. However, this PR fixes it. Let me know if there are any issues or side effects with it.

@weluvgoatz weluvgoatz added the type:bugfix Pull Requests that fix bugs. label Mar 22, 2024
src/object/player.cpp Outdated Show resolved Hide resolved
@Brockengespenst
Copy link
Contributor

It was definitely not my intention to introduce bugs, if that happened, I am really sorry.
The code was introduced for the following scenario:

  • Tux is big
  • Player presses and holds down key for the rest of the described sequence -> Tux is crawling
  • Player crawls towards a slope -> Tux starts to slide on the slope
  • Slope ends and Tux comes to a stop -> Tux remains in crawling animation

The original behavior - that your PR now restores - is that Tux sets the "duck" action. If you then press left or right, the action will switch from duck to crawling animation which looks a bit weird. In my opinion remaining in the crawl action looks much better than switching to duck action.
So m_was_crawling_before_slide should in my opinion not be set to false in case player stopped sliding due to friction.

@Brockengespenst
Copy link
Contributor

At least that was my intention that Tux does not go to duck when it stops after sliding, but it seems that it is not working like that (I was pretty sure I tested this several times), so at this part your PR does not seem to change the behavior.
Anyway: The flag is used to skip the "duck" part of the sliding animation when Tux was crawling before (otherwise Tux would crawl, standup and then transit to slide again in the above described scenario [transition from crawl to slide]).

@mrkubax10 mrkubax10 added category:code status:needs-review Work needs to be reviewed by other people involves:physics labels Mar 24, 2024
@Brockengespenst
Copy link
Contributor

I remember now, why I was so sure that the described behavior is working: Please check out also my changes in #2810 . With these changes, the described behavior should work. Looks like you're in a flow right now, so maybe you could take all these information into account and do the ultimate fix. :-)

@weluvgoatz
Copy link
Member Author

I remember now, why I was so sure that the described behavior is working: Please check out also my changes in #2810 . With these changes, the described behavior should work. Looks like you're in a flow right now, so maybe you could take all these information into account and do the ultimate fix. :-)

Can you be a little more verbose regarding this message, please? I'm afraid I don't understand what you mean. And also sorry for coming across as mean in the original PR message.

@Brockengespenst
Copy link
Contributor

Brockengespenst commented Mar 27, 2024

Sure, sorry for the confusion.

I thought that Tux would already remain in the crawl action when he was sliding before and comes to a stop while the down key is still pressed (but without left or right being pressed). I forgot that I added/fixed this in #2810 (among the other things that are mentionened in the PR description), so I thought your PR here would break this behavior. Maybe you want to have a look at #2810 and check if it either makes things better or maybe you have a better idea how to improve it otherwise.

Also I mixed this discussion here a bit up with the behaviour regrading m_was_crawling_before_slide, which is supposed to skip the first frames of the slide action when Tux is crawling and directly goes into the slide action. The first frames of the slide actions show Tux changing from standing to sliding, but this leads to animation glitches if Tux is already crawling.

I hope this makes it a bit clearer. Please let me know if it is still too confusing. :-)

@weluvgoatz
Copy link
Member Author

I just tested both PRs together and they seem to work pretty independently of each other and both of them together seem to fix the bugs they set out to, so I think both are fine right now.

@Brockengespenst
Copy link
Contributor

Perfect, thanks!

@weluvgoatz weluvgoatz merged commit 6298405 into SuperTux:master Apr 1, 2024
33 checks passed
@mrkubax10 mrkubax10 removed the status:needs-review Work needs to be reviewed by other people label Jun 15, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants