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

Global: Clean up. #47

Merged
merged 4 commits into from
May 15, 2021
Merged

Global: Clean up. #47

merged 4 commits into from
May 15, 2021

Conversation

discordier
Copy link
Contributor

Some further nitpicks that came up while writing the typescript typings.
Several examples were missing return statements and some wrote to non existing or unused properties.

@coveralls
Copy link

Pull Request Test Coverage Report for Build 756

  • 0 of 0 changed or added relevant lines in 0 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage remained the same at 98.755%

Totals Coverage Status
Change from base Build 755: 0.0%
Covered Lines: 4852
Relevant Lines: 4868

💛 - Coveralls

@@ -165,7 +165,7 @@
player.position.set( - 13, - 0.75, - 9 );

controls = new FirstPersonControls( player );
controls.lookSpeed = 2;
controls.lookingSpeed = 2;
Copy link
Owner

Choose a reason for hiding this comment

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

Do you mind deleting the lines which were controls.lookSpeed = 2;? I'd like to keep the current behavior and with setting lookingSpeed to 2 the controls are too sensitive.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You misunderstand, the value was from you.
Either we remove this line (as lookSpeed is no where used), or we have to rename the variable in the controls (as it is named lookingSpeed there).

The problem is, you are writing to a property that is never read.
See the code in controls.js:


this.movementX -= event.movementX * 0.001 * this.lookingSpeed;
this.movementY -= event.movementY * 0.001 * this.lookingSpeed;

Copy link
Owner

@Mugen87 Mugen87 May 14, 2021

Choose a reason for hiding this comment

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

Sorry for not being clear 😅 . I've meant this:

Either we remove this line (as lookSpeed is no where used),

Copy link
Owner

Choose a reason for hiding this comment

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

Never mind! I clean things up 👍 .

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah, ok, sorry. That got lost in translation. 😸
All good then.

@Mugen87 Mugen87 changed the title Hotfix/cleanup Global: Clean up. May 15, 2021
@Mugen87 Mugen87 merged commit 0a59875 into Mugen87:master May 15, 2021
@discordier discordier deleted the hotfix/cleanup branch June 2, 2021 10:34
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.

None yet

3 participants