In 4.0 generic max() and min() global functions should work as expected for Vector* types

[jordo]

Godot version:
4.0 latest

OS/device including version:
all OS/device

Issue description:
Notes this issue is 4.0 specific as 3.2.X doesn't expose these functions to any argument type and number of arguments

So I'm working on a project and doing a bunch of vector math in GDScript, and to my surprise I end up getting some really weird results with some computations. After the loss of a few hours trying to figure out what was going on it ended coming down to the addition of the generic global func max(Variant...) implemented in @reduz refactor to Variant operators here: f239780 (which I wholly appreciate as imo more refactoring and cleanup is greatly appreciated over new features).

But at the core of my issue was some GDScript that roughly looked like this:

    var a : Vector2 = Vector2(-15,4)
    var b : Vector2 = Vector2(0,0)
    var c : Vector2 = max(a,b)

var c in the above scenario evaluates to {0,0}. After discussion on IRC it was revealed that it's due to the implementation of max() in the generic case (

max(T a, T b) {
   return a < b ? b : a;
}

or iterating the ...varg in a loop and comparing with Variant::OP_LESS for more than 2 arguments.

I just want to point out that this was really unexpected to me. GDScript compiler/parser/VM & editor will happily run that code and return {0,0}. When brought up on IRC, the solution is just to 'document' it. Document that max() and min() use the < operator for arguments of the same type. I guess I sometimes feel the solution to just 'document the behaviour' is starting to be the solution to too many usability issues right now.

Anyways, I could totally be in the wrong. Maybe I'm totally off base with what max(Vector2, Vector2) should return, as I discovered a few believe my posted GDScript sample was what should be expected.

For myself, from a usability standpoint, I don't really want to read the docs on the max() function. If the function happily accepts two vectors, I expect the function to do vector math. Or, alternatively not be available at all (as is the case in 3.2.X), or perhaps give me a compiler/parser/runtime error.

** Steps to reproduce **
Run this Gdscript in 4.0:

    var a : Vector2 = Vector2(-15,4)
    var b : Vector2 = Vector2(0,0)
    var c : Vector2 = max(a,b)
    print(c)

[aaronfranke]

One could make an argument that the current behavior is the expected behavior. The current behavior is very consistent across types because it always uses the inequality comparison operators. I doubt this should be changed, it's good as-is, but maybe additional documentation would be a good idea.

[nhold]

I expect the function to do vector math.

To start with I don't know the possible use cases for either way. With a vector what max is could be defined however we want (The highest total components vs the highest length). I would be inclined to keep it as is unless there is no use case for the current usage and there is a great need to break consistency.

Right now you can easily work around this by providing your own max\min_vector_length functions

static func max_length_vector(vecA:Vector2, vecB:Vector2) -> Vector2:
	var magA = vecA.length()
	var magB = vecB.length()
	
	return vecB if magA < magB else vecA

[jordo]

I guess I sometimes feel the solution to just 'document the behaviour' is starting to be the solution to too many usability issues right now.

Response:

I doubt this should be changed, it's good as-is, but maybe additional documentation would be a good idea.

Lol, now that's just trolling...

But both these comments miss the whole point of what I said. And I'm sorry, whatever the underlying implementation reason for the behaviour is not a very good reason for 'why it's awkward'. As well, this doesn't 'break consistency' because it's not even available in 3.2. It's new in 4.0 (added a month ago), and it's awkward for this type.

The fact is absolutely nobody in their right mind is going to expect that max(Vector2, Vector2) is going to return the vector who's x-coordinate is the largest. Zero, nobody. It's awkward default behavior, and probably just straight up wrong. Pick any vector math library in any language and I will guarantee that this is not the behavoir of a max function on two vectors.

The point of bringing this up is to improve usability, nothing else. If you are aware of the underlying implementation then that's great for you but not intuitive for anyone else. And of course I can and will work around it. It's trivial, but that's totally missing the point.

[aaronfranke]

I guess I sometimes feel the solution to just 'document the behaviour' is starting to be the solution to too many usability issues right now.

