Minor additions

[MystMagus]

Just a few misc additions.

[eugeneko]

@MystMagus I have slight hostility towards proposed changes in IntVector.
Impilicit conversion from float to int may be source of errors.
Moreover, it can even break existing code:

IntVector2 i = IntVector2::ONE * 3;
i /= 3.2f;

It is (1, 1) before and (0, 0) after your PR, and I even cannot say that (0, 0) is more correct.
Even if this example is somewhat synthetic, I don't like this kind of traps.

Can you please elaborate in what use cases you need these operations?
Can you consider using explicit conversions instead:
VectorFloorToInt(Vector2(v) / 3.2f)
Note that this snippet behaves differently comparing to your code (in the negative area).
If you really need plain cast from Vector2 to IntVector2, extra ctor may be added.

[MystMagus]

@eugeneko I would say that (0, 0) in that example is more correct (after all, if you do int(3 / 3.2f) the result is 0). What I am using the conversion for is calculating int position in a grid based on screen position. Implicit conversion was causing it to become the wrong value in certain situations. I did not know that VectorFloorToInt existed, my bad for not looking harder. I do however think that the change I made makes sense.

[eugeneko]

I would say that (0, 0) in that example is more correct (after all, if you do int(3 / 3.2f) the result is 0)

After I wrote my previous comment, I came to the same conclusion -- that (0, 0) feels a bit more natural. So, one point for this change.

However, I have three points againts this change:

1. Urho legacy codebase isn't that good from casts perspective.
   There may be places in out codebase that are relying on current behavior and after this PR they will be silently broken.

2. If you enable warnings in old version, you will be warned if you try to divide IntVector2 by float in the exact place in the code where you do it.
   However, after this PR you will not get any warning about such op.

3. This point is somewhat abstract and a bit subjective, but I think math classes should be "pure" in a sense that they use exactly one scalar type. E.g. if you have vector or matrix of floats, or doubles, or ints, it should use only its scalar type in its whole interface.

I tried to make Urho math template several times. If I ever try it again and succeed, it would be simpler to maintain classes with "pure" interfaces than classes with mixed types.
Maintaining float&double mul&div for IntVector2 will require some compilcated SFINAE garbage.

Upd: I remember now why I failed. Manual script bindings. They suck so much. Maybe I'll do math refactoring in rbfx...

[MystMagus]

Are you sure there are any implicit calls with floats on IntVectors in the codebase? I tried setting the float operators to private, and it seems to compile without issue.

In any case, I don't really agree with the third point but it is as you say subjective. For the second one, dividing an int by a float is a legal operation though, and the current behavior results in the implicit conversion happening before the operation so the result isn't what you would normally expect.

[eugeneko]

Are you sure there are any implicit calls with floats on IntVectors in the codebase?

I'm not sure. However, Urho codebase is bigger than just this Urho3d/master.
And it's is not cool to silently break legit user code.

We already had similar issue before when @weitjong made StringHash case-sensitive.
Beneficial change on its own, but it has broken code for many people.

For the second one, dividing an int by a float is a legal operation though, and the current behavior results in the implicit conversion happening before the operation so the result isn't what you would normally expect

It's up to user whether to enable or disable this warning.
https://gcc.gnu.org/wiki/NewWconversion
Moving implicit conversion from user code to IntVector internals will deprive users from this choise.

I don't really agree with the third point but it is as you say subjective.

