test_move() has misleading name and returns the opposite of the expected result, implement expected behavior

[CarpenterBlue]

Describe the project you are working on

a JRPG

Describe the problem or limitation you are having in your project

I wasn't sure if I should report this as a bug or propose as a feature but here I go.
When I call test_move() I expect to return TRUE when there is no collision because I am testing for possibility of movement, not collision.

Currently I have to do:
if NOT test_move():

Which when read gives off the incorrect idea.

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

Fix test_move() by inverting the behavior. Testing the possibility of movement will then return expected result.
OR
deprecate test_move() and implement test_collision() / test_move_collision() or any other name that better communicates what the function is actually doing. No compatibility breakage.

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

Either we duplicate the functionality under new function with better name or we fix the returned value of the existing function.

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

it can be worked around with 1-3 characters:
! or NOT

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

This is a change to GDScript.

[AThousandShips]

As this is very clearly documented, and changing it would be a completely unacceptable compatibility breakage, this isn't something that can happen soon

I don't consider it misleading, the name implies nothing about that it would return true or false for collision

[CarpenterBlue]

That's why I proposed the alternative solution.
I don't think we should dismiss it just because it is "well documented", it still makes code confusing when read.

The relevant part:

deprecate test_move() and implement test_collision() / test_move_collision() or any other name that better communicates what the function is actually doing. No compatibility breakage.

[AThousandShips]

That's a lot of work and clutter to correct some people being confused, especially when that confusion just goes away if you read the documentation, I don't think it's a good reason to deprecate and change a method that's been around for a long time, this was the same in 3.x, with test_motion

I don't think we should dismiss it just because it is "well documented", it still makes code confusing when read.

I don't think it's confusing, and I've never encountered anyone else having this issue, so I don't see any reason to believe it's a common cause of confusion, can you link to others having this issue? This isn't new

Edit: there's not a single error report or proposal about this confusion, so your confusion does not seem common

[yosoyfreeman]

Is common practice for this kind of tester functions to return something if the movement was not achieved. Furthermore, inverting the behaviour would be extremely confusing. If something was found on the path, then you have information about what it was, if you found nothing, you don't have it and don't need it.

It would be really confusing to have something returning null or false when is indeed providing you with information.

[AThousandShips]

Exactly

Further this has been this way always in Godot, or at least since 2.1

This matches Unity's "SweepTest"

[CarpenterBlue]

Is common practice for this kind of tester functions to return something if the movement was not achieved. Furthermore, inverting the behaviour would be extremely confusing. If something was found on the path, then you have information about what it was, if you found nothing, you don't have it and don't need it.

It would be really confusing to have something returning null or false when is indeed providing you with information.

Sorry, I edited the proposal since people keep focusing on the wrong solution. It should be now more clear.

Exactly

Further this has been this way always in Godot, or at least since 2.1

This matches Unity's "SweepTest"

But SweepTest doesn't have misleading name.
Also, Godot is not Unity.

[CarpenterBlue]

I think "Let's not fix a problem because it's been here for ages and we documented that this is a problem" is not a good reason to keep the problem in.

(By deprecation, I do not mean removing the test_move() function. There should be function with correct name though.

"Not enough people complained about this, to fix it" is also irrelevant, I am well aware that this is a papercut, test move is used like once or twice in my projects and then I promptly forget about it and cut myself on it every year or so when I need to use it. I myself have been ignoring it for actual years and just now realized it's been very annoying.

Furthermore, I do not think that single new function with different name is lot of clutter. Pretty sure the deprecated flag for documentation was introduced for this exact reason.

[yosoyfreeman]

Duplicating functionality for the sake of doing it is even a worse idea. Again, it makes no sense at all to return true if nothing was found and then return false + the information. This is standard practice. Is this way in any place you look. Any collision resolution paper also use this dynamic. There is no problem to solve, but this proposal would create a huge one.

Again, if you found something there is information to provide, if not, there is nothing. It does not even make practical sense.

[AThousandShips]

By deprecation, I do not mean removing the test_move() function.

I know... You don't need to educate me on this...

I think "Let's not fix a problem because it's been here for ages and we documented that this is a problem" is not a good reason to keep the problem in.

There should be function with correct name though.

We don't agree that it's a problem, or that it's poorly names, you're declaring it is, and rejecting arguments against it

"Not enough people complained about this, to fix it" is also irrelevant

Making everyone adjust their code because a few people are confused isn't fair or reasonable... And no one, not a single person, has raised this as an issue in the last ten years, unless you can find some

Pretty sure the deprecated flag for documentation was introduced for this exact reason

It was introduced for necessary deprecation where something needs to change

Sorry, I edited the proposal since people keep focusing on the wrong solution.

We're focusing on what's literally the title of this proposal and what you are suggesting, namely adding new, opposite, code, and it's still unnecessary

[AThousandShips]

What I'm saying is this, so we don't get confused about what I'm actually saying here:

• This doesn't seem to be confusing for anyone else, either that or people adjust their incorrect assumptions easily and move on
• This is a well established function with 8 years of history at least (I have no clue how this looked back in 2.0 or 1.x), so changing it would be confusing because it'd break a long tradition of how it works
• Adding duplicate methods with different behavior, even with deprecating the old behavior, adds clutter and bloat to the engine, and adds confusion, without deprecating it (which I don't think is valid for the above reasons) it is even worse
• This is well established in other engines and in the field, so Godot isn't unusual in this (hence referencing Unity, which was seemingly ridiculous)

In the end the solution is often to just adjust to the way the engine works if it's not a common issue, as annoying as it might be, especially instead of making things confusing and worse for everyone else

[CarpenterBlue]

Hm, ok, I am closing this proposal and I will make another attempt in the future with better explanation.
But I talked about this problem with people in Godot's discord and there are peope with opinion that the name is bad and someone even suggested I got dogpiled.

[AThousandShips]

Your explanation is sufficient, we just don't agree with your reasoning, not a lack of details or explanation, people can disagree with you, it doesn't require them not not understand

Honestly the implication that we just don't understand you is insulting and unproductive

And no one dogpiled you

Edit: Also, if you do chose to revisit this, you should reopen this issue, not open a new one