I doubt this should be changed, it's good as-is, but maybe additional documentation would be a good idea.

Lol, now that's just trolling...

When did I say that I disagree with that statement? Additional documentation is a good idea. Your feelings were right on that documenting the behavior is a solution.

The fact is absolutely nobody in their right mind is going to expect that max(Vector2, Vector2) is going to return the vector who's x-coordinate is the largest. Zero, nobody.

Except for people who need to take a set of vectors and sort them in a defined order.

[jordo]

I guess I sometimes feel the solution to just 'document the behaviour' is starting to be the solution to too many usability issues right now.

I doubt this should be changed, it's good as-is, but maybe additional documentation would be a good idea.

Lol, now that's just trolling...

When did I say that I disagree with that statement? Additional documentation is a good idea. Your feelings were right on that documenting the behavior is a solution.

You're misinterpreting me. My point is that's not desired, and it's becoming too common to solve usability problems through 'documentation'. You don't want to use software that's unintuitive and have on documentation. The goal is to be MORE intuitive and MORE usable by default. That's a productivity win for everyone.

Except for people who need to take a set of vectors and sort them in a defined order.

Sorting a set vectors has nothing to do with taking the max of two vectors, I'm sorry. The only reason these are currently coupled together is an implementation detail. And an implementation that relies on the < operator (not the inequality comparison operator). And it's '<' operator is defaulted to just compare the x-coordinate, which is completely unintuitive. I would argue it's even unintuitive in your sorting example ( '<' should probably default to the lengthSq() of the vector ).

Anyways, like i mentioned, this won't be the default behavoir in any other vector math library in any language. If you want to expose max() to Vectors in 4.0, it should do the right thing.

[nhold]

What is the desired functionality and what is your use case? There is no defined 'right thing' as what constitutes a 'higher' vector it could be length or could be component based or it could be normalised most positive direction or even just highest y value. GLM\Kronos have less than functions that return a vector bool (So each component) and so does matlab it looks like.

https://github.com/g-truc/glm/blob/master/glm/detail/func_vector_relational.inl
https://au.mathworks.com/help/matlab/ref/lt.html

Here is the less than implementation of a Vector2, isn't this the one invoked by < operations in max? This only compares x if x is not equal, otherwise it compares y.

bool operator<(const Vector2 &p_vec2) const { return x == p_vec2.x ? (y < p_vec2.y) : (x < p_vec2.x); }

[jordo]

I want some expected behaviour by default or I don't want the behaviour at ALL. That's my use case.

You're right, there is no 'right thing' for what a max() function does when passed in Vectors by value. It's ambiguous by definition, which is why most API's are designed better with more explicit naming conventions. But the fact that godot exposes this now (it didn't before) means some consideration should be taken into what's expected now that calling max() on two vectors is completely valid and part of the new api design.

Forget about the implementation for a minute, because my issue as per the title is about the max() and min() global functions.

This is whats exposed in 3.2:

This is whats now exposed in 4.0:

So now in 4.0, it's perfectly valid to call max() on whatever number arguments and types, and godot is perfectly happy to give you a result. Not only that but there two other global functions called maxf() and maxi(), why even have these anymore? Promote the int/float to a variant and get rid of the complexity and api cruft.

So now what happens when we pass in Vector types. Ask yourself what should be printed below. (Vector2 are pass by value).

var a : Vector2 = Vector2(-15,4)
var b : Vector2 = Vector2(0,0)
var c : Vector2 = max(a,b)
print(c)

There's only three permutations. {-15,4}, {0, 4}, and {0,0}.

My whole point is that if you're going to expose an ambiguous API at the very least make the default functionality the one that's most intuitive, not the least.

If you don't understand my point, then run a poll on twitter or whatever and just design the API by committee. {0,0} will definitely be the LEAST expected result from the above usage.

I'm sorry but I don't want the global max() & min() functions as designed in 4.0. We should just get rid of them. That's great that they can pull out a max value for variadic NUMERIC arguments. But for just about any other Variant type it's just garbage and confusing by default. They will cost more hours then they will save.

