Add helper methods to construct a vector from a scalar.

[WhalesState]

This implements new helper methods for all vector inside variant.h since it already includes all the vector headers.

usage:

// Before
follow.scale(Vector2(get_follow_viewport_scale(), get_follow_viewport_scale()));
// After
follow.scale(vec2_from_scalar(get_follow_viewport_scale()));

[Ivorforce]

Note that we had several earlier PRs attempting to do (loosely) the same, e.g. #104404 and #99783.

I believe what stalled these PRs is partially that we're not fully convinced this is an improvement. Vector3(1, 1, 1) is just a lot less ambiguous than Vector3(1) — which could also mean Vector3(1, 0, 0).

Personally I'm still somewhat in favour, because i'm used to tensor broadcasting, which makes "single-argument construction" possible by default — but it's not a clear win.

[Ivorforce]

Making this a template argument is more complicated than it needs to be.
What Vector needs for x and y is real_t. It copies the real_t to x and y. So the argument should be real_t.

[WhalesState]

It conflicts with the copy constructor Vector2(Vector2 &p_v) that is generated by the compiler, we will have to redefine it for all types or the compiler will be confused when we use Variant. See how it was handled in #104404 to avoid this issue.

[Ivorforce]

Right... The appropriate fix would be to remove implicit conversions that cause the conflict. Though that's out of scope for this PR.

[AThousandShips]

I still personally think a static method for this is the best option, it involves the least code hacking to make it work in various cases, it is the most transparent and least confusing what it actually means, it is also far easier to notice in reading code fast, foo(Vector2(1), 2) is far harder to read at a glance IMO for example

[WhalesState]

I still personally think a static method for this is the best option, it involves the least code hacking to make it work in various cases, it is the most transparent and least confusing what it actually means, it is also far easier to notice in reading code fast, foo(Vector2(1), 2) is far harder to read at a glance IMO for example

I see your point, but for source code it’s actually shorter and faster to just use the constructor than to rely on a static method, even if the static one gets inlined. In some of our use cases, the new constructor avoids creating a temporary variable and skips redundant operations.

For scripting languages I agree with you though, it makes more sense not to expose the constructor directly, and instead provide a static wrapper for the single-value case.

I also ran tests with the inlined static method and didn’t observe any performance gain. From what I can tell, it still passes the arguments through to the vector constructor (x and y) and the constructor ends up recalculating them twice anyway.

[WhalesState]

Replaced the constructor with a static method splat, since Vector2::splat(variable) always makes it explicit that the input is a scalar, whereas Vector2(variable) requires additional type checking.

[clayjohn]

I've said this in other chats where this is being discussed, but all the proposals for constructing vectors from a single element stress that they want it to reduce typing or to match GLSL. This PR does neither. By way of example, users feel that Vector3(1, 1, 1) is too much to type, they want to type Vector3(1) instead. Unfortunately, Vector3.splat(1) is more typing and requires more keyboard travel. So it doesn't solve the problem that lead to the proposals in the first place.

It's unclear what problem this actually solves. There is no performance benefit. Whether it is more readable or not is arguable IMO it is less readable, both because it relies on users assuming that the single element will be copied n times, and because splat is not descriptive at all. I haven't seen that terminology in any other API. And it doesn't reduce typing/boilerplate either.

To be perfectly blunt, at this point this PR is just adding bloat to the core and making the codebase less readable for contributors.

[AThousandShips]

I'd argue that the main reason is less so amount of typing and more so ease of updating and safety of use, especially as this is for the codebase and not an exposed method, it avoids cases where the argument needs to be stored in a temporary or similar. Also while it's true that for trivial cases Vector3.splat(1) is shorter for a lot of real world cases it is shorter than the alternative, such as:

• Vector3(center, center, center) -> Vector3::splat(center)
• Vector3(longest_axis, longest_axis, longest_axis) -> Vector3::splat(longest_axis)

And especially for cases with temporaries, for example:

