-
-
Notifications
You must be signed in to change notification settings - Fork 21.6k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add Vector4 #59970
Add Vector4 #59970
Conversation
Oh, that's cool. I would also go through all Color/Alpha ports/inputs in visual shader and unify them into single vec4. |
@Chaosus Shouldn't Color ports be using Color, not Vector4? |
editor/icons/Vector4.svg
Outdated
xmlns:inkscape="http://www.inkscape.org/namespaces/inkscape" | ||
xmlns:sodipodi="http://sodipodi.sourceforge.net/DTD/sodipodi-0.dtd" | ||
xmlns="http://www.w3.org/2000/svg" | ||
xmlns:svg="http://www.w3.org/2000/svg"> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This file can be minified with svgcleaner
(plus ensure there's a newline at the end).
// Constants | ||
private static readonly Vector4 _zero = new Vector4(0, 0, 0, 0); | ||
private static readonly Vector4 _one = new Vector4(1, 1, 1, 1); | ||
private static readonly Vector4 _inf = new Vector4(Mathf.Inf, Mathf.Inf, Mathf.Inf, Mathf.Inf); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's likely worth adding some constants for the basic unit vectors. It's not sensible to give friendly names like "up" and "right", but we can still declare some constants.
private static readonly Vector4 _unitX = new Vector4(1, 0, 0, 0);
private static readonly Vector4 _unitY = new Vector4(0, 1, 0, 0);
private static readonly Vector4 _unitZ = new Vector4(0, 0, 1, 0);
private static readonly Vector4 _unitW = new Vector4(0, 0, 0, 1);
{ Variant::TRANSFORM2D, 6 * sizeof(float), 6 * sizeof(float), 6 * sizeof(double), 6 * sizeof(double) }, | ||
{ Variant::PLANE, (vec3_elems + 1) * sizeof(float), (vec3_elems + 1) * sizeof(float), (vec3_elems + 1) * sizeof(double), (vec3_elems + 1) * sizeof(double) }, | ||
{ Variant::PLANE, vec4_elems * sizeof(float), vec4_elems * sizeof(float), vec4_elems * sizeof(double), vec4_elems * sizeof(double) }, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This change doesn't make sense semantically. A plane data structure is composed of a normal vector (for 3D, that's a Vector3), and the distance from the origin.
I think that the entire purpose of vec3_elems
is to make the code more semantic, since Vector3 is used inside of many other structures, so this change doesn't make much sense. Also, since Vector4 isn't used inside of other structures, it probably doesn't make much sense to define vec4_elems
, since we don't have rect2_elems
or similar.
Maybe, at first glance, shaders does not have "color" type, it's syntax sugar over |
core/io/resource_format_binary.cpp
Outdated
v.x = f->get_32(); | ||
v.y = f->get_32(); | ||
v.z = f->get_32(); | ||
v.w = f->get_32(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
v.x = f->get_32(); | |
v.y = f->get_32(); | |
v.z = f->get_32(); | |
v.w = f->get_32(); | |
v.x = f->get_real(); | |
v.y = f->get_real(); | |
v.z = f->get_real(); | |
v.w = f->get_real(); |
As I mentioned on the proposal, I am not convinced by the usefulness of this. The only situation I think they might be really useful are Quaternions, for which an explicit type makes more sense. For other places like @aaronfranke suggested, I think keeping dedicated types for Color, Plane or Rect2 is better and makes a lot more sense than using a generic Vector4 type. For example, there's no reason why you would add two Rect2 represented as Vector4, as it would basically mean adding the position and the size together, this seems like a quite useless thing to do. I general, for future PRs, I would suggest you to ask maintainers (for core things like that, likely reduz) for an approval. Otherwise all of this might end up in a lot of wasted work. |
@groud Sorry, I am not sure if I understood you correcly. My intention here is not to replace these specialized types but to supplement them by a general 4D vector which can be used in all cases where Plane, Quaternion, Rect2 and Color aren’t suitable. In my opinion, using Plane for a tuple which does not represent a plane at all feels hacky. Some use cases in which Plane/Quaternion/Rect2/Color make no sense or aren’t fitted (as @aaronfranke stated in godotengine/godot-proposals#629):
Looking at the amount of upvotes godotengine/godot-proposals#629 got, this seems to be a feature which many users miss in Godot. |
The problem I see with most of those use cases, is that none of them benefit from what a Vector4 would provide. The point of creating a new type is mainly to allow manipulating them as a single element, where addition, multiplication, or any other operation generic to a Vector would make sense. In most of you examples, namely:
In all those examples, there's always a 4th component that needs to be manipulated independently of the other ones. Which makes a Vector4 type a lot less interesting, as you will need a lot of helper functions to manipulate them according to the underlying type of object they represent anyway. In the end, the only advantage they provide is that, in some cases, they would be more efficient than returning a dict when used as a generic container for four float values. In my opinion, this highlights the fact that we instead kind of need a better support of tuples. Or maybe make users aware it is simply advised to create a dedicated class for their use case, when they need it, to represent a set of packed values. |
I agree with @groud, the use for this is too limited, and Variant is a very core type, for which adding something to it has to be warranted as really being used a lot. |
@Geometror I just want to say that this isn't wasted work, it's useful to me because it shows a minimal example of how to add a new type to Variant. But yeah, this really needed approval from reduz beforehand if your plan was to have it merged. |
@aaronfranke As I stated in my last comment, I wouldn't see this as wasted work too :) |
If |
I think it's a pretty bad argument to say where does godot draw the line if godot accepts a vector4 struct in Variant. It's pretty clear that the line would be drawn at vector4, as that's exactly where it's drawn in almost every other game engine. Unity and unreal, et al. As well, moving beyond the internal size of Variant's Odd there is aversion to this, when Plane, Quaternion, Rect2 and Color are already handled specialized Variant 4-float types when the more generalized form from my perspective would be utilized the most. Especially wrt mapping type to shader uniform type for example as mentioned above. Anecdotally we use Color right now for this, which is so bizzare because I have to keep reminding myself that all our 4-dimentional vectors are colors because there is no Vector4 type. |
For reference: https://docs.unrealengine.com/4.27/en-US/API/Runtime/Core/Math/FVector4/ I'm actually having a hard time finding any other engine that DOESN'T expose a native Vector4 type... Are there even any? I feel like this probably isn't a good one for Godot to be the 'standout'. |
The first time I found out that GD Script doesn't have a vec4 I was also very puzzled. |
It is great to see so much interest in this. There has also been some discussion on the RocketChat #core channel which I aim to summarize below. The largest consideration here is the cost of adding another type to If we are going to add something to core, then we need to ensure that it is both necessary and the cost is minimized. As time goes on and the usage of Godot increases. It becomes very hard to show that something is "necessary" as other developers will just say "not having it hasn't stopped anyone yet". All that is to say that the threshold to merge this is very high. Much higher than for an equivalent change to On the other hand, between this PR and the proposal, we haven't had a chance to fully develop the areas which absolutely need the introduction of a Vector4 class. @aaronfranke has helpfully started us off with the proposal, which lists some use-cases godotengine/godot-proposals#629. and @Geometror has refined that list in this comment (reproduced below):
In the conversation, significant doubt has been shed on the necessity of these use-cases. I will go through them one-by-one here. Also, if anyone else has other use-cases, please post them as well. Working with Shaders At the same time, this is more of a cosmetic thing, we could also provide an overload for setting vec4s, something like It would definitely be more convenient to just use a Vector4 from GDscript when working with vec4s in shaders. Representing Axis-angle 3D rotation Representing 3D vectors in Direction-Magnitude format 4D noise offsets For general linear algebra calculations Simply giving the freedom to store 4 values All the other engines expose it Conclusion |
For bone indicies and bone weights in Godot Engine 4 they can be 4 or 8 floats. In other engines they may be 32 or greater per vertex. I prefer the current structure of using float arrays. Edited: wrong attribute. |
@@ -480,13 +480,20 @@ _FORCE_INLINE_ static void _fill_std140_variant_ubo_value(ShaderLanguage::DataTy | |||
gui[1] = v.y; | |||
gui[2] = v.z; | |||
gui[3] = v.w; | |||
} else { | |||
} else if (value.get_type() == Variant::PLANE) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
On the topic of misc fixes, I think it's likely better that this check this should be here anyway, and the final else
would be an error.
scene/resources/visual_shader.cpp
Outdated
} break; | ||
case VisualShaderNode::PORT_TYPE_VECTOR_3D: { | ||
inputs[i] = "dot(" + src_var + ", vec3(0.333333, 0.333333, 0.333333))"; | ||
} break; | ||
case VisualShaderNode::PORT_TYPE_VECTOR_4D: { | ||
inputs[i] = "dot(" + src_var + ", vec3(0.25, 0.25, 0.25, 0.25))"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
inputs[i] = "dot(" + src_var + ", vec3(0.25, 0.25, 0.25, 0.25))"; | |
inputs[i] = "dot(" + src_var + ", vec4(0.25, 0.25, 0.25, 0.25))"; |
scene/resources/visual_shader.cpp
Outdated
} break; | ||
case VisualShaderNode::PORT_TYPE_VECTOR_3D: { | ||
inputs[i] = "dot(float(" + src_var + "), vec3(0.333333, 0.333333, 0.333333))"; | ||
} break; | ||
case VisualShaderNode::PORT_TYPE_VECTOR_4D: { | ||
inputs[i] = "dot(float(" + src_var + "), vec3(0.25, 0.25, 0.25, 0.25))"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
inputs[i] = "dot(float(" + src_var + "), vec3(0.25, 0.25, 0.25, 0.25))"; | |
inputs[i] = "dot(float(" + src_var + "), vec4(0.25, 0.25, 0.25, 0.25))"; |
The existing 4-float types in godot's Variant A vec4 is a common structure in game development as it's generally understood as no-restriction. I guess my specific use-case is that it's very un-ergonomic to work with packed I believe the engine should strive for consistency and intuitiveness. Shading language |
core/math/vector4.h
Outdated
// Multiplication operators required to workaround issues with LLVM using implicit conversion | ||
// to Vector3i instead for integers where it should not. | ||
|
||
_FORCE_INLINE_ Vector4 operator*(const float p_scalar, const Vector4 &p_vec) { | ||
return p_vec * p_scalar; | ||
} | ||
|
||
_FORCE_INLINE_ Vector4 operator*(const double p_scalar, const Vector4 &p_vec) { | ||
return p_vec * p_scalar; | ||
} | ||
|
||
_FORCE_INLINE_ Vector4 operator*(const int32_t p_scalar, const Vector4 &p_vec) { | ||
return p_vec * p_scalar; | ||
} | ||
|
||
_FORCE_INLINE_ Vector4 operator*(const int64_t p_scalar, const Vector4 &p_vec) { | ||
return p_vec * p_scalar; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since there's no Vector4i
, these probably aren't necessary.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I tried to remove it, but it wouldn't compile. I have a feeling that this comment might be outdated since it seems like these operators are needed regardless of having an integer variant of Vector4.
My use cases: shaders and noise AND this: godotengine/godot-proposals#4311 Quite possible vector4 would make it even easier to do than the current hacking around with planes and transforms and whatnots |
ddf50af
to
fd32876
Compare
core/variant/variant_setget.cpp
Outdated
// return step > 0; | ||
// } | ||
// return step < 0; | ||
// } break; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What's the reason behind adding this commented-out code?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, this was by mistake. I initially planned to implement the iter_* functions for Vector4, but I was not sure how to do it correctly, so I wanted to wait whether this PR would be approved first.
Since Godot's shading language defines |
Can I pile on this too? 🙃 There is a clear evidence in Godot's own API that we need a Vector4. Here's a method of
|
I have looked through few other forums (e.g., Unity) and it seems like the amount of people using Vector4 for 4D math is actually not that low. It can be difficult to compile a list of concrete use cases since often those who use it in their code are working on closed-source games/projects. Besides that, we have the aspect of consistency (again, using a Quaternion, Rect, Plane or Color instead of a 4D vector in places they don't make sense feels hacky, especially when all of these types are used promiscuously) I can understand that adding new types to the core must be a deliberate decision, but I genuinely believe that in this case the whole codebase would benefit from it. Given the amount of upvotes the original proposal and this PR got, I think we should reconsider adding a general Vector4 type to core. I agree with @jordo, there has to be a reason why nearly all engines have a general 4D vector type. I gladly rebase/adjust my PR if a final decision is made. Maybe this can be discussed in the next PR meeting? If we really don't want an additional variant type there was also this[https://github.com/godotengine/godot-proposals/issues/629#issuecomment-1092619812] suggestion to replace Plane with Vector4 which is - although not optimal - still better than the other way around, using Plane as a 4D vector, in my opinion. |
Here's the result of the discussion on PR review:
|
Superseded by #63219. |
This PR adds Vector4 as a new variant type and streamlines the usage of 4D-Vectors across the engine. There are a lot of possible use cases for a general 4-tuple where Plane, Color or Quaternion feel hacky because they all have specific constraints and should be used in a specific context. See godotengine/godot-proposals#629 (comment) for a good summary on this problem. Also, new users may find themselves confused, not knowing which type can be used for a general vector with 4 components since many other game engines have a general Vector4 type. Sure, this adds a lot of (boilerplate) code, but having a Vector4 makes things much cleaner in my opinion.
Closes godotengine/godot-proposals#629
Closes godotengine/godot-proposals#3761
Implements godotengine/godot-proposals#258 partly
Changes in detail:
This PR could be split up if desired (add Vector4 variant type | (visual) shader integration).
Since this introduces a new variant type which is deeply integrated into the engine, a thorough review and extensive testing may be required.
Optional/still considering:
Feedback is, as always, much appreciated :)