Enhancement: Provide frozen equivalents of Vec2 and Vec3

[pushfoo]

What should be added?

Vec2 and other classes were once immutable. They have since been made mutable for convenience and efficiency when changing attributes on objects.

It would be useful to add the following:

1. A frozen version of these classes that supports hashing in addition to the normal addition and subtraction operations
2. Convenience methods to make frozen copies of mutable originals

Why

It would be useful for defining immutable constants interoperable with the mutable versions. For example, immutability would prevent people from accidentally changing directions if a constant is passed as an argument to a function (ie game.constants.NORTH, etc):

# will raise errors if any attempt at in-place modification is made with += or similar
NORTH = FrozenVec2(0.0, 1.0)
WEST = FrozenVec(-1.0, 0.0)
NORTHWEST = NORTH + WEST

It would also be useful for defining immutable hashable versions that can be added to sets, dicts, or other containers and re-used later. This would be great for quickly prototyping path finding, but it would also be useful in other cases. For example, although the current version of Vec2 can be converted to tuples for storage, tuples cannot be added to mutable Vec2 instances:

>>> destinations.append(tuple(a_vec2_instance))
...
>>> position = Vec2(0,0)
>>> position += destinations.popleft()
Traceback (most recent call last):
  File "/usr/lib/python3.9/code.py", line 90, in runcode

[pushfoo]

I'm working on this with two goals:

1. Implementing all current functionality for FrozenVec* classes
2. Unit tests for current & new classes so we can make changes safely

This is my current plan for type interactions:

# binary operations always return a new instance
frozen  +  frozen   # creates a new frozen
frozen  +  mutable  # creates a new frozen
mutable +  frozen   # creates a new mutable
mutable +  mutable  # creates a new mutable

# in-place operations get more complicated
frozen  += frozen   # raises AttributeError
frozen  += mutable  # raises AttributeError
mutable += frozen   # modifies in-place
mutable += mutable  # modifies in-place

I'm not sure if I'll add the in-place operation behavior in the first PR.

[benmoran56]

That looks like a good plan. Your logic looks sound.

[olehermanse]

@pushfoo

frozen  +  mutable  # creates a new frozen
mutable +  frozen   # creates a new mutable

Why should these 2 be different?

[TomFryers]

frozen  += frozen   # raises AttributeError
frozen  += mutable  # raises AttributeError

Why not have these be equivalent to frozen = frozen + frozen and frozen = frozen + mutable?

[benmoran56]

@pushfoo

frozen  +  mutable  # creates a new frozen
mutable +  frozen   # creates a new mutable

Why should these 2 be different?

My feeling is that the leftmost object is the primary object, due to left -> right order of operations, so it makes sense to use that type for the result. I'm not sure of a specific rule.

[benmoran56]

frozen  += frozen   # raises AttributeError
frozen  += mutable  # raises AttributeError

Why not have these be equivalent to frozen = frozen + frozen and frozen = frozen + mutable?

I'm not speaking for @pushfoo here, but my opinion:

Since += implies in-place updating, it's probably better to be explicit with the frozen types. You can of course have += return a new object, but it wouldn't be clear to the user what was happening (unless they checked the object id). Since you would only be using frozen types in places where you don't expect them to be changed, the exception here makes more sense to me.

[pushfoo]

@TomFryers What Ben said is the exact reason for raising an AttributeError when code tries to use in-place operations with Frozen instances:

Since += implies in-place updating, it's probably better to be explicit with the frozen types. You can of course have += return a new object, but it wouldn't be clear to the user what was happening (unless they checked the object id). Since you would only be using frozen types in places where you don't expect them to be changed, the exception here makes more sense to me.

I'm rather certain about this design choice because it prevents all sorts of mistakes. For example:

# accidentally replacing a constant with a new value
NORTH += SOUTH

# replacing a local frozen value in-place silently
frozen_instance += mutable_or_frozen

The text of the raised exception can direct the user toward storing their mutable data in a non-frozen Vec class instead. These are mutable by default in master branch, and should be used whenever a vector can reasonably be expected to change.

[pushfoo]

@olehermanse

frozen  +  mutable  # creates a new frozen
mutable +  frozen   # creates a new mutable

Why should these 2 be different?

I apologize for the delay.

tl;dr: Ben got the gist of it. To my understanding, the alternatives will also be slower to execute, more complicated to implement, and generally more brittle.