• Vector3::splat(Math::sqrt(double(new_simplex->circum_r2)) from two lines with a temporary

In fact from a glance most of the cases in this PR saves at least a few characters, and several saves dozens

But it is a matter of taste and preference, but I'd say that the main goal is more to reduce duplication, the specific readability would depend on the case of course

---

To be clear I'm personally mixed on this particular change, but I do think it has specific merits, but might be more reasonable to apply it in targeted ways where it replaces messy constructions, but would be good to assess broader opinions on the readability

[WhalesState]

The goal of this PR is to improve clarity and efficiency in the source code by introducing a scalar constructor (or static method) for vectors.

Both approaches (Vector2::splat(scalar) or Vector2(scalar)) help us avoid creating temporary variables or dummy scaling operations. This results in shorter and more readable code in those cases.

DisplayServer::get_singleton()->window_set_window_buttons_offset(Vector2i(title_bar->get_global_position().y + title_bar->get_size().y / 2, title_bar->get_global_position().y + title_bar->get_size().y / 2), DisplayServer::MAIN_WINDOW_ID);
DisplayServer::get_singleton()->window_set_window_buttons_offset(Vector2i::splat(title_bar->get_global_position().y + title_bar->get_size().y / 2), DisplayServer::MAIN_WINDOW_ID);

main_button->draw_circle(Vector2(button_radius * 2, button_radius * 2), button_radius, color);
main_button->draw_circle(Vector2::splat(button_radius * 2), button_radius, color);

const Size2 borders = Size2(4, 4) * EDSCALE;
const Size2 borders = Size2::splat(4 * EDSCALE);

Vector3 scale = Vector3(root_scale, root_scale, root_scale);
Vector3 scale = Vector3::splat(root_scale);

box->set_size(Vector3(2, 2, 2) * p_applied_root_scale);
box->set_size(Vector3::splat(2 * p_applied_root_scale));

src_bitmap->create((aabb.size + Vector2(divide_by - 1, divide_by - 1)) / divide_by);
src_bitmap->create((aabb.size + Vector2::splat(divide_by - 1)) / divide_by);

Vector2 zoom_factor = (debug_uv->get_size() - Vector2(1, 1) * 50 * EDSCALE) / tex->get_size();
Vector2 zoom_factor = (debug_uv->get_size() - Vector2::splat(50 * EDSCALE)) / tex->get_size();

xform.set_scale(Vector2(editor_zoom_widget->get_zoom(), editor_zoom_widget->get_zoom()));
xform.set_scale(Vector2::splat(editor_zoom_widget->get_zoom()));

mesh->set_custom_aabb(AABB(Vector3(-selectable_icon_size, -selectable_icon_size, -selectable_icon_size) * 100.0f, Vector3(selectable_icon_size, selectable_icon_size, selectable_icon_size) * 200.0f));
mesh->set_custom_aabb(AABB(Vector3::splat(-selectable_icon_size * 100.0f), Vector3::splat(selectable_icon_size * 200.0f)));

// Before
int thumbnail_size = EDITOR_GET("filesystem/file_dialog/thumbnail_size");
thumbnail_size *= EDSCALE;
Vector2 thumbnail_size2 = Vector2(thumbnail_size, thumbnail_size);
// After
Vector2 thumbnail_size2 = Vector2::splat(int((int)EDITOR_GET("filesystem/file_dialog/thumbnail_size") * EDSCALE));

[Bromeon]

To be perfectly blunt, at this point this PR is just adding bloat to the core and making the codebase less readable for contributors.

@clayjohn In my opinion, the diff demonstrates the improvements in readability quite well -- it not only saves typing but more importantly, repetition:

In several places, extra variable declarations are used at the moment, here even with quite a bit of distance to its usage:

There are even cases where current code works around the lack of such a constructor by using multiplication:

---

and because splat is not descriptive at all. I haven't seen that terminology in any other API.

Agree, maybe it's not as common as I thought, there are many other possibilities like fill, repeat... 🙂

[clayjohn]

Just to highlight two things:

1. Readability is naturally a subjective thing. I disagree that this is more readable, but I won't die on that hill.
2. There have been 4 proposals for this feature and this PR addresses none of them. That isn't strictly an issue if this isn't being exposed to end-users, but it does highlight that we may be missing the point.

On the point of readability, grabbing a. random example from above:

box->set_size(Vector3(2, 2, 2) * p_applied_root_scale);
box->set_size(Vector3::splat(2 * p_applied_root_scale));

To someone dropping into the codebase the first is very clear, the size of the box is 2 times the applied root scale in each axis. The second one could mean anything. What axis does splat apply to? Does splat apply any other operation? Perhaps it applies a scaling factor or size limits or something. In order to understand the code, the reader needs to navigate to where splat is defined whereas in the former case, the code speaks for itself.

IMO this is a place where trying to come up with a clever solution to reduce the amount of code makes the codebase harder to navigate and maintain.

The best solution for all the above cases IMO is to have a temporary variable i.e.:

main_button->draw_circle(Vector2(button_radius * 2, button_radius * 2), button_radius, color); // godot
main_button->draw_circle(Vector2::splat(button_radius * 2), button_radius, color); // this PR

//improved
Vector2 circle_radius = button_radius * 2;
main_button->draw_circle(Vector2::(circle_radius, circle_radius), button_radius, color);

[WhalesState]

I agree that splat may not be familiar to many developers, especially newer contributors. To improve readability, I’d suggest alternatives such as Vector3::filled(x), Vector3::all(x), or Vector3::scaled(x). These names make it clearer that the same scalar value is being applied across all components.

Edit: Vector3::filled(x) is better for readability, and Vector3::all(x) is just shorter but still not clear, so I'd prefer Vector3::filled(x) over Vector3::splat(x)

[WhalesState]

Regarding the proposals: most of the debate was about exposing this as a constructor in GDScript and GDExtension, which sparked a lot of disagreement. To avoid reopening that discussion, this PR is limited to the C++ side. If they'd agree on using a static method instead, we can expose it to scripting languages and finally resolve the open proposals and PRs.

[Bromeon]

The best solution for all the above cases IMO is to have a temporary variable i.e.:

This is a valid approach too, one thing to note though: this is harder to standardize and relies on out-of-band docs/guidelines. You see it from the current code, various people have different ideas on a best approach:

• Vector3(long_expr, long_expr, long_expr)
• float a = long_expr; Vector3(a, a, a)
• long_expr * Vector3(1, 1, 1)

A dedicated method Vector3::fill(a) does not have this issue: people can directly point to it in reviews without needing to cite a "convention". Once it's known, contributors reading the code recognize the pattern immediately. But I agree there's an initial phase where it may be slightly confusing, although with fill/repeat naming the risk is more limited.

[WhalesState]

In Godot’s codebase, methods that return a modified copy are consistently written in the past tense:

Transform2D::rotated(x); // Returns a rotated copy.

Transform2D::scaled(x); // Returns a scaled copy.

So following that pattern, Vector2::filled(x) makes more sense than Vector2::fill(x), because it signals Return a Vector2 filled with x rather than Fill this Vector2 with x for a static method.

Edit: if we expose it later for scripting languages users may mistakenly use it on a vector directly.

node.scale.fill(x * y)

# Instead of
node.scale = Vector2.filled(x * y)

[clayjohn]

A dedicated method Vector3::fill(a) does not have this issue: people can directly point to it in reviews without needing to cite a "convention". Once it's known, contributors reading the code recognize the pattern immediately. But I agree there's an initial phase where it may be slightly confusing, although with fill/repeat naming the risk is more limited.

I think you are solving a problem that doesn't exist. I care about people being able to read the code. I don't care about enforcing arbitrary style preferences on people. I think it would be a mistake to create Vector3::fill() and then impose that on all contributors in PR reviews. That just makes the PR review process more bureaucratic and hostile for contributors for no benefit in the codebase.

[AThousandShips]

Indeed it would be a case-by-case use for reducing code duplication where it would be appropriate if we go with it

[Bromeon]

I think it would be a mistake to create Vector3::fill() and then impose that on all contributors in PR reviews. That just makes the PR review process more bureaucratic and hostile for contributors for no benefit in the codebase.

I definitely don't mean to be dogmatic about Vector3::filled (or whatever its name), it can be used where it helps and left out where it doesn't. It's not different from other utility APIs in that regard. If you see it as putting unneccessary indirection/complexity into the code, I also understand that.

The best solution for all the above cases IMO is to have a temporary variable

At least we seem to agree that there's some room for improvement, even if in different directions 😉

[WhalesState]

Added a new helper methods inside varitant.h

// Static methods will need a long name to describe it.
Vector3::all(x);
Vector3::fill(x);
Vector3::splat(x);
Vector3::filled(x);
Vector3::from_scalar(x);

// A method that we can call directly will be shorter and still describes the method usage.
Vector3::from_scalar(x);
vec3_from_scalar(x);

Edit: We also can add macros to help calling these methods.

// Vector from scalar helpers.
#define VEC2S(scalar) vec2_from_scalar(scalar)
#define VEC3S(scalar) vec3_from_scalar(scalar)
#define VEC4S(scalar) vec4_from_scalar(scalar)
#define VEC2IS(scalar) vec2i_from_scalar(scalar)
#define VEC3IS(scalar) vec3i_from_scalar(scalar)
#define VEC4IS(scalar) vec4i_from_scalar(scalar)