Skip to content

Absolute value operations on vector classes - #2962

Open
AfonsoDGBatista wants to merge 3 commits into
jMonkeyEngine:masterfrom
AfonsoDGBatista:master
Open

AfonsoDGBatista wants to merge 3 commits into
jMonkeyEngine:masterfrom
AfonsoDGBatista:master

Conversation

@AfonsoDGBatista

Copy link
Copy Markdown

Added abs and absLocal methods on Vector2f, Vector3f and Vector4f classes.

Add methods to compute absolute values of Vector2f components.
Added methods to compute absolute values of vector components.
Added methods to compute absolute values of vector components.
* @return a new Vector3f with non-negative components
*/
public Vector3f abs() {
return new Vector3f(Math.abs(x), Math.abs(y),Math.abs(z));

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Missing space after the comma between Math.abs(y) and Math.abs(z). The rest of this file (and the repo's documented style, Google style per CONTRIBUTING.md) uses a single space after argument commas.

Suggested change
return new Vector3f(Math.abs(x), Math.abs(y),Math.abs(z));
return new Vector3f(Math.abs(x), Math.abs(y), Math.abs(z));

* @return a new Vector4f with non-negative components
*/
public Vector4f abs() {
return new Vector4f(Math.abs(x), Math.abs(y),Math.abs(z),Math.abs(w));

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Same formatting issue as in Vector3f: missing space after the comma before Math.abs(z).

Suggested change
return new Vector4f(Math.abs(x), Math.abs(y),Math.abs(z),Math.abs(w));
return new Vector4f(Math.abs(x), Math.abs(y), Math.abs(z), Math.abs(w));

y = Math.abs(y);
z = Math.abs(z);
return this;
}

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

The six new public methods have no test coverage. jme3-core/src/test/java/com/jme3/math/Vector3fTest.java exercises essentially every other public method of Vector3f (including maxLocal, minLocal, negate), so abs()/absLocal() should get matching tests there. Worth asserting the edge cases the existing suite cares about: negative components, -0.0f (→ +0.0f), and NaN propagation. Vector2f/Vector4f have no test classes, so covering the Vector3f variants is the minimum.

@jaime-jmebot jaime-jmebot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

  • The added logic is correct: abs()/absLocal() follow the engine's ...Local naming, and Math.abs maps -0.0f to +0.0f and propagates NaN consistently with the rest of com.jme3.math.
  • Two formatting violations (missing space after a comma) in Vector3f.abs() and Vector4f.abs(); committable one-line fixes are posted inline.
  • No tests for the six new methods. Vector3fTest is the established home for per-method vector tests and should be extended to cover the abs variants, including -0.0f and NaN inputs.
  • Per CONTRIBUTING.md, non-trivial additions are expected to start with a forum thread; link the discussion in the PR description if one exists.

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.

2 participants