Added component-wise min and max functions for vectors
#72316
Conversation
|
LGTM (just need to fix the static checks) Not sure about the way it's done in variant though, I'll take a closer look later. Would be good to add tests for the new methods |
0xafbf
force-pushed
the
component-wise-minmax
branch
from
061b17a
to
5c46b24
Compare
0xafbf
force-pushed
the
component-wise-minmax
branch
2 times, most recently
from
a6dd4f6
to
7c73a92
Compare
0xafbf
force-pushed
the
component-wise-minmax
branch
from
7c73a92
to
164b42a
Compare
|
I think that I got the changes right this time, let me know if you think I should make any change in the variant code. |
|
Better but still needs work. Now they fail if you pass a combination of ints and floats. Not sure what the best way to fix this would be, maybe someone more experienced can help. |
core/variant/variant_utility.cpp
Outdated
| Variant::evaluate(Variant::OP_LESS, base, *p_args[i], ret, valid); | ||
| if (!valid) { | ||
| const Variant &x = *p_args[i]; | ||
| if (x.get_type() != base_type) { |
There was a problem hiding this comment.
A special case in this condition for ints and floats would be sufficient for now. (Likewise for min())
|
As you have seen on the issue you linked, the asked feature is controversial. Several users (including myself) are not happy with the fact According to a discussion we had with other maintainers, we are more leaning towards removing this helper for values that are not basic types like int, float, bool (basically everywhere the current behavior can confuse users). We however believe the component-wise max/min could be exposed, either as a method of Vector2/3 (like, |
We got that already, but only for Vector2{i}, and not in Vector3/4. Though this PR implemented it for the remaining vector types. I also want bring up that in GlobalScope methods like |
There was a problem hiding this comment.
I think for now it's fine to implement the component-wise min/man methods for Vector3(i) and Vector4(i), but whether the global min/max functions should operate component-wise is still controversial (see comments in #45320). Could you strip the changes to the global functions from this PR?
0xafbf
force-pushed
the
component-wise-minmax
branch
from
164b42a
to
a90e151
Compare
|
Thanks! |
|
This has yet to be implemented for the GDScript side... Any progress? |
|
No it has been implemented for scripting a month ago: |
After looking a bit further into it, it seems that it was only implemented for Godot Latest/4.3. Any plans to cherry-pick this change for Godot 4.2 and lower? |
|
No, this is a feature and not a patch. |
Implements changes needed to solve this issue: #45320
EDIT:
This PR doesn't solve the issue mentioned above. This PR only implements the min/max functions for the vector classes in the cpp side, but doesn't change the way min/max works in GDScript. That change may come with another PR at a later time.