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

Add utility to get/set ex.Vector size #1277

Closed
eonarheim opened this issue Oct 8, 2019 · 15 comments · Fixed by #1298
Closed

Add utility to get/set ex.Vector size #1277

eonarheim opened this issue Oct 8, 2019 · 15 comments · Fixed by #1298
Labels
api change Breaking api change community in-progress This issue is marked in-progress. This label will be removed if there is no activity for 30 days. good first issue good for people new to open source and Excalibur

Comments

@eonarheim
Copy link
Member

Context

It would be useful to set the magnitude of a vector, this allows easy scaling or capping at a maximum value. This is especially useful in physics simulations.

Proposal

I propose something like the following (there is probably a more efficient method of changing the size of the vector)

  /**
   * The magnitude (size) of the Vector
   * @obsolete magnitude will be removed in favor `.size` in version v0.24.0
   */
  public magnitude(): number {
    return this.distance();
  }
  
 /**
  * The size (aka magnitude) of the Vector
  */
  public get size(): number {
    return this.distance();
  }

  public set size(newLength: number) {
    this.normalize().scale(newLength);
  }

Usage:

const vel = new ex.Vector(3, 4);

console.log(vel.size); // 5
vel.size = 13;

// Automatically calculates to correct scale
console.log(vel.x); // 5
console.log(vel.y); // 12
@eonarheim eonarheim added proposal Applied to issues that are a proposal for an implementation api change Breaking api change good first issue good for people new to open source and Excalibur Hacktoberfest and removed proposal Applied to issues that are a proposal for an implementation labels Oct 8, 2019
@eonarheim eonarheim changed the title Proposal: Add utility to get/set ex.Vector size Add utility to get/set ex.Vector size Oct 8, 2019
@saahilk
Copy link
Contributor

saahilk commented Oct 9, 2019

Hi, I'd like to work on this issue.

@eonarheim eonarheim added the community in-progress This issue is marked in-progress. This label will be removed if there is no activity for 30 days. label Oct 9, 2019
@eonarheim
Copy link
Member Author

@saahilk All yours! I've added the community in-progress label for you

@saahilk
Copy link
Contributor

saahilk commented Oct 9, 2019

@eonarheim Alright let me get started then!

@saahilk
Copy link
Contributor

saahilk commented Oct 9, 2019

@eonarheim Is there a doc where I can find how to setup the environment? I only found this. Also could you point me to the files I should look through to get started on this issue specifically?

@eonarheim
Copy link
Member Author

@saahilk Let me know if this doesn't cover everything https://github.com/excaliburjs/Excalibur#environment-setup, please let me know if there are any gaps in the readme 👍

@eonarheim
Copy link
Member Author

We'll want to fix the readme if any steps are ambiguous, missing, wrong, or poorly written

@saahilk
Copy link
Contributor

saahilk commented Oct 10, 2019

@eonarheim Alright. Thanks!

@saahilk
Copy link
Contributor

saahilk commented Oct 16, 2019

@eonarheim Hi which files do I look at to start with?

@eonarheim
Copy link
Member Author

Hi @saahilk Check out Algebra.ts

@saahilk
Copy link
Contributor

saahilk commented Oct 20, 2019

@eonarheim Do I keep the existing magnitude function marking it as obsolete or just remove it adding the size() function?

@eonarheim
Copy link
Member Author

Good question, lets keep the existing magnitude function with an @obsolete in the JSDoc comment. We'll remove it in the future v0.25.0 release.

saahilk pushed a commit to saahilk/Excalibur that referenced this issue Oct 21, 2019
…or size.\n Marked existing magnitude function as obsolete.\n Updated changelog.\nResolves: excaliburjs#1277
@saahilk
Copy link
Contributor

saahilk commented Oct 21, 2019

Hi @eonarheim I've submitted an initial pull request with some changes. Let me know if there's anything else that needs to be changed.

@saahilk
Copy link
Contributor

saahilk commented Oct 22, 2019

Hi @eonarheim the comments in the usage you have mentioned in the above issue is incorrect. When you set vel.size=13 the unit vector is (0.6, 0.8) and the scaled vector will be (7.8, 10.4) and not (5, 12).

@eonarheim
Copy link
Member Author

@saahilk Good find! I was trying to do math in my head when writing the issue and failed oops!

@saahilk
Copy link
Contributor

saahilk commented Oct 23, 2019

@eonarheim Could you check the modified pull request here.

eonarheim pushed a commit that referenced this issue Oct 23, 2019
Closes #1277 

## Changes:

- Add get/set vector size utility
- Marked existing magnitude function as obsolete
- Updated CHANGELOG.md
@jedeen jedeen added this to the 0.24.0 Release milestone Apr 26, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
api change Breaking api change community in-progress This issue is marked in-progress. This label will be removed if there is no activity for 30 days. good first issue good for people new to open source and Excalibur
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants