Bug in Slice.java

[andreas]

Hello William,

While working with the slice shape I realized that it won't react physically correct, if it is rotated before it is attached to a body.
Therefore I reviewed your code and I think I found the problem:

You used the expression localAxis.y in the Method getFarthestFeature to decide which Feature should be returned. This only works if the local X-Axis is (1,0) or if you rotate localAxis first:

Code: Select all

   /* (non-Javadoc)
    * @see org.dyn4j.geometry.Convex#getFarthestFeature(org.dyn4j.geometry.Vector2, org.dyn4j.geometry.Transform)
    */
   @Override
   public Feature getFarthestFeature(Vector2 vector, Transform transform) {
      Vector2 localAxis = transform.getInverseTransformedR(vector);
      if (Math.abs(localAxis.getAngleBetween(this.localXAxis)) <= this.alpha) {
         // then its the farthest point
         Vector2 point = this.getFarthestPoint(vector, transform);
         return new PointFeature(point);
      } else {
         // check if this section is nearly a half circle
         if ((Math.PI - this.theta) <= 1.0e-6) {
            // if so, we want to return the full back side
            return Segment.getFarthestFeature(this.vertices[1], this.vertices[2], vector, transform);
         }
         // otherwise check which side its on

         //without inserting the next line you cannot decide on localAxis.y which segment you have to return
         localAxis.rotate(this.localXAxis.getAngleBetween(X_AXIS));

         if (localAxis.y > 0) {
            // then its the top segment
            return Segment.getFarthestFeature(this.vertices[0], this.vertices[1], vector, transform);
         } else if (localAxis.y < 0) {
            // then its the bottom segment
            return Segment.getFarthestFeature(this.vertices[0], this.vertices[2], vector, transform);
         } else {
            // then its the tip point
            return new PointFeature(transform.getTransformed(this.vertices[0]));
         }
      }
   }


While I reviewed your code I didn't understand your calculations of the radius. Maybe I just don't understand which radius should be calculated. Please correct me, if I'm wrong.

In Line 70 the radius is calculated the following way:

Code: Select all

   private Slice(boolean valid, double radius, double theta, Vector2 center) {
      super(center, Math.max(center.x, radius - center.x));


In AbstractShape it is written that the radius should be the maximum radius. The orange line should always be greater than "radius - center.x", shouldn't it?



Therefore I suggest the following change:

Code: Select all

   private Slice(boolean valid, double radius, double theta, Vector2 center) {
      super(center, Math.max(center.x, center.distance(new Vector2(radius, 0).rotate(0.5*theta))));


There should be a similar situation in getRadius:

Code: Select all

   /* (non-Javadoc)
    * @see org.dyn4j.geometry.Shape#getRadius(org.dyn4j.geometry.Vector2)
    */
   @Override
   public double getRadius(Vector2 center) {
      return this.radius + center.distance(this.center);
   }


Here I think the only possible situation where the radius isn't the maximum distance beween the given center and one of the vertices is if the center is in the shown region:



Therefore I would check if Math.abs(center.getAngleBetween(localXAxis))>=Math.PI-alpha to decide which way the radius should be calculated:

Code: Select all

   /* (non-Javadoc)
    * @see org.dyn4j.geometry.Shape#getRadius(org.dyn4j.geometry.Vector2)
    */
   @Override
   public double getRadius(Vector2 center) {
      return Math.abs(center.getAngleBetween(localXAxis))>=Math.PI-alpha?this.radius + center.getMagnitude():Geometry.getRotationRadius(center, this.vertices);
   }


Anyway Dyn4j is one of the best things I learnt so far.

Best regards Andreas

[William]

Thank you for reporting this. You are correct on all accounts, these are certainly bugs that need to be fixed. I'm working on them now.

There are similar bugs with the Capsule, Ellipse, and HalfEllipse shapes as well (namely the getRadius(Vector2) methods).

Thanks,
William

[andreas]

By the way, while seaching for them, I couldn't extend your AbstractShape in an own package, because the two member variables center and radius don't have the protected visibility. Is there any reason for this?
Having them protected visibility, it would be much easier to create own Shapes based on Abstract Shape.

Greetings
Andreas

[William]

The intent is that both center and radius would be effectively final. That said it might make sense to make these protected and non-final so that implementing classes can still mutate them after the super constructor. I'll give it some thought.

William

[andreas]

Sorry ... this was my fault. I didn't want to mutate them ... I just didn't realize that there are getter Methods. I copied the code from github and saw the errors in my IDE. Then I just copied the AsbtractShape, too instead of searching for ohter solutions.

My apologies for bothering you with that (stupid) request.
Andreas

[freerun999]

Thank you for reporting this.
วิธีแทงบอล

[William]

Thanks again for the bug report! I've released a new version of the library with fixes included.

William