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

Player API: Animation-dependent collisionbox and eye_height #2743

Closed
wants to merge 2 commits into from
Closed

Player API: Animation-dependent collisionbox and eye_height #2743

wants to merge 2 commits into from

Conversation

paramat
Copy link
Contributor

@paramat paramat commented Sep 25, 2020

Closes #2742
See #2742 (comment) and following comments.
For the registered MTG player model, collisionbox and eye-height will now be correctly set when 'sitting' or 'laying'.
The main benefit however is for mod-registered player models and their various and possibly more unusual animations, for example a crawling animation could now allow a player to pass through 1-node high spaces.

Player model collisionbox and eye_height can each be defined as a table of per-animation values, or left as a single universal value (which also ensures backwards compatibility).
When animation is set in set_animation(), the animation-dependent object properties are also set.

The set_animation() function returns if the requested animation is already applied to a player, so the function only executes when the animation needs to change, for example starting walking, stopping walking etc.

Now that object properties are also set in set_animation(), an object properties packet is sent to the client each time animation changes. I hope this is not a significant increase in network traffic, as it will be on average 1 extra packet every second (very approximately).

@paramat
Copy link
Contributor Author

paramat commented Sep 25, 2020

Testing ...

screenshot_20200925_221747

^ Dead player collision/selection box

Screenshot from 2020-09-25 22-03-09

^ The new view when dead, close to the ground

screenshot_20200925_222641

^ Sitting player collision/selection box

screenshot_20200925_222747

^ The new view when sitting

@appgurueu
Copy link
Contributor

appgurueu commented Sep 26, 2020

This PR is fine content-wise, but IMO player_api needs a redo: table keys should not be duplicated - instead, subtables should be used. Example:

animations[animname] = {x = ..., y = ...}, collisionbox[animname] = {...}, eye_height[animname] = {...}

would become

animations[animname] = {x = ..., y = ..., collisionbox = {...}, eye_height = ...}

@appgurueu appgurueu mentioned this pull request Sep 26, 2020
@paramat
Copy link
Contributor Author

paramat commented Sep 26, 2020

Thanks for the input.

table keys should not be duplicated

I accept that might be better coding practice, but it is not important or essential.
Your suggestion requires radical changes to player_api. Familiarity, simplicity, minimal change and minimisation of compatibility risk is perhaps more important.

Backwards compatible with previous model definition format.
For the Minetest Game player model, this avoids dead players having
obstructive tall collision and selection boxes.
@paramat paramat added Bugfix and removed WIP labels Sep 26, 2020
@paramat
Copy link
Contributor Author

paramat commented Sep 26, 2020

PR is ready.
Some collisionbox and eye height values still need fine-tuning, but that will be done very soon and before a merge, the PR is otherwise complete, reviewable and testable.

@paramat
Copy link
Contributor Author

paramat commented Sep 27, 2020

Added a second commit that avoids setting properties if there is no change in collisionbox, which is most of the time for the MTG player model, as the collisionboxes for stand, walk, mine, walk_mine are all identical.
A change of eye_height is not checked, it is assumed that eye height does not change if collisionbox does not change, this is the case for the MTG player model and will almost always be the case for all models. This assumption simplifies the code.
Retested.

@paramat
Copy link
Contributor Author

paramat commented Oct 1, 2020

Closing, #2745 is better.

@paramat paramat closed this Oct 1, 2020
@paramat paramat deleted the animcollbox branch April 2, 2021 19:33
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.

Player API: Animation-dependent player model collisionbox
2 participants