Skip to content
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 + and - operators for math primitives to Vector2/3/4(i) #5476

Open
AndreSacilotto opened this issue Sep 23, 2022 · 12 comments
Open

Add + and - operators for math primitives to Vector2/3/4(i) #5476

AndreSacilotto opened this issue Sep 23, 2022 · 12 comments

Comments

@AndreSacilotto
Copy link

AndreSacilotto commented Sep 23, 2022

Describe the project you are working on

"Mesh" generation project, using shaders

Describe the problem or limitation you are having in your project

I will use C# for the exemples, but this proposal it's not for C# only.

Not really a limitation, but I think is pretty annoying and memory inefficient to do vertex + new Vector2(2f, 2f) when could simply be vertex + 2f.

In Unity and Unreal this is allowed.

Describe the feature / enhancement and how it helps to overcome the problem or limitation

Add operators overloads to + and - to the classes on the title.

I know it's not a totally correct mathematical definition of a vector, but it's pretty accurate if we think that vector is just a point in space, and that's what godot vectors are most often used for. And it would be align with what * and / are already used for, thought they are mathematical correct.

So the pros are:

  1. In shaders you can do it, adding this would result in a more consistente workflow between the two;
  2. You would create one/two/tree less float/int variable(s), so less memory wasted;
  3. Make easier to convert code from other engines that already have it;
  4. Easier to read equations;

And the cons:

  1. Maybe some math purists/lovers would be angry/upset?
  2. Human Logic vs Machine Logic (read Mickeon comment)

Describe how your proposal will work, with code, pseudo-code, mock-ups, and/or diagrams

Add overload operators c# and c++ have this feature.
I will try do a PR for the c# side later - done #66286

If this enhancement will not be used often, can it be worked around with a few lines of script?

For me will be used very very often. But there is workarounds you can define a function to do that or in C# you can make an extension method.
But using the workround will not result in a cleaner code/equation.

Is there a reason why this should be core and not an add-on in the asset library?

Need to modify internal engine classes, so not happening.

@Mickeon

This comment was marked as outdated.

@Mickeon
Copy link

Mickeon commented Sep 23, 2022

Maybe some math purists/lovers would be angry/upset?

It do not believe it's because of this. The reason is very similar as to why no Vector* constructor accepting a single parameter is provided in Godot, unlike similar languages (e.g. (Vector3(7.5)).
It can be especially easy to mix-up and confuse, for readers and beginners alike. You personally, along with many, may not believe as such because you may have gotten accustomed to it, and that is pretty nice. However, Godot prioritises being clear and explicit whenever reasonable.

For example, suppose I, beginner, want to nudge my character 2 pixels to the right and 2 pixels down. I write this:

var offset = Vector2(2, 2)
# [...]
position = position + horizontal_move.x

And then it doesn't compile. Why? Because I mixed up Vector2 and numbers together. Oops.
If it were to work as suggested in this proposal, it would compile completely fine, then the moment I decide to nudge my character 5 pixels down instead, it would behave strangely, I'd lose a few screws over it, and then realise my miniscule yet troublesome mistake.

So, ideally, it would be best if the operation were to error out instead of misbehave.

Although, an argument in favour can be made for the Shader language supporting it.

If this enhancement will not be used often, can it be worked around with a few lines of script?

You can also do vertex + Vector2.ONE * 2, at least in GDScript. Not sure how constants are defined in C#, but the syntax should be similar. I don't think you should worry about performance, the compiler should be smarter than that.

@Calinou
Copy link
Member

Calinou commented Sep 23, 2022

I also don't think allowing Vector2 operators to work with float/int is a good idea, as it's too error-prone. It's better require typing some more characters and avoid difficult-to-debug errors this way.

@AndreSacilotto
Copy link
Author

AndreSacilotto commented Sep 23, 2022

@Calinou

I also don't think allowing Vector2 operators to work with float/int is a good idea, as it's too error-prone. It's better require typing some more characters and avoid difficult-to-debug errors this way.

Also dont think is a good ideia to have implicity convertions of int to float or the reverse, so would be only float for Vector2/3/4 and int for Vector2i/3i/4i.
This is the reason I make the PR, was to show that. But I'm not sure I interpreted your comment correctly.

difficult-to-debug errors

So this ability can be remove from shaders has they are far harder for debugging...
And why it should make hard for debugging? Multiplications and divisions have the same difficult of this.

I googled a bit on #shader discord to see if anyone had this problem in the past and found nothing.

@Mickeon

About Vector constructor with one argument

I'm not gonna lie I want that, but I don't see why to add it:

  1. Reasonable
  2. The compiler could simply inline the extension function with no problems.
  3. Reasonable
  4. Arguable

Your workround

