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

Add minetest.register_on_punchplayer #2691

Closed
wants to merge 1 commit into from

Conversation

TeTpaAka
Copy link
Contributor

Rebase of #1307
Additionally adds a return value.
Return true to disable the damage handling.

@TeTpaAka TeTpaAka changed the title Player on punch Add minetest.register_on_punchplayer May 11, 2015
Please note that forceloaded areas are saved when the server restarts.
* `minetest.register_on_punchplayer(player, hitter, time_from_last_punch, tool_capabilities, dir)`
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm missing a func( here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed. It was in the original pull request like this and I didn't see the mistake.

@est31
Copy link
Contributor

est31 commented May 12, 2015

When you return true inside the function, then it still seems for the local player that the punched loses HP (turn debug output on with pressing f5 to get HP of other SAOs). Also, the log will show it up as actual damage, like:

ACTION[ServerThread]: Player a punched by player b, damage 1 HP

@TeTpaAka
Copy link
Contributor Author

How can I force the send of an HP packet? The problem is, if I'm correct, that the HP packet is only send for an HP change (reasonable) but the client predicts the damage and applies it. Maybe the client should ask for a new HP packet after it predicts the damage?
BTW, the same happens for disabled damage on current master, so the damage bug is not completely my fault.

@est31
Copy link
Contributor

est31 commented May 12, 2015

Ah I see. Ok, then it isn't required. 👍 if it gets squashed.

@TeTpaAka
Copy link
Contributor Author

Squashed.

@TeTpaAka
Copy link
Contributor Author

Changed the return from RUN_CALLBACKS_MODE_LAST to RUN_CALLBACKS_MODE_OR so any mods can log punch actions without re-enabling the damage. (Same commit)

@TeTpaAka
Copy link
Contributor Author

I fixed the damage prediction and server log anyway. Should I squash again?

Please note that forceloaded areas are saved when the server restarts.
* `minetest.register_on_punchplayer(func(player, hitter, time_from_last_punch, tool_capabilities, dir))`
Copy link
Member

Choose a reason for hiding this comment

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

This is in the wrong place, you should add it where all the other register_ons are.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sorry, fixed.

@rubenwardy
Copy link
Member

As a general rule, don't look at surrounding code for a style guide.

People usually correct code around their patches, but that doesn't matter much. (Not usually a condition of merging)

@est31
Copy link
Contributor

est31 commented May 12, 2015

Style guide here.

}


actionstream<<"Player "<<m_player->getName()<<" punched by "
Copy link
Contributor

Choose a reason for hiding this comment

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

There should be spaces before and after each <<.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed.

@est31
Copy link
Contributor

est31 commented May 12, 2015

Very nice. I've already 👍 ed it.

@TeTpaAka
Copy link
Contributor Author

Maybe I should also pass the inflicted damage? I just realized it was missing.

} else {
actionstream << ", damage handled by lua";
}
actionstream<<std::endl;
Copy link
Contributor

Choose a reason for hiding this comment

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

spaces :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Missed it. Now fixed.

@Zeno-
Copy link
Contributor

Zeno- commented May 13, 2015

Always squash the commits if the "function"/intent of the PR remains the same. E.g. something that fixes a typo or style issue in your PR should always be squashed.

E.g.#2 none of the current extra commits changes what the PR does -- i.e. "add minetest.register_on_punchplayer" -- so they should be squashed, yes.

@TeTpaAka
Copy link
Contributor Author

Also pass the damage value to the on_punchplayer function and squashed everything in one commit.

@TeTpaAka
Copy link
Contributor Author

Fixed wrong comment, stating on_dieplayers and fixing the table name to core.registered_on_punchplayers (the s was missing).

@Ekdohibs
Copy link
Member

👍

@@ -38,6 +40,9 @@ class ScriptApiPlayer
void on_joinplayer(ServerActiveObject *player);
void on_leaveplayer(ServerActiveObject *player);
void on_cheat(ServerActiveObject *player, const std::string &cheat_type);
bool on_punchplayer(ServerActiveObject *player,
Copy link
Contributor

Choose a reason for hiding this comment

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

remove trailing whitespace please

@est31
Copy link
Contributor

est31 commented May 15, 2015

removed them. c5b4e54

@est31 est31 closed this May 15, 2015
@TeTpaAka TeTpaAka deleted the player-on-punch branch May 15, 2015 09:16
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

6 participants