Search Unity

  1. Unity Asset Manager is now available in public beta. Try it out now and join the conversation here in the forums.
    Dismiss Notice
  2. Megacity Metro Demo now available. Download now.
    Dismiss Notice
  3. Unity support for visionOS is now available. Learn more in our blog post.
    Dismiss Notice

[Suggest] Remove `Vector3Int(Vector3 v)` constructor and create static factroy methods.

Discussion in '2D Experimental Preview' started by RyotaMurohoshi, Nov 12, 2016.

  1. RyotaMurohoshi

    RyotaMurohoshi

    Joined:
    Jun 20, 2013
    Posts:
    24
    In Release 2, Vector3Int has constructor that takes Vector3 argument.

    I suggest remove the Vector3Int(Vector3 v) constructor and create static factroy methods,
    • Vector3Int.FloorToInt(Vector3 v)
    • Vector3Int.RoundToInt(Vector3 v)
    • Vector3Int.CeilToInt(Vector3 v)
    I'll describe it.

    Please check next test result.

    Code (CSharp):
    1.  
    2.    [Test]
    3.     public void Vector3ConstructorTest()
    4.     {
    5.         Assert.AreEqual(
    6.             new Vector3Int(0, 0, 0),
    7.             new Vector3Int(new Vector3(0, 0, 0))
    8.         );
    9.  
    10.         Assert.AreEqual(
    11.             new Vector3Int(1, 2, -1),
    12.             new Vector3Int(new Vector3(1.0F, 2.0F, -1.0F))
    13.         );
    14.  
    15.         Assert.AreEqual(
    16.             new Vector3Int(1, 2, -1),
    17.             new Vector3Int(new Vector3(1.2F, 2.1F, -0.7F))
    18.         );
    19.     }
    20.  
    And next code is the Vector3Int constructor implementation (via MonoDevelop assembly browser).

    Code (CSharp):
    1.  
    2.         public Vector3Int (Vector3 v)
    3.         {
    4.             this.x = Mathf.FloorToInt (v.x);
    5.             this.y = Mathf.FloorToInt (v.y);
    6.             this.z = Mathf.FloorToInt (v.z);
    7.         }
    8.  
    Vector3Int(Vector3 v) uses FloorToInt in its implementation.



    I think that the Vector3Int(Vector3 v) is not good, because the constructor is ambiguous and easy to be misunderstood.

    I think that
    most users expect Vector3Int(Vector3 v) uses FloorToInt in the constructor implementation
    but some users expect Vector3Int(Vector3 v) uses RoundToInt in the constructor implementation.
    (Maybe, few users expect Vector3Int(Vector3 v) uses CeilToInt in the constructor implementation.)

    So the Vector3Int(Vector3 v) behaviour is ambiguous and someone may make mistake with the behaviour.
    (Of course, reference document will show Vector3Int(Vector3 v) behaviour exactly, but some users don't read reference document.)
    (I understand the current Vector3Int(Vector3 v) constructor is useful for TileMap. But...)


    So, I suggest 2 issues.
    • remove Vector3Int(Vector3 v) constructor.
    • added static factory methods
      • Vector3Int.FloorToInt(Vector3 v)
      • Vector3Int.RoundToInt(Vector3 v)
      • Vector3Int.CeilToInt(Vector3 v)
    The static factory methods behaviour are so clear from their names. Their implementation are like next code.

    Code (CSharp):
    1.  
    2. public static Vector3Int FloorToInt(Vector3 v)
    3. {
    4.     return new Vector3Int(
    5.         Mathf.FloorToInt (v.x),
    6.         Mathf.FloorToInt (v.y),
    7.         Mathf.FloorToInt (v.z)
    8.     );
    9. }
    10.  

    Code (CSharp):
    1.  
    2. public static Vector3Int RoundToInt(Vector3 v)
    3. {
    4.     return new Vector3Int(
    5.         Mathf.RoundToInt (v.x),
    6.         Mathf.RoundToInt (v.y),
    7.         Mathf.RoundToInt (v.z)
    8.     );
    9. }
    10.  
    Code (CSharp):
    1.  
    2. public static Vector3Int CeilToInt(Vector3 v)
    3. {
    4.     return new Vector3Int(
    5.         Mathf.CeilToInt (v.x),
    6.         Mathf.CeilToInt (v.y),
    7.         Mathf.CeilToInt (v.z)
    8.     );
    9. }
    10.  

    And Vector2Int(Vector2 v) constructor is same.
    • remove Vector2Int(Vector2 v) constructor.
    • added static factory methods
      • Vector2Int.FloorToInt(Vector2 v)
      • Vector2Int.RoundToInt(Vector2 v)
      • Vector2Int.CeilToInt(Vector2 v)
     
  2. Xelnath

    Xelnath

    Joined:
    Jan 31, 2015
    Posts:
    402
    This seems like a wise idea.
     
    RyotaMurohoshi likes this.
  3. RyotaMurohoshi

    RyotaMurohoshi

    Joined:
    Jun 20, 2013
    Posts:
    24
    In Release3, Vector3Int does not have constructor with Vector3 and Vector3Int has CeilToInt, FloorToInt and RoundToInt.

    Unity 2D development team members! Thank you for your adopting my suggestion!!!

    By the way, I think it is breaking change.But there are no description about it in Release Note for Release3.

    How about add them.


    * Remove Vector3Int(Vector3) constructor
    * Remove Vector2Int(Vector2) constructor
    * Add Factory methods to create Vector3Int: Vector3Int.CeilToInt, Vector3Int.FloorToInt, Vector3Int.RoundToInt
    * Add Factory methods to create Vector2Int: Vector2Int.CeilToInt, Vector2Int.FloorToInt, Vector2Int.RoundToInt
     
    Xelnath likes this.
  4. Hokis

    Hokis

    Joined:
    Oct 26, 2012
    Posts:
    5
    Agreed that it should be in the release notes: this was a breaking change for me. More inconvenient than the Tilemaps/TileMap syntax change, funnily enough!
     
  5. UNITY3D_TEAM

    UNITY3D_TEAM

    Joined:
    Apr 23, 2012
    Posts:
    720