If math is converted to templates (and I'm going to do it eventually), these additional operators for IntVector2 will likely be removed because they don't fit template interface like Vector<T, N>. And this part is not subjective at all. This may never happen, but still keep it in mind, please.

[MystMagus]

Well, I've no more things to add. My main point remains that by doing the conversion before the actual operations, the result is not the logically expected one. Who is to say if rather than things relying on this, perhaps there are things that are stealthily not working correctly because of it at the moment.

[eugeneko]

Okay, I can point out one more reason why math classes should be pure.

Math classes already have dozens of operators for just one type.
If you add another type, you have a choise: make this interface several times more compliated, or make it inconsistent.

For example, why can I multiply IntVector2 and float, but cannot multiply IntVector2 and Vector2?
These operations are basically the same -- uniform and non-uniform vector scale.

If you add IntVector2 and Vector2 multiplication and disivion, then you should add addition and subtraction for consistency, and so on... You see where it's going.

By the way, you've already added one critical UX bug in this PR.
I wonder if you can see it now...

PS. I'd like to hear out others regarding these changes before doing anything

[weitjong]

@eugeneko can you cut the crap and point out what you think is the “critical bug”. We are contributing to the Urho3D project on their spare time and some may be like me reading all the comments on a phone in the toilet. Our focus should be constantly making Urho3D better and not to keep it stale so that the downstream forks which don’t merge back to upstream have an easy time. We should be allowed to break things
If we have to. Just a general comment, not really pertinent to this PR.

[rokups]

@eugeneko very accurately pointed out math class purity issue. While those additions do sound convenient and tempting to have i think we should be conservative on this one. These classes are core of everything, they should be perfect in every possible way.

[MystMagus]

@eugeneko I'm not trying to pick a fight with you you know. I am very well aware that it is not my decision how this goes forward, all I want to do is to make my case for this. I do not agree with some of the arguments you have put forward, you do not agree with some of mine. And no, I don't see the issue so please enlighten us.

[iSLC]

Maybe this might be a dumb suggestion. But what if we implement a thin wrapper type for fundamental types with an explicit constructor. So as to avoid implicit conversions.

Imagine this wrapper: https://gist.github.com/iSLC/adc5d643aa745ab55a5e6a9c948e647f

And then something like this:

using FloatType = NamedType< float >;

And an operator like:

IntVector2 operator *(FloatType rhs) const { return IntVector2(x_ * rhs.Get(), y_ * rhs.Get()); }

You can't implicitly do my_vec * 2.3f but you should be able to do my_vec * FloatType{2.3f}.

This should prevent the user from getting unwanted implicit operations. Bur still give you the flexibility you need without much performance cost. Since the compiler should be able to optimize the wrapper away.

I honestly have no idea if that code works. I'm at work and i can't test. But it's a dumb idea that can be considered and even gracefully rejected. It doesn't affect me either way.

[eugeneko]

@MystMagus

And no, I don't see the issue so please enlighten us.

1.8f * IntVector2(2, 2) and IntVector2(2, 2) * 1.8f produces different results after PR.
It's easy to fix, yeah.

I'm not trying to pick a fight with you you know. I am very well aware that it is not my decision how this goes forward, all I want to do is to make my case for this.

Nah, it's not about fights at all. I'm just trying to clarify my position.
I'm not going to fight against this PR if it will be decided to merge it, but I still think that these changes are against Urho math system design.

---

There are basically two ways how to design math library.

You can make it type-inclusive, so all the types are reasonably compatible with each other.
For example, HLSL. As long as dimensions are preserved, you can naturally mix types as if they are scalars. And I have nothing against this approach on its own -- it has its benefits.

On the other hand, you can make your library strict-typed, so all the types can be used only with their corresponding scalars. For example, GLSL. You must use same scalar type everywhere in calculations and explicitly cast if you need.

From your perspective, you expected Urho math behave like type-inclusive library, i.e. like built-in scalars.
So when you noticed weird behaviour, you considered it a bug in the vector.

From my perspective... well, I know that Urho math library is strict-typed.
Therefore I always cast all the types to common type in my code.
For me, current behavior is expected and the bug is in inaccurate user casts.

I'm fine with keeping Urho library strict-typed.
I'm fine with making Urho library type-inclusive.
But I'm not fine with making Urho library neither first nor second.

[MystMagus]

Fair enough, but if one were to make it strict wouldn't it be best to disable those implicit conversions completely (by having them as private non-implemented methods)? Not everyone thinks about turning on more warnings and the bugs that the conversion can cause at the moment are very subtle.

[eugeneko]

, but if one were to make it strict wouldn't it be best to disable those implicit conversions completely (by having them as private non-implemented methods)?

Didn't think about it... I like this idea.

Yes, this option sounds much better.
It may also break user code, but in a good way (compile-time error).
It will encourage people to write explicit conversions and more clean code.

These free functions should be enough to sink in all unwanted conversions:

inline void operator * (const IntVector2& lhs, double rhs) { static_assert(0, "Implicit conversion from float or double to int is not allowed"); }
inline void operator / (const IntVector2& lhs, double rhs) { ... }
inline void operator * (double lhs, const IntVector2& rhs) { ... }

inline void operator * (const IntVector3& lhs, double rhs) { ... }
inline void operator / (const IntVector3& lhs, double rhs) { ... }
inline void operator * (double lhs, const IntVector3& rhs) { ... }

Are there any other potentially harmful conversions? Cannot think of any.

What do you think?

[MystMagus]

Yes, that would work. I can get behind that. I guess we can wait a bit longer and see if anyone else has anything to add, and if not then you or I can add that.

[SirNate0]

Just my two cents, but I'm much more in favor of having the type stystem be more inclusive, maybe with a limitation that implicit conversions that cause loss of precision will be avoided to avoid some errors (I wouldn't mind if that possibility did exist though). But mostly that's because I don't like having to write really verbose code, so take that as you will.

