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

Character controller #1856

Merged
merged 102 commits into from
Dec 12, 2021
Merged

Character controller #1856

merged 102 commits into from
Dec 12, 2021

Conversation

utf94
Copy link
Contributor

@utf94 utf94 commented Nov 24, 2021

No description provided.

app-manager.js Outdated
@@ -543,9 +543,16 @@ class AppManager extends EventTarget {
physicsObject.position.copy(app.position);
physicsObject.quaternion.copy(app.quaternion);
physicsObject.scale.copy(app.scale);

if(app.appType === "vrm") {
Copy link
Contributor

Choose a reason for hiding this comment

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

App manager should not be aware of app type. This code belongs in the loader.

class CharacterPhysics {
constructor(player) {
this.player = player;
this.rb = null;
Copy link
Contributor

Choose a reason for hiding this comment

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

What is rb?


const collision = this.collideCapsule(localVector, localQuaternion2.set(0, 0, 0, 1));
const capsuleOffset = new THREE.Vector3(0, this.player.avatar.height/2, 0);
Copy link
Contributor

Choose a reason for hiding this comment

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

We should not allocate here, since I think it is part of the render loop.

const collision = this.collideCapsule(localVector, localQuaternion2.set(0, 0, 0, 1));
const capsuleOffset = new THREE.Vector3(0, this.player.avatar.height/2, 0);
localVector.add(capsuleOffset);
const collision = this.collideCapsule(localVector, localQuaternion2.set(0, 0, 0, 1)); // TODO: Replace with physicsManager.isGrounded restitution check
Copy link
Contributor

Choose a reason for hiding this comment

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

We should not collide physics here. This should be removed.

Copy link
Contributor

@avaer avaer Nov 25, 2021

Choose a reason for hiding this comment

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

I also don't think isGrounded is something you can ask after the simulation.

Grounded state must come from the simulation itself, not after. After the simulation returns, we don't know if we are truly grounded since the state has already been restituted and that knowledge has been lost.

@@ -156,6 +176,8 @@ class CharacterPhysics {
localVector.y += 1;
localQuaternion.premultiply(localQuaternion2.setFromAxisAngle(localVector3.set(0, 1, 0), Math.PI));
}
const feetOffset = new THREE.Vector3(0, 0.05, 0); // Or feet will be in ground, only cosmetical, works for all avatars
Copy link
Contributor

Choose a reason for hiding this comment

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

More allocation.

}
updateTransform() {
if(this.rb) {
const physicsObjects2 = [];
Copy link
Contributor

Choose a reason for hiding this comment

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

Allocation in the render loop?

updateVelocity() {
if(this.player.avatar) {
if(this.rb) {
physicsManager.setVelocity(this.rb, this.velocity.clone());
Copy link
Contributor

@avaer avaer Nov 25, 2021

Choose a reason for hiding this comment

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

Allocation from .clone().

Copy link
Contributor

@avaer avaer left a comment

Choose a reason for hiding this comment

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

More code issues to fix.

I suspect they will affect functionality (for the better) once fixed.

@alisaad673
Copy link
Contributor

The CCD works fine within its domain. The issues that appear in it are purely fps based depending on each scene. I have attached few examples how fps affect the performance of avatar.

1-On 60 or more fps, everything works perfectly fine without any kind of issues/jitter. Just as fps drop, The issues such as jitter while walking/running appear.

-On 60 fps(no jitter issues.)

WhatsApp.Video.2021-11-29.at.8.35.42.PM.mp4

-FPS drop in the same scene causing jitter

WhatsApp.Video.2021-11-29.at.8.35.42.PM.1.mp4

-No issue in scenes like prototype, grid where fps remain stable at above 60.

WhatsApp.Video.2021-11-29.at.8.35.42.PM.2.mp4

-Jitter drop in high requirement scenes causes issues.

WhatsApp.Video.2021-11-29.at.8.44.11.PM.1.mp4

IMO the issues related to this PR are resolved, The FPS related issues should be fixed separately. The CCD can be pushed to production.

@ahadshams
Copy link
Contributor

ahadshams commented Nov 29, 2021

FPS has improved for me..
Wall jitter doesnt occur at high fps..
Can be pushed for final code review imo

@alisaad673
Copy link
Contributor

@avaer The CCD related issue are fixed. What is left is FPS related issues. Kindly review it for final verdict.

@Loryhoof
Copy link
Contributor

on collision function is still in use for the grounded check, haven't really gotten it to work without it.
It still works, but if it's necessary to change it I'll need to get help from the team.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants