search

Unity-Technologies / Unity.Mathematics

The C# math library used in Unity providing vector types and math functions with a shader like syntax
Other
1.38k stars 156 forks source link

Revert "Move axes from math to float3 (#146)" #149

Closed unpacklo closed 4 years ago

unpacklo commented 4 years ago

This reverts commit e1340b2b26ed1d78f9a2f59da63c230c2462170d.

Reverting because this change broke burst compilation. This reopens DOTS-1356 and is now blocked.

unpacklo commented 4 years ago

@sheredom I added you so the burst team has some visibility. For now, the straightforward thing to do is just revert the change and go back to the way things were. It's not the end of the world if these helper axes remain in the math class, but I was surprised that adding a new member to the float3 struct caused burst to fail compiling.

xoofx commented 4 years ago

Note also that it would be a breaking change of the API, requiring to bump a major version of the package, it's not something that you can do lightly. as It would require also to bump burst to a major version. We should avoid this kind of refactoring until we break the explicit dependency between Burst and Unity.Mathematics.

unpacklo commented 4 years ago

Which part of this would have been a breaking change? CI didn't fail me during package validation so I assumed it wasn't breaking.

I had originally added this to math and then later thought it made more sense for it to be in float3, but the whole time these changes were unreleased.

xoofx commented 4 years ago

Which part of this would have been a breaking change? CI didn't fail me during package validation so I assumed it wasn't breaking.

Moving methods between math e.g math.up() to float3.up() is a breaking change. Projects that were using these methods in math would have their code broken after. But yeah, weird if the package validation didn't notify this... it could be that it doesn't really run correctly in fact, we had similar trouble in Burst at some point (for other reasons).

unpacklo commented 4 years ago

Ah, right. When I moved these around, I didn't actually move math.up since I saw it already existed and was public. I ended up creating a new float3.up which simply called math.up.

I'm starting to build up a list of breaking changes so that whenever we do want to pay the price of doing a major version bump, I can do a bunch of breaking changes at the same time.

xoofx commented 4 years ago

Ah, right. When I moved these around, I didn't actually move

Han ok, sorry for the noise then! 😅

I'm starting to build up a list of breaking changes so that whenever we do want to pay the price of doing a major version bump, I can do a bunch of breaking changes at the same time.

Yeah, good idea 👍