[eugeneko]

but I'm much more in favor of having the type stystem be more inclusive, maybe with a limitation that implicit conversions that cause loss of precision will be avoided

@SirNate0 I think this is exactly the case here -- precision loss.

And inclusive math has its own issues, e.g. ambiguity.

E.g. you multiply IntVector2(1, 2) and 1.999997. Did you really want result to be trimmed to (1, 3)?
Or you just forgot to add rounding and wanted to get (2, 4)? Or maybe you wanted to get floating point Vector2 out of it?

Whatever option you choose, you hide possible bugs in user code.

[MystMagus]

I'm not sure static_assert will work for this, it seems to me like it always errors when done like this, even if the function isn't used.

[eugeneko]

@MystMagus Oh, my bad. Can you try deleted functions instead?

inline void operator * (const IntVector2& lhs, double rhs) = delete;
inline void operator / (const IntVector2& lhs, double rhs) = delete;
inline void operator * (double lhs, const IntVector2& rhs) = delete;

inline void operator * (const IntVector3& lhs, double rhs) = delete;
inline void operator / (const IntVector3& lhs, double rhs) = delete;
inline void operator * (double lhs, const IntVector3& rhs) = delete;

[MystMagus]

*= and /= cases as well, and made explicit IntVector(Vector) constructors for usability (and since Vector already has the reverse too).

Ed.: Actually should probably have been two commits in retrospect.

[eugeneko]

Thanks!

Just a minor note -- can you use static_cast instead of c-style casts in new ctors?
c-style casts are easier to write, sure, but they are pain in the ass to deal with later. I'd prefer not to propagate them.

E.g. it's trivial to find all places in codebase where you do const_cast or reinterpret_cast to revise them again, but it's impossible to reliably find all c-style casts in codebase. They are hidden and you have no means to find them except than thoughtfully look at every line of code.

[MystMagus]

Oh yeah sure, I just did the same one as in one of the other constructors. I'll do it tomorrow.

[eugeneko]

I have a feeling that the build is broken because you are trying to reference Vector2 in bindings at the point when it is not declared yet.
Probably this new ctor has to be moved from RegisterIntVector2 to RegisterVector2.

Oh god, I hate AS so much. I wish it had died already.

[rokups]

Oh god, I hate AS so much. I wish it had died already.

Hold my 🍺

[MystMagus]

Yes, I noticed just earlier this morning. In the process of fixing now.

[MystMagus]

I mashed the register functions together rather than putting some of them in the other, unsure if that is the preferred way. Considering the C++ classes are defined in the same file I think it should be okay?

[github-actions]

Stale pull request message