-
Notifications
You must be signed in to change notification settings - Fork 35
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-use-pertick-interpolated #119
base: master
Are you sure you want to change the base?
Conversation
silnarm
commented
Oct 14, 2023
- add player position/rotation interpolation to perTick use action
* add player position/rotation interpolation to perTick use action
perTick test 1_14_4 (read-only).zip
|
IServerPlayerEntity iplayer = (IServerPlayerEntity)(Object)player; | ||
EntityPlayerActionPackActionTypeAccessor typeAccessor = ((EntityPlayerActionPackActionTypeAccessor)(Object)type); | ||
EntityPlayerActionPack.Action self = (EntityPlayerActionPack.Action)(Object)this; | ||
IEntityPlayerActionPackActionTypeUse actionTypeUSE = (IEntityPlayerActionPackActionTypeUse)(Object)EntityPlayerActionPack.ActionType.USE; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does it only support "USE" action pack? imo it's much better if all action packs support interpolation
at the same time, and them will have the same behavior
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
added to ATTACK as well.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You can use the global flag suggestion described in 2. of #119 (comment), so you don't need to mixin into every action implemetation that invokes EntityPlayerActionPack#getTarget
...ion/mixins/carpet/tweaks/command/playerActionEnhanced/EntityPlayerActionPackActionMixin.java
Outdated
Show resolved
Hide resolved
{ | ||
private float tickPart = 1.0f; | ||
|
||
@Redirect( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@Redirect
is bad, try use @ModifyExpressionValue
or @WrapOperation
from mixin extra, or set require = 0
if you do need an @Redirect
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, bad I know. It is carpet code at least, not MC. Added the require = 0
, but I was unable to get rid of it, @ModifyExpressionValue gave an unhelpful error message, but I assume the static function is the problem.
src/main/java/carpettisaddition/mixins/command/player/pertick/ServerPlayerEntityMixin.java
Outdated
Show resolved
Hide resolved
Some suggestions:
|
|
||
actionTypeUSE.setTickPart(1.0f); | ||
iplayer.swapOldPosRot(false); | ||
iplayer.pushOldPosRot(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So here's where you record the rotation and position of the player:
- when the action starts
- when the action completes an execution
You're assuming that the player moves smoothly between 2 positions, but this assution might be wrong. The player might be teleported, and teleporation do not have interpolation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is true. I had largely discounted it as an issue originally because player reach meant the use attempts would do nothing anyway, but it will potentially load chunks which does suck.
Will have a think about it, seems like it will be a few more injects at least though, I'm not convinced it is worth handling this explicitly.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
imo, it's important to ensure interpolation only applies in the correct scenarios. As you mentioned, interpolating in unexpected scenarios can cause problems, especially since carpet doesn't provide fine-granular permission system for these commands
and that's why the interpolation feature is hard to implement
* remove static in ActionType.USE, move 'tickPart' to ActionPack. * move last tick pos/rot into Action * dupe USE mixin for ATTACK * change some names