Unused method arguments in `Rot`. Can it be removed?

[gkjpettet]

These two methods in common/rot.dart:

Vector2 getXAxis(Vector2 xAxis) => Vector2(cos, sin);

Vector2 getYAxis(Vector2 yAxis) => Vector2(-sin, cos);

Take a Vector2 but as far as I can see they aren't used. It looks like dynamics/fixture.dart calls this method and passes in a Vector2 but I think it is defunct.

I'm trying to port this library to another language and I just hope I'm not missing anything here.

[gkjpettet]

In fact I'm sure this is a bug. Looking at the JBox2D Rot.java code:

 public void getXAxis(Vec2 xAxis) {
    xAxis.set(c, s);
  }

  public void getYAxis(Vec2 yAxis) {
    yAxis.set(-s, c);
  }

It looks like it should be:

Vector2 getXAxis(Vector2 xAxis) {
    xAxis.x = cos
    xAxis.y = sin
}
Vector2 getYAxis(Vector2 yAxis) {
    yAxis.x = -sin
    yAxis.y = cos
}

[spydon]

It should be an optional (and be used), it is used to minimize the amount of objects created.

[gkjpettet]

@spydon: So do you think that mean it's a bug?

In Forge2D/fixture.dart there is this code:

renderCenter.setFrom(Transform.mulVec2(xf, circle.position));
final radius = circle.radius;
xf.q.getXAxis(renderAxis);

(it's actually the only code in the library that calls getXAxis()). I'm guessing that renderAxis will be unaltered but I don't deeply understand the code to know if that's an issue.

I only ask because I am porting the Rot class at the moment and am unclear whether to omit the parameter or not.

[spydon]

That does look like a bug indeed. It depends on what language you're porting to whether you need it or not, depending on how expensive it is to create a new vector object in the language.

I'd implement it something like this so that the user at least have the possibility to re-use a vector object:

Vector2 getXAxis({Vector2? out}) {
  final result = out ?? Vector2.zero();
  return result..setValue(cos, sin);
}

[gkjpettet]

Thank you for clearing this up. I'm going to use your suggested fix for the getXAxis() and getYAxis() method calls.

I'll leave this issue open though so the team can correct the bug.