stride icon indicating copy to clipboard operation
stride copied to clipboard

Deprecate Vector*.Normalize()

Open manio143 opened this issue 4 years ago • 3 comments

Is your feature request related to a problem? Please describe. The Normalize() method on the vector classes modifies the vector in place. That may be convenient but can lead to issues when users don't realize they shouldn't call it for example on properties (because a copy of the value will be modified and discarded).

Describe the solution you'd like Ensure each vector class has two static methods

  • vec Normalize(in vec vector)
  • void Normalize(ref vec vector)

Put the [Obsolete] attribute on existing Normalize method with a comment pointing at the recommended one.

manio143 avatar Apr 27 '22 07:04 manio143

I would make those structs readonly and the first variant an instance method.

The parameter cannot be in or it clashes with the ref variant plus usually structs are already passed by reference at the assembly level and having an in modifier might cause the compiler to do defensive copies.

ericwj avatar Jul 19 '22 15:07 ericwj

Making those structs readonly is a huge breaking change, unlikely to ever happen as the code base and our users are already writing logic like vector.x = someValue which afair is illegal with a readonly structs. We can change the naming scheme to better reflect the function's behavior: static vec Normalized(in vec v); static void Normalize(ref vec v)

having an in modifier might cause the compiler to do defensive copies.

This has been fixed afaik, it will only do defensive copies if it couldn't assert that the method wasn't modifying the value, which didn't occur in all the cases I tried.

Eideren avatar Jul 20 '22 07:07 Eideren

Yeah I kind of expected that if it hasn't been readonly from the start. Still wouldn't change it. It'll hurt readability, discoverability of normalize variants and a small change in the <summary> and the fact it is void should be more than enough signals for everyone but the most novice C# programmers.

ericwj avatar Jul 20 '22 16:07 ericwj