I may be wrong and would appreciate any feedback or proposals for alternative behavior you can provide.

As to why I proposed the specific behavior you asked about:

1. Consistency

As Ben mentioned, it follows the general left-right order of operations used in standard infix notation:

the leftmost object is the primary object, due to left -> right order of operations, so it makes sense to use that type for the result.

With the behavior I outlined earlier, the leftmost type always determines what happens, including in-place modification operators and functions involving another vector such as lerp. Documentation can explain it as "freezing from the left".

2. Simple & Reasonably Fast Implementation

Implementing the idea above for binary operators/methods is simple, reasonably fast (no isinstance), and works well with inheritance:

def __add__(self, other: Vec2) -> Vec2:
    return self.__class__(self.x + other.x, self.y + other.y)

3. Support Orthogonal Concerns

1. Get a basic set of unit tests out the door for Vec types. We have none for Vec types at the moment, and adding them can help us safely make changes to specific behaviors before release.
2. Encourage using the in-place mutable types for stuff like position += velocity * dt

[TomFryers]

I see your point re preventing accidental reassignments. My worry is that this is different behaviour to all of the built-in immutable types, which may be unexpected.

[pushfoo]

@TomFryers Does this look more reasonable?

# binary operations always return a new instance
frozen  +  frozen   # new frozen
frozen  +  mutable  # new mutable
mutable +  frozen   # new mutable
mutable +  mutable  # new mutable

# frozen vectors do not support in-place operations
frozen  += frozen   # raises AttributeError
frozen  += mutable  # raises AttributeError
mutable += frozen   # modifies in-place
mutable += mutable  # modifies in-place

I'm not sure of the best way to implement it efficiently while still allowing inheritance. Maybe discarding inheritance is the way to go for now? I'm also unsure if there's a convenient way to imitate Java's final keyword applied to a class without adding dependencies.

My worry is that this is different behaviour to all of the built-in immutable types, which may be unexpected.

I'm not quite sure what you meant here. If you mean exception raising behavior applying to all inbuilt immutable types, I think it'd be reasonable. It would force people to be explicit so they don't override constants.

However, we could also limit it to only FrozenVec* types. We'll be doing this at first anyway since I don't intend to touch matrices in any PR related to this ticket, and matrices are all immutable already anyway.

[TomFryers]

I mean, for int, float, tuple and str, a += b is perfectly valid, and equivalent to a = a + b. Getting an error when DRYing a = a + b to a += b may be unexpected.

[pushfoo]

I mean, for int, float, tuple and str, a += b is perfectly valid, and equivalent to a = a + b. Getting an error when DRYing a = a + b to a += b may be unexpected.

I agree that it's unexpected, but that's intentional. None of the types you mentioned were created with the explicit intent of acting as module-level constants. Adding this behavior highlights that and can inform the user of it. We could also name the proposed Vec subclasses something like ConstantVec*, but that would break the precedent set by frozenset.

For the time being, I think I'll work on separating out the unit tests I've been working on from the preliminary Frozen implementations. The actual implementation details can wait until after we have the tests to validate them.

[TomFryers]

I think I'm viewing these types a bit differently. If added, I'd want to use them everywhere in place of their mutable counterparts, since mutability often causes bugs.

[pushfoo]

I'd want to use them everywhere in place of their mutable counterparts, since mutability often causes bugs.

Thank you for explaining. I understand your viewpoint a lot better now. I think the old Vec implementations were immutable and based on tuples for the same reasons you outlined. Maybe it's worth pursuing doing something similar down the line for FrozenVecs. For now, I'll stick to the current plan of adding unit tests before doing further work.

There might be ways of using advanced class features to accommodate everyone's priorities, but I don't know enough about them yet. One potentially unsatisfying way to deal with this ticket is to put it off until 3.8 is the minimum supported Python version. Doing so would allow the use of typing protocols and make implementation easier, in addition to allowing custom Vec types.

[benmoran56]

I'm strongly considering changing all Vec types to be immutable (tuple subclasses, like Mat3/4) for the upcoming v2.1 release. This is a breaking release, so a good chance to do it.

There were some usability arguments to having mutable Vec types (for instance player_position.x += dt), but in the end it seems to have caused more issues. In that example, it's probably not a good idea to be treating the Vector as a distinct object anyway.

[pushfoo]

Update per what ben said today: this is going to be closable on 2.1's release.