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

Redo player_api #2745

Merged
merged 26 commits into from Jan 17, 2022
Merged

Redo player_api #2745

merged 26 commits into from Jan 17, 2022

Conversation

appgurueu
Copy link
Contributor

@appgurueu
Copy link
Contributor Author

Possible compatibility breakage:

  1. Mods (like beds) using set_eye_offset to counter eye_height of old player_api
  2. Collisionbox changing making entering physical objects/nodes easier

mods/player_api/api.lua Outdated Show resolved Hide resolved
@paramat
Copy link
Contributor

paramat commented Sep 26, 2020

Please do not undermine and hijack my #2743 work. I am already working on this feature and started before you did, you are now trying to make it redundant before i have had time to finish mine, this is irritating.
For this reason, i am -1 for this PR until mine is merged or closed unmerged.
However i appreciate the input on the implementation.

I have recently been studying the player_api mod closely over the last few weeks, because i am rewriting and improving it for use in new games. So i ask everyone, please wait until i have had time to review this PR before any possible merge.

@appgurueu
Copy link
Contributor Author

appgurueu commented Sep 27, 2020

Please do not undermine and hijack my #2743 work. I am already working on this feature and started before you did, you are now trying to make it redundant before i have had time to finish mine, this is irritating.

With all due respect for your work, the implementation of this feature takes approximately 5 minutes. I am also not satisfied with your implementation, which is why I have redone it. Differences (advantages, IMO):

No [animation] = collisionbox/eye_height lookup; instead per-animation collisionboxes are optional and no such lookup, instead collisionbox & eye_height are stored as subtables; higher code quality (no hacky workaround for keeping compatibility, which your lookup breaks), less code duplication (no need to specify collisionbox & eye_height for each animation).

@SmallJoker
Copy link
Member

I don't mind if the collisionbox change is integrated here as long it'll be stated in the commit(s).
Overall I like this concept due to its code simplification.

@paramat
Copy link
Contributor

paramat commented Oct 1, 2020

Sorry i was irritated. You have the right to offer an alternative implementation of course, it can just be a little irritating when that happens and i have the right to get irritated by that =)
I removed my -1 above as it is not justified.

As a personal project i have been investigating how player_api could be improved for use in new games, so i really appreciate seeing work from others on this.
player_api has had the same basic form for 7 years, and it was reviewed and merged rather quickly and casually, so i have been suspecting it is not particularly good code.

Looking at your PR, it is certainly better code than my minimum-change alteration of player_api in #2743 , and fixes some other issues too, so i am closing my PR.
The Player API is the part of MTG most likely to be used in new games, so i think it is a good idea to get this mod into a good state despite MTG being feature-frozen.

@paramat paramat added the Bugfix label Oct 1, 2020
game_api.txt Show resolved Hide resolved
mods/player_api/api.lua Outdated Show resolved Hide resolved
@appgurueu
Copy link
Contributor Author

So, my solution looks like this:

  1. During model registration:
    1. Delete values equal to the model default values
    2. Use same references for equal collisionboxes
  2. This allows us to simply check (during set_animation):
    1. prev_anim.eye_height == anim.eye_height
    2. prev_anim.collisionbox == anim.collisionbox

Performance could be increased by caching some sort of property combination ID which could then be compared in set_animation (single comparison instead of double). Thoughts? Should the "model registration" part be removed and rather all comparisons be done in set_animation (could be seen as increased code quality, at the expense of performance) ?

@paramat
Copy link
Contributor

paramat commented Oct 10, 2020

In my PR i discovered you cannot do boolean '==' or '~=' between the collision tables, so i ended up using table.concat() to get values that worked:
if table.concat(new_cbox) == table.concat(old_cbox) then
What do you think of that approach? It avoids mod code that loops through table values to compare them, but may actually be lower performance. It also seems a little crude.

game_api.txt Show resolved Hide resolved
game_api.txt Outdated Show resolved Hide resolved
@appgurueu
Copy link
Contributor Author

Ready for merge.

@sfan5
Copy link
Member

sfan5 commented Feb 1, 2021

Bug: For the first time in a game session sleeping in a bed will not set the model.

This doesn't seem to be caused by this PR however...

@appgurueu
Copy link
Contributor Author

Bug: For the first time in a game session sleeping in a bed will not set the model.

This doesn't seem to be caused by this PR however...

It is not. It depends on whether you enter the bed in first-person mode, AFAIK.

@sfan5 sfan5 added this to the 5.5.0 milestone Feb 1, 2021
@appgurueu
Copy link
Contributor Author

Bump. Merge conflicts resolved.

Copy link
Member

@sfan5 sfan5 left a comment

Choose a reason for hiding this comment

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

Tested, works.

@appgurueu
Copy link
Contributor Author

Last commit is untested and effectively implements #2751

Copy link
Member

@sfan5 sfan5 left a comment

Choose a reason for hiding this comment

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

Appears to work fine

@sfan5
Copy link
Member

sfan5 commented Jan 16, 2022

I am still very curious why changing the player model (when entering a bed) does not work in third-person mode. This detail should be mostly invisible to the game.

Edit: Okay that's obvious now. It works for everyone else but the client in question, set_local_animation would have to called too (but only in this case). Not so simple.
But setting eye height is actually broken Fixed

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