Clamp(vector) doesn't do what I expect it to

Godot Version

4.2.1

Question

Does clamping a vector mean something other than clamping the individual components or is the following a bug?

Scenario

I wrote the below function to translate from pixels to a ‘map’ and it contained the code:

clamp( map_coord - Vector2i.ONE
     , Vector2i.ZERO
     , Vector2i( MAP_X_DIM, MAP_Y_DIM )
     )

Note that map_coord is Vector2i
If map_coord is ( 0, n * MAP_Y_DIM ), then the code returns ( 0, n ).
If map_coord is ( n * MAP_X_DIM, 0 ), then the code returns ( n, -1 ).

That second result is surprising (to me).

I have re-written the code to this:

Vector2i( clampi( map_coord.x - 1, 0, MAP_X_DIM )
        , clampi( map_coord.y - 1, 0, MAP_Y_DIM )
        )

Which returns (0, n) and (n,0) as expected and is how I would imagine clamp(vector...) would work.

Can you provide the code.

const MAP_X_DIM        : int = 28
const MAP_Y_DIM        : int = 15
const MAP_PX_PER_UNIT  : int = 64

# This version clamps a vector
func _map_coord_from_px_CV( px_coord : Vector2 ) -> Vector2i :
	
	var map_coord : Vector2i = ( px_coord / MAP_PX_PER_UNIT ).round() 
	
	return clamp( map_coord - Vector2i.ONE, Vector2i.ZERO, Vector2i( MAP_X_DIM, MAP_Y_DIM ))

# This version clamps the components and make a vector
func _map_coord_from_px_VC( px_coord : Vector2 ) -> Vector2i :
	
	var map_coord : Vector2i = ( px_coord / MAP_PX_PER_UNIT ).round() 
	
	return Vector2i( clampi( map_coord.x - 1, 0, MAP_X_DIM ), clampi( map_coord.y - 1, 0, MAP_Y_DIM ))

# This is the reciprocal code used in the test cases in `_ready`, below
func _map_coord_to_px( map_coord : Vector2i ) -> Vector2 :
	
	var px_coord : Vector2 = ( map_coord + Vector2i.ONE ) * MAP_PX_PER_UNIT
	
	return px_coord

func _ready() :

	var test_vectors : Array[Vector2] = [ Vector2( 0, 1000 )
										, Vector2( 1665, 0 )
										, _map_coord_to_px( Vector2i.ZERO )
										, _map_coord_to_px( Vector2i.ONE )
										, _map_coord_to_px( Vector2i( 27, 14 ))
										, _map_coord_to_px( Vector2i( 28, 15 ))
										]
	
	for px_coord in test_vectors:
		var map_coord = _map_coord_from_px_CV( px_coord )
		print( "CV px: ", px_coord, "; map: ", map_coord )

	for px_coord in test_vectors:
		var map_coord = _map_coord_from_px_VC( px_coord )
		print( "VC px: ", px_coord, "; map: ", map_coord )

Attach that to a random node and run your project. I receive the following output:

CV px: (0, 1000); map: (0, 0)
CV px: (1665, 0); map: (25, -1)
CV px: (64, 64); map: (0, 0)
CV px: (128, 128); map: (1, 1)
CV px: (1792, 960); map: (27, 14)
CV px: (1856, 1024); map: (28, 15)
VC px: (0, 1000); map: (0, 15)
VC px: (1665, 0); map: (25, 0)
VC px: (64, 64); map: (0, 0)
VC px: (128, 128); map: (1, 1)
VC px: (1792, 960); map: (27, 14)
VC px: (1856, 1024); map: (28, 15)

Note the -1 on line 2

I just noticed line 1 is wrong, also…

Yep definitely broken. I can’t see what us the problem but for better safty you can use Vector2i.clamp() function. Btw why is subtracting Vector2i.ONE is necessary?

1 Like

As far as I understand clamp() compares the passed arguments with the > operator, which does this for Vectors:
Compares two Vector2 vectors by first checking if the X value of the left vector is greater than the X value of the right vector. If the X values are exactly equal, then it repeats this check with the Y values of the two vectors. This operator is useful for sorting vectors.
So no, it doesn’t clamp individual elements.

But using clamp on the vector2 itself like: my_vector2.clamp(min, max) does clamp the elements as you’d expect.

1 Like

Regarding the Vector2i.ONE, this is because the play area has a border that is approximately 64 pixels wide.

That’s still a bug, though, right?

IMHO, the library should either not support vectors or support them properly.

Well, clamp() is implemented as a function template, as it is right now, it can only handle vectors the same way it handles any other variable (which is figuring out which one is greater using >, as I’ve said):

template <typename T, typename T2, typename T3>
constexpr auto CLAMP(const T m_a, const T2 m_min, const T3 m_max) {
	return m_a < m_min ? m_min : (m_a > m_max ? m_max : m_a);
}

So it’s not really a bug, just a limitation of using a single template I guess. I had the same issue as you, so I agree it’s not ideal…

I can see two solutions to this, one in the compiler of gdscript and one in C++ itself. As I have no idea how gdscript is implemented, I don’t know which would work best.

In gdscript compiler, there could be code somewhere that says:

if m_a.has_method( "clamp" ):
    emit( "m_a.clamp( m_min, m_max" )
else:
    emit( "clamp( m_a, m_min, m_max" )

You can do something similar in C++ directly with template specialisation but it’s been a LONG TIME since I’ve had to read, let alone write, anything so gnarly.

1 Like

Well I just like to dig around in the code, but where / how you’d fix it is beyond my expertise :smile: You’d have to raise an issue on github I guess if you’d like it to change.

This topic was automatically closed 30 days after the last reply. New replies are no longer allowed.