Made Rect/FRect move/move_ip FASTCALL

[itzpr3d4t0r]

I have ported #1958 since the last addition of FRect. As I am still adapting, this may not be the ideal implementation. Please let me know if you know better methods. I have also added more benchmarks for FRect to account for these changes in the code.

Results (columns show old/new):
Move

Case	Rect (s)	Uplift (%)	Frect (s)	Uplift (%)
1 arg tup inside tup	0.10717282 / 0.08810694	21.6	0.08717053 / 0.06963296	25.1
1 arg tup	0.10452923 / 0.0842131	24.1	0.08617208 / 0.06879297	25.2
1 arg lst	0.13279415 / 0.11934226	11.2	0.1210619 / 0.10032487	20.6
2 args	0.10887068 / 0.07456696	46.0	0.08739826 / 0.05937409	47.1

Move_ip

Case	Rect (s)	Uplift (%)	Frect (s)	Uplift (%)
1 arg tup inside tup	0.09123963 / 0.0729865	25.0	0.06778982 / 0.05828947	16.3
1 arg tup	0.09867253 / 0.06984699	41.2	0.06575731 / 0.05717076	15.0
1 arg lst	0.12765383 / 0.09541213	33.8	0.09919653 / 0.07748874	28.0
2 args	0.08948597 / 0.05262832	70.0	0.06839517 / 0.04137996	65.2

Not part of this PR but can be taken as a reference:

Case	Rect (s)	Frect (s)
ref move by attribs	0.17353	0.14476007

The code I used:

from pygame import Rect, FRect

from statistics import fmean
from timeit import repeat

r = Rect(0, 0, 33, 33)
fr = FRect(0, 0, 33, 33)
rep = 10

glob = globals()


def test(test_name: str, func: str) -> float:
    value = fmean(repeat(func, globals=glob, repeat=rep))
    print("-) " + test_name + f": {round(value, 8)}")


print("--- Rect Move ---")
test("1 arg tup inside tup", "r.move(((1, 1),))")
test("1 arg tup", "r.move((1, 1))")
test("1 arg lst", "r.move([1, 1])")
test("2 args", "r.move(1, 1)")
print("--- Rect Move_ip ---")
test("1 arg tup inside tup", "r.move_ip(((1, 1),))")
test("1 arg tup", "r.move_ip((1, 1))")
test("1 arg lst", "r.move_ip([1, 1])")
test("2 args", "r.move_ip(1, 1)")

print("--- Rect attribute move ---")
test("move(ip) by attribs", "r.x += 1; r.y += 1")

print("=====================================")

print("--- FRect Move ---")
test("1 arg tup inside tup", "fr.move(((1.0, 1.0),))")
test("1 arg tup", "fr.move((1.0, 1.0))")
test("1 arg lst", "fr.move([1.0, 1.0])")
test("2 args", "fr.move(1.0, 1.0)")

print("--- FRect Move_ip ---")
test("1 arg tup inside tup", "fr.move_ip(((1.0, 1.0),))")
test("1 arg tup", "fr.move_ip((1.0, 1.0))")
test("1 arg lst", "fr.move_ip([1.0, 1.0])")
test("2 args", "fr.move_ip(1.0, 1.0)")

print("--- FRect attribute move ---")
test("move(ip) by attribs", "fr.x += 1.0; fr.y += 1.0")

[yunline]

LGTM

[MyreMylar]

Noticed that the comments need updating to reflect the newly multi-type code.

[Starbuck5]

@itzpr3d4t0r I took the liberty of resolving Myre's comments about type names in comments.

I also changed two error messages:

• "Invalid position" -> "invalid argument" because it's not really a "position" when called by the move functions. So calling it argument makes it more generic so it applies better to both.
• "function takes at most 2 arguments" -> this is to better match a 'standard' result by PyArg_Parse. For example see this Surface creation: pygame.Surface((20,20), 500, 10, 10, 10, 10) -> TypeError: function takes at most 4 arguments (6 given)

I'd like to make sure these changes are good with you.

[itzpr3d4t0r]

I'd like to make sure these changes are good with you.

I'm fine with the changes. I guess we could improve error messages a bit in the two arguments case. Like right now it says "x must be a numeric value" if the first argument isn't ok and same thing but with y in the other case.
I suppose using "x" and "y" isn't 100% guaranteed to be fine in all cases so I propose we swap those messages with:

• "Invalid argument. The first argument must be a numeric value."
• "Invalid argument. The second argument must be a numeric value."

These or just the second part. I also think we could rename the RectExport_pgCoord_FromFastcallArgs function to like RectExport_pgTwoValues_FromFastcallArgs. Again makes it more general as not everything is a coordinate.

[Starbuck5]

@itzpr3d4t0r I agree, so I pushed the changes up.

Is "invalid argument" redundant?

If it's a bit weird I'm committing to your branch, I'm just doing it because you said you were pretty occupied with IRL stuff.

[itzpr3d4t0r]

I changed error messages to be more precise and informative. It would take a bit to make a direct comparison but generally every error message should now state what the error is, what it expected and what it got in terms of value/type.

Btw I'm planning to port this way of handling errors to #1842.

[Starbuck5]

Still looks good to me.

I'm a bit apprehensive of the extra code complexity to support the error messages, but you seem to have it well handled.