search

godotengine / godot

Godot Engine – Multi-platform 2D and 3D game engine
https://godotengine.org
MIT License
91.38k stars 21.26k forks source link

Vector2i % implemented wrong for gdextensions only #99518

Open mayoff opened 3 days ago

mayoff commented 3 days ago

Tested versions

The code of interest was introduced in commit 221f5da85 on 2021-08-15.

I apologize for not including an MRP, but this is a bug in a method exported via the gdextension API and I do not believe the bug can be reproduced with GDScript, C#, or godot-cpp because none of them ever call the method in question. I am working on SwiftGodot (Swift language bindings for Godot).

System information

macOS 14.7.1 - Godot 4.3 or master (61598c5c88)

Issue description

In variant_op.h lives the class OperatorEvaluatorModNZ which implements the % operator, and at line 301 begins the specialization for the class for Vector2i % Vector2i:

https://github.com/godotengine/godot/blob/f952bfe9985ad8f507cc29b2c7601bbba18b8039/core/variant/variant_op.h#L301-L323

At line 312 it uses the overloaded C++ operator % operator on behalf of GDScript:

https://github.com/godotengine/godot/blob/f952bfe9985ad8f507cc29b2c7601bbba18b8039/core/variant/variant_op.h#L312

At line 317 it again uses the overloaded C++ % for gdextensions calling through the Variant API:

https://github.com/godotengine/godot/blob/f952bfe9985ad8f507cc29b2c7601bbba18b8039/core/variant/variant_op.h#L317

But at line 320, it incorrectly uses the overloaded C++ operator / for gdextensions calling through the pointer API:

https://github.com/godotengine/godot/blob/f952bfe9985ad8f507cc29b2c7601bbba18b8039/core/variant/variant_op.h#L320

Line 320 needs to be changed to also use operator %.

Steps to reproduce

Write new language bindings that use the engine to compute Vector2i % Vector2i.

In my case, I am writing test cases that compare what the engine returns and what my native Swift code computes, to verify that my Swift code matches the engine behavior. Imagine my surprise when the engine gave the wrong answer!

Minimal reproduction project (MRP)

I don't think it's feasible to include an MRP. Reproducing the problem requires creating new language bindings, because none of GDScript, C#, and godot-cpp rely on this exported engine method.

akien-mga commented 3 days ago

Seems like a copy-paste mistake, feel free to make a PR if you confirm that this fixes the issue.