Commons Math
  1. Commons Math
  2. MATH-1117

twod.PolygonsSet.getSize produces NullPointerException if BSPTree has no nodes

    Details

    • Type: Bug Bug
    • Status: Closed
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: 3.2
    • Fix Version/s: 3.3
    • Labels:
      None
    • Environment:

      Mac OS 10.9, Java 6, 7

      Description

      org.apache.commons.math3.geometry.euclidean.twod.PolygonsSet.getSize() uses a tree internally:

      final BSPTree<Euclidean2D> tree = getTree(false);

      However, if that tree contains no data, it seems that the reference returned is null, which causes a subsequent NullPointerException.

      Probably an exception with a message ("tree has no data") would clarify that this is an API usage error.

      1. Report3_1.java
        0.3 kB
        Cyrille Artho
      2. Report3.java
        0.5 kB
        Cyrille Artho

        Activity

        Transition Time In Source Status Execution Times Last Executer Last Execution Date
        Open Open Resolved Resolved
        12d 13h 54m 1 Luc Maisonobe 26/Apr/14 17:57
        Resolved Resolved Closed Closed
        22d 22h 16m 1 Luc Maisonobe 19/May/14 16:13
        Luc Maisonobe made changes -
        Status Resolved [ 5 ] Closed [ 6 ]
        Hide
        Luc Maisonobe added a comment -

        Closing all resolved issue now available in released 3.3 version.

        Show
        Luc Maisonobe added a comment - Closing all resolved issue now available in released 3.3 version.
        Hide
        Luc Maisonobe added a comment -

        Yes, these constructors are used outside of their package. Typically, the constructor for polygons (in 2D) is called from the split method in SubPlane and also from an internal class in OutlineExtractor, both in the 3D package. As the methods have been public for a while, they may also be used from user code. The call from the split method is a very important one: it is used a very large number of times as part of building 3D BSP trees, and here the tree has been built using a complex algorithm to ensure it is correct, so we don't want to visit all its leafs to check it.

        Thanks for the hint about the typos, I have fixed them now.

        Show
        Luc Maisonobe added a comment - Yes, these constructors are used outside of their package. Typically, the constructor for polygons (in 2D) is called from the split method in SubPlane and also from an internal class in OutlineExtractor, both in the 3D package. As the methods have been public for a while, they may also be used from user code. The call from the split method is a very important one: it is used a very large number of times as part of building 3D BSP trees, and here the tree has been built using a complex algorithm to ensure it is correct, so we don't want to visit all its leafs to check it. Thanks for the hint about the typos, I have fixed them now.
        Hide
        Cyrille Artho added a comment -

        Thanks for clarifying this one. I guess making these constructors package-private may break existing code so the documentation is the best way to warn the user.
        By the way, the new documentation has a typo in "task" (currently "taks"). The same typo is also in PolyhedronsSet.java. Of course that's easy to fix

        Show
        Cyrille Artho added a comment - Thanks for clarifying this one. I guess making these constructors package-private may break existing code so the documentation is the best way to warn the user. By the way, the new documentation has a typo in "task" (currently "taks"). The same typo is also in PolyhedronsSet.java. Of course that's easy to fix
        Luc Maisonobe made changes -
        Status Open [ 1 ] Resolved [ 5 ]
        Fix Version/s 3.3 [ 12324600 ]
        Resolution Fixed [ 1 ]
        Hide
        Luc Maisonobe added a comment -

        The two problems you report are completely different from each other.

        The first report is really a wrong API use. The call did not fulfill the constraints that were already given in
        the javadoc, i.e. that the provided tree MUST have proper Boolean attributes at leaf nodes. There is no
        verification here for performance reasons (it would imply walking the full trees all the time). I really don't
        think it would be a good idea to do such verifications. So I only improved the javadoc of the constructor
        to make it more clear this constructor is for expert use only (building a tree is difficult) and that there is
        no verifications, adding in the documentation that failing to provide appropriate arguments is the
        responsibility of users.

        In fact, general users should never use this specific constructor, but should rely on the other ones which
        are there precisely to avoid this kind of errors : the other constructors ensure the tree is correct before
        calling this constructor.

        The second one is a real problem, I have fixed it (see r1590251).

        Please reopen the issue if you do not agree with the fix for the first problem, as it is documentation only.

        Show
        Luc Maisonobe added a comment - The two problems you report are completely different from each other. The first report is really a wrong API use. The call did not fulfill the constraints that were already given in the javadoc, i.e. that the provided tree MUST have proper Boolean attributes at leaf nodes. There is no verification here for performance reasons (it would imply walking the full trees all the time). I really don't think it would be a good idea to do such verifications. So I only improved the javadoc of the constructor to make it more clear this constructor is for expert use only (building a tree is difficult) and that there is no verifications, adding in the documentation that failing to provide appropriate arguments is the responsibility of users. In fact, general users should never use this specific constructor, but should rely on the other ones which are there precisely to avoid this kind of errors : the other constructors ensure the tree is correct before calling this constructor. The second one is a real problem, I have fixed it (see r1590251). Please reopen the issue if you do not agree with the fix for the first problem, as it is documentation only.
        Cyrille Artho made changes -
        Attachment Report3_1.java [ 12640029 ]
        Hide
        Cyrille Artho added a comment -

        Just calling the constructor with the right numbers produces the same effect, for example this one-liner:

        new org.apache.commons.math3.geometry.euclidean.twod.PolygonsSet(0.0d, 0.0d, 0.0d, 10.3206397147574d);

        Show
        Cyrille Artho added a comment - Just calling the constructor with the right numbers produces the same effect, for example this one-liner: new org.apache.commons.math3.geometry.euclidean.twod.PolygonsSet(0.0d, 0.0d, 0.0d, 10.3206397147574d);
        Cyrille Artho made changes -
        Attachment Report3.java [ 12640013 ]
        Cyrille Artho made changes -
        Field Original Value New Value
        Attachment Report3.java [ 12640013 ]
        Attachment Report3.java [ 12640014 ]
        Hide
        Cyrille Artho added a comment -

        JUnit test to reproduce problem.

        Show
        Cyrille Artho added a comment - JUnit test to reproduce problem.
        Cyrille Artho created issue -

          People

          • Assignee:
            Unassigned
            Reporter:
            Cyrille Artho
          • Votes:
            0 Vote for this issue
            Watchers:
            2 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development