The workround you present is valid (even in c#), but I dont believe the compiler can optimize more than this:
vertex + Vector2(2f, 2f), because not using vec2 could change the behavior of the operation, so you are still wasting memory and in vector operation that are likely to execute every frame speed matters.

About Beginners

Because I mixed up Vector2 and numbers together

Well multiplication and division exist. But I get what you mean, what was truly been misunderstand in that code was machine logic vs human logic.

About the Error

I think it's a good thing that no error is given, it makes you understand what the code does instead of believing what it does.

And is an error that would rarely happens, because only 2 understandings are possible:
A. The X is add both Vector.X and Vector.Y;
B. The X is add to Vector.X only; But this one has a logical flaw, because X could be added to Vector.Y instead

X and Y are human constructions/logic, a good programmer should know that.

And any good programmer that tries B and don't work will assume A. This is exactly what happened when I discussed it on the discord server.

Edits: Typos and better rationally

@AndreSacilotto
Copy link
Author

AndreSacilotto commented Sep 23, 2022

Previous was my formal answer, now this is from the heart.

position = vec2(4, 5)
motion = vec2(2, 3)
add = position + motion .x
mult = position * motion .x

What is the difference between line 1 and 2? Only the operator.

Why would you assume that in the add operation the result would be vec2(6, 5) when in the result of the mult is vec2(8, 10) this is inconsistent. The logical result of the add operation is vec2(6, 7).

@Mickeon From your logic the result of the mult should be vec2(8, 5) or an error, so why it's not?

Why multiplication is allowed and when addiction is not? Only because a book or your teacher said, I dont think is a good reason.

This is why I call some people math purists, generally you look only for the math side, try to take a look from logic side and you will see what I see.

Side note:
I dont think been a math purist is a bad thing 
for me it's something very similar to a religion, so I try respect it. 
But godot is a game engine and not a math library, so pls take this in consideration.

The current way is more on side of people that have learn math from books, and not the one which have learned from code pratice like myself.

Edit: Trying to be less agressive, sorry by that.

@Mickeon
Copy link

Mickeon commented Sep 23, 2022

Remember that Code of Conduct is applied here, too.

I am not particularly educated in math. I have not been educated by anyone about Vector math. From what I'm aware of, and from what I've seen across the codebase, multiplying Vectors by a given float is a pretty common operation.

I don't know if I can say the same about this Proposal. Maybe in the future the need for this operation may increase and really warrant its inclusion, but in the years Godot has been public, the operation has yet to be implemented, as simple as it is, which has me wondering about its utility. It doesn't help that no concrete example on its usage has been given as of the time I'm writing this.

@AndreSacilotto

This comment was marked as off-topic.

@AndreSacilotto
Copy link
Author

AndreSacilotto commented Sep 23, 2022

I was trying to find a good exemple and notice that float / Vector2 also dont exist.
So if this would be added this operation would also make sense to be added or not?

Exemple

But anyway here is a exemple, one important thing ot notice this proposal do not bring any utility to the table everyting can already be done in godot current state. It is all about cleaner code.

private Vector2 Zoom;

public void ZoomDir(float dir)
{
    const float K = 3; //K is a heuristic

    //CURRENT CODE
    Zoom += new Vector2(K / (K + Zoom.x), K / (K + Zoom.y)) * dir;

    //NEW CODE
    Zoom += (K / (Zoom + K)) * dir;

    //NEW CODE no `float / Vector`
    Zoom += new Vector2(K, K) / (K + Zoom) * dir;

    Zoom = Zoom.Clamp(MIN_ZOOM, MAX_ZOOM);
}

This a zoom camera2D code that uses diminishes returns. If dir > 0 it's ZoomIn if dir < 0 it's ZoomOut.

You can see how the new code would be much more compact.

@MewPurPur
Copy link

MewPurPur commented Sep 26, 2022

It just looks confusing IMO, it's not as intuitive as you make it out to be. Multiplication with a float X can also be interpreted as adjusting the vector's length: this cannot.

Boasting the memory freed up from such an operator seems exaggerated - and even if we wanted the boost, there must be other ways to achieve it in Godot

@Mickeon
Copy link

Mickeon commented Sep 26, 2022

How much of a boost would it even be? Without actual comparisons it's difficult to say.

@AndreSacilotto
Copy link
Author

AndreSacilotto commented Sep 27, 2022

Hard to say even with benchmarks would be difficult to say, values types are cheap and work different depending on the OS.

Let me propose other solution, instead of overload operators what could be done is an Add()* method be added. So we can have that memory performance if needed.

*A Subtraction method could also be added, but would be redundant. And I dont think Add is good name others suggestions would be Sum, Addiction or AddUp.

I still prefer my original proposal, but this would be good enought.

    Zoom += new Vector2(K, K) / Zoom.Add(K) * dir;

   //Still better than this
    Zoom += new Vector2(K, K) / (Zoom + new Vector2(K, K)) * dir;

@AndreSacilotto
Copy link
Author

AndreSacilotto commented Sep 27, 2022

I decide to search in other engines Vector2 "source code"`or docs, to see if they would already have this feature so here is my results:

Have Operators: Unity, Unreal, FlexEngine,

Have Only Funcs:

Don't Have Any: Cocos2d, CryEngine, Godot, Phaser, lumberyard, Monogame/XNA, Stride, Raylib

Don't Even Have a Vector Class: Constuct 3, GM2

This is an funny and interesting data, so comercial engines have it while open source don't.

Probably because of controversial topic, so it will never have approval of a great majority so it never merged.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

5 participants