If people really want to do this they can use the existing max() and min() functions in Array. Which should stay for backwards compatibility.

[nhold]

I want some expected behaviour by default or I don't want the behaviour at ALL. That's my use case.

Yep, lets define the current behaviour as expected behaviour. Now what?

I'm asking you what you think is intuitive or expected exactly. For me, none of the vector max potential implementations are intuitive and you can define anything to be the expected behaviour (Including returning an array of bools).

You're right, there is no 'right thing' for what a max() function does when passed in Vectors by value.

By right thing, I'm also saying no more intuitive or generally preferred expected result either.

It's ambiguous by definition, which is why most API's are designed better with more explicit naming conventions.

I don't think naming conventions matter...we can determine a Vector2\3 type and handle them specifically. The question is what do you think is intuitive and why? Even if we agree there is a problem you haven't given the replacement implementation.

EDIT:

There is a PR open right now that implements what I think you might be after (At least for vector2i):

https://github.com/godotengine/godot/pull/45278/files

in core/math/vector2.h

[0xafbf]

I was just bitten by this in the latest godot beta (8). I expected the max(vector, vector) function to return a vector with component-wise max values. I think it is the most useful (and expected use case) that way. It allows for straightforwardness, in cases where otherwise I have to unwrap a vector, do component-wise max, and then rewrap it in another vector

a simple use case would be finding a bounding box for possibly unordered values like so:
for example:

var world_space_start = Vector3(...)
var world_space_end = Vector3(...)
# first I project to screen space
var screen_space_start = camera.unproject_position(world_space_start)
var screen_space_end = camera.unproject_position(world_space_end)
# then find min and max in screen space
var screen_space_min = min(screen_space_start, screen_space_end)
var screen_space_max = max(screen_space_start, screen_space_end)
# otherwise i would have to do something like this:
var screen_space_min = Vector2(min(screen_space_start.x, screen_space_end.x), min(screen_space_start.y, screen_space_end.y))
var screen_space_max = Vector2(max(screen_space_start.x, screen_space_end.x), max(screen_space_start.y, screen_space_end.y))
# which impacts on readability, and maybe also on performance

hopefully this can be taken into consideration, although I am very sorry if this is being discussed elsewhere, I posted it here because it is the first place I found where its discussed

[groud]

I disagree, I think the max and min functions should behave the exact same independently of the used type. This means using the underlying < comparator for each types and return the highest/lowest one. min and max should not transform into component-wise max/min depending on the provided type. That would mean an inconsistent behavior and a lot of custom code, including custom processing of Vector3, Array, PackedStringArrays, etc...

I am not against a warning in the documentation though, as I understand it might be confusing. Something like:
warning: this function does not provide a component-wise max for arrays or Vector2/3 types, it instead compares them using their built-in comparator, and returns the max/min of the two provided arguments. A lexicographical comparator is used for most arrays or vector types.

[jordo]

I disagree entirely, min and max should do component wise min / max when operating with vector types as min / max are a common operation for use with vector types. I would definitely prefer the engine do what would be expected when working with these types. For sorting, everything should still use the < operator, so I don't think this really changes much and we could keep existing behaviour for all other types listed Array, PackedStringArrays, etc....

This is fundamentally an issue around min and max as exposed global functions, and how they operator on vector types. And I really disagree with keeping the current behaviour here because the current implementation is not useful for any scenario, confusing, and introduces bugs. The fact that it may be nice to keep the implementation consistent by using the underlying < comparator is an implementation detail and shouldn't drive the behaviour of the external interface.

[groud]

I disagree entirely, min and max should do component wise min / max when operating with vector types as min / max are a common operation for use with vector types.

This is not how min/max functions are implemented in most languages. If you want a component-wise operation, then you should be able call a specific method on the Vector2 itself, not a global operator that should behave more or less the same for all types. Something like Vector2.max_component() could be implemented I suppose.

I would definitely prefer the engine do what would be expected when working with these types. For sorting, everything should still use the < operator, so I don't think this really changes much and we could keep existing behaviour for all other types listed Array, PackedStringArrays, etc....

What you expect is not necessarily what other expect. The max thing as it is right now is how I would expect it to work, as it makes total sense for sorting or search algorithms.

We discussed on rocketchat to disable this built-in function for vectors or arrays, as it's indeed confusing to newcomers. That would be ok to me as is min/max operators when you can use < are not that useful anyway. But I will be strictly against changing the built-in functions behavior just for vectors, as it would be very inconsistent.

[jordo]

What you expect is not necessarily what other expect.

Absolutely correct, but please consider this also applies to yourself as well.

The max thing as it is right now is how I would expect it to work, as it makes total sense for sorting or search algorithms.

You are conflating the issue by relating max to sorting and search algorithms... max is not used in either of those, a comparator function is used (which return's a bool)... ex: 'less than' or 'greater than'.

[groud]

Absolutely correct, but please consider this also applies to yourself as well.

Of course. My point was that, considering one behavior does not seem to be 100% consensual (which is fine), then we should make sure that such function are at least consistent. Either max/min should always return the highest/lowest of the two element (using the < operator, which is lexicographical for vectors), OR returns a new element that has each of its components as the max/min of the two input elements.

But a single function should not do both IMO.

[0xafbf]

This is not how min/max functions are implemented in most languages.

But in shading languages, which are very related, it is how it works.

Either max/min should always return the highest/lowest of the two element (using the < operator, which is lexicographical for vectors)

I don't understand why this would be useful. I would understand when used with arrays (for example, for getting which would be first when alphabetically sorted). But for vectors, I see this as imposing ourselves a constraint that I can't come up with a use case for.

And in that case, I agree with jordo, min/max shouldn't be related to sorting problems. if anything, I would say that Arrays shouldn't have a min/max implementation, although they could still use the < operator.

But I will be strictly against changing the built-in functions behavior just for vectors, as it would be very inconsistent.

I think the opposite. having it do component-wise min/max makes it more consistent with what people expects. (at least, experienced people I think.). I understand that it would add complexity to the underlying system. but I think the example given above shows how it reduces complexity for user problems.

[vpellen]

Any standard mathematical operation performed on a vector should be performed component-wise. That is the conventional behavior, and the expected behavior for anybody who has ever used vector math to accomplish anything non-trivial, including myself. If I perform a comparison between two vectors, I expect to either receive a vector of bools, or, failing that, an error. If I compare two vectors and get a lexographic comparison as a result, I file a bug report, because That's Not How Vector Math Works.

The min/max scenario is even worse, because not only does it produce a value you don't expect, it produces a value you don't expect in a type that you do. This is a bug that absolutely would have bitten me in the ass eventually, and frankly, I'm surprised it hasn't already, so props to jordo for that.

To call this a documentation error is thoroughly asinine.

[akien-mga]

I'll just remove those methods since there's no consensus on what they should do.

[vpellen]

I suppose even no implementation is better than the current one.

[Geometror]

So... I did a bit of research :)
Languages/Libraries/Engines with component-wise min/max functions:

• GLSL and GLM
• HLSL
• Unity
• Unreal Engine
• Bevy
• three.js
• MATLAB
• Numpy (more or less, there is no explicit vector type, so arrays can be used with minimum/maximum
• Eigen3 (min/max for array types, cwiseMin/cwiseMax for matrix types)
• our own shading language (including VisualShader min/max nodes)

Some like Irrlicht, OGRE or O3DE seemingly don't have min/max functions defined for vector types.
GMTL has generic min/max functions, but I think they don't implement the order defining operators for vector types, so the user has to define the behavior themselves.

In my opinion, we should follow the principle of least surprise and have component-wise min/max functions which just do what they do everywhere else.

[Geometror]

Oh no, somehow I overlooked/forgot the fact that this issue is about the global min/max functions. That changes some things. I think we should definitely expose component wise min/max functions for each vector type, but regarding the global ones I tend to agree that the current behavior is expected and probably the most consistent. I decided to keep the answer above, maybe it can still add some value to the discussion.