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

Vertical swimming. #3251

Merged
merged 5 commits into from
Jul 19, 2022
Merged

Vertical swimming. #3251

merged 5 commits into from
Jul 19, 2022

Conversation

patriboz
Copy link
Contributor

Implemented vertical swimming animations on keys SPACE and C.

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature. The key has expired.
@patriboz patriboz changed the base branch from master to tcm-vis-swim July 14, 2022 16:01
@gonnavis gonnavis self-requested a review July 14, 2022 16:04
Copy link
Contributor

@lalalune lalalune left a comment

Choose a reason for hiding this comment

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

I have added a review. Mostly cosmetic and PR cleanups.

export * from 'http://127.0.0.1:8080/lore-model.js';
Copy link
Contributor

Choose a reason for hiding this comment

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

please remove this from PR

isFirstBone,
} = spec;

Copy link
Contributor

Choose a reason for hiding this comment

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

This looks like VSCode auto formatting.

I know we have a bunch of red squigglies, but can we remove these / paste them back in for the sake of making a really simple PR and then we can do formatting separately?

dst.slerp(localQuaternion2, f);



Copy link
Contributor

Choose a reason for hiding this comment

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

no need for these spaces

scenes/water.scn Outdated
0,
0
],
"start_url": "/practice/dual-contouring-terrain-main/"
Copy link
Contributor

Choose a reason for hiding this comment

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

do we need these changes in pr?

Copy link
Contributor Author

@patriboz patriboz Jul 15, 2022

Choose a reason for hiding this comment

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

No, we don't need these changes. I made them to have more space for swimming while testing. Will remove them.



// Swinging Hips for Breaststroke
if(boneName === 'Hips' && avatar.sprintFactor === 0 && movementsTimeS > 0) {
Copy link
Contributor

Choose a reason for hiding this comment

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

since we are checking if bonename === hips below, could we embed this boolean check inside the other one?

@@ -977,16 +977,28 @@ class UninterpolatedPlayer extends StatePlayer {
emote: new BiActionInterpolant(() => this.hasAction('emote'), 0, crouchMaxTime),
movements: new InfiniteActionInterpolant(() => {
const ioManager = metaversefile.useIoManager();
Copy link
Contributor

Choose a reason for hiding this comment

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

We could probably move the useIoManager call up to the top of the function and then re-use it

Copy link
Contributor Author

@patriboz patriboz Jul 15, 2022

Choose a reason for hiding this comment

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

I tried to move it up to the top of the function, but when doing so, metaversefile somehow is only an empty EventTarget object and useIoManager() isn't defined. So I'll keep it as it is for now.

}, 0),
movementsTransition: new BiActionInterpolant(() => {
const ioManager = metaversefile.useIoManager();
return ioManager.keys.up || ioManager.keys.down || ioManager.keys.left || ioManager.keys.right || ioManager.keys.space || ioManager.keys.ctrl;
}, 0, crouchMaxTime),
Copy link
Contributor

Choose a reason for hiding this comment

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

is crouchmaxtime right for swimming?

Copy link
Contributor

Choose a reason for hiding this comment

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

crouchMaxTime used more like a default value in existing codes.
But it should be better to rename to defaultMaxTime.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I will change crouchMaxTime to defaultMaxTime.

@gonnavis
Copy link
Contributor

@patriboz Vertical swimming works fine.
But if already swim up on surface of water, should turn back to floating or horizontal swimming, instead of keep swim vertically.

And I think should stick on surface until press C to dive.

Reference: https://youtu.be/prreEsPw1Qg

You may need chat with @tcm390 on how to check if on surface.

@patriboz
Copy link
Contributor Author

@gonnavis Yes, you are right. I will change the animation accordingly and talk to @tcm390 about this.

@gonnavis
Copy link
Contributor

  • Vertical state to surface state need smooth transition too.
  • If on surface not moving, need play floating animation instead of swimming ( even holding space key ).
  • I feel the sinusoidal motion too "vertically", need turn back to "horizontally" a little.
9d443f3fff8489e37de7991d0f57a3a8.mp4

image

@gonnavis
Copy link
Contributor

Looks like the "sinusoidal motion too vertically" issue caused by out of sync.
It's ok at start, but getting worse after some time, even will "impulse at most vertical pose".

Can try to sync, but maybe good to delete "sinusoidal motion" feature in this PR.
Let this PR focus on vertical swimming due to space C and camera angle.

@gonnavis
Copy link
Contributor

gonnavis commented Jul 15, 2022

Have not considered "vertical swimming due to camera angle".

@gonnavis gonnavis changed the title Patriboz/swim Vertical swimming. Jul 15, 2022
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

3 participants