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: Player got falling damage when dismount #61

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

Conversation

mhtvsSFrpHdE
Copy link

@mhtvsSFrpHdE mhtvsSFrpHdE commented Oct 3, 2019

close #53

This time I use async method to do this:
first I set player invulnerable to true, this can avoid damage for the player.
then use async method to set invulnerable to false after 3 second,
this should enough for him to fall.

A event listener solution certainly better than "invulnerable hack",
but that have too much job to do.

For me, I feel not good when your magic without comment everywhere.
The thing is, when you try to do same things but use method different than
"Bukkit new developer tutorial & guide", as your personal style...
or even just use some API, I, or the after contributors, are not here to DECOMPILE your code.

If now I want to implement the "on demand event listen" solution instead of "invulnerable hack",
I need to know where to put my "traveling player array".
So I start read code from main method and one line after another,
to figure out where are you placing similar things.
In this process, I don't want to care about how exactly the code to implement human design.
Just like any "HelloWorld" program that doesn't need to care about how the graphic card and HDMI protocol or anything else to work.
So I need the "green things"(comment) says "Oh here I do some stuff because I want to...", "The process to dismount player: one two three four five", "I put these variable here because..."
By read the green things I can fast a lot to know if a new things join in, where to put it,
or if I want to edit something, where to find.

OK yet, I know how bad is "tell the open source author you should do things like this".
Just my two cents about somebody is can't even understand what happened in "onLoad" method.
Sometimes, for better OTHER human readable, I even NOT to simplify

if (player.isInvulnerable() == false)

to

if (!player.isInvulnerable())

@mhtvsSFrpHdE
Copy link
Author

The close #53 is masked because now it is not a universal way.
I need to copy & paste it to every code section that dismount player.

To make it callable, like "safeDismount(Player player)" or something
I still need a deep understand about how this project is designed.

@mhtvsSFrpHdE
Copy link
Author

Some other bug that:
A player may be able to use this patch to get permanent invulnerable status.

It's very rare to happen this but sometimes the async cancel method may not trigger.

  • Server crashed.
  • Server stopped.
  • Server restarted.

@Totollie
Copy link
Contributor

Totollie commented Oct 3, 2019

The code is simple to follow

@Phiwa
Copy link
Owner

Phiwa commented Oct 3, 2019

To be honest, I do not like to create another list with players because this increases the risk of "forgetting" something when mounting/dismounting/..., but if it is implemented in the right place, it could be done and I'd be okay with it. You just need to make sure that it does not lead to problems.
Adding the player to the list should be done in the mount method and the removing in the async task scheduled in removeRiderAndDragon, I guess. You could pobably use the existing EntityDamageEvent listener.

@mhtvsSFrpHdE
Copy link
Author

So we now don't use setInvulnerable instead, we use event cancel to avoid permanent invulnerable.
This is really simple I just need to add a background task at the remove player from list method,
let him be removed later for few seconds instead of immediately.

I later do the coding.

@Phiwa
Copy link
Owner

Phiwa commented Oct 10, 2019

But as I said, this won't work out that easily...

  • You cannot simply use the existing list
  • Creating another list doesn't seem like a nice option either

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants