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.

Sign up for GitHub

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

Jump to bottom

GDScript: Fix object iterator opcodes #89639

Merged
merged 1 commit into from
Apr 26, 2024
Merged

GDScript: Fix object iterator opcodes #89639

merged 1 commit into from
Apr 26, 2024

Conversation

dalexeev
Copy link
Member

@dalexeev dalexeev commented Mar 18, 2024 •
edited
Loading

See core methods for reference:

godot/core/variant/variant_setget.cpp

Lines 1304 to 1924 in 7abe0c6

bool Variant::iter_init(Variant &r_iter, bool &valid) const {
valid = true;
switch (type) {
case INT: {
r_iter = 0;
return _data._int > 0;
} break;
case FLOAT: {
r_iter = 0.0;
return _data._float > 0.0;
} break;
case VECTOR2: {
double from = reinterpret_cast<const Vector2 *>(_data._mem)->x;
double to = reinterpret_cast<const Vector2 *>(_data._mem)->y;
r_iter = from;
return from < to;
} break;
case VECTOR2I: {
int64_t from = reinterpret_cast<const Vector2i *>(_data._mem)->x;
int64_t to = reinterpret_cast<const Vector2i *>(_data._mem)->y;
r_iter = from;
return from < to;
} break;
case VECTOR3: {
double from = reinterpret_cast<const Vector3 *>(_data._mem)->x;
double to = reinterpret_cast<const Vector3 *>(_data._mem)->y;
double step = reinterpret_cast<const Vector3 *>(_data._mem)->z;
r_iter = from;
if (from == to) {
return false;
} else if (from < to) {
return step > 0;
}
return step < 0;
} break;
case VECTOR3I: {
int64_t from = reinterpret_cast<const Vector3i *>(_data._mem)->x;
int64_t to = reinterpret_cast<const Vector3i *>(_data._mem)->y;
int64_t step = reinterpret_cast<const Vector3i *>(_data._mem)->z;
r_iter = from;
if (from == to) {
return false;
} else if (from < to) {
return step > 0;
}
return step < 0;
} break;
case OBJECT: {
if (!_get_obj().obj) {
valid = false;
return false;
}
#ifdef DEBUG_ENABLED
if (EngineDebugger::is_active() && !_get_obj().id.is_ref_counted() && ObjectDB::get_instance(_get_obj().id) == nullptr) {
valid = false;
return false;
}
#endif
Callable::CallError ce;
ce.error = Callable::CallError::CALL_OK;
Array ref;
ref.push_back(r_iter);
Variant vref = ref;
const Variant *refp[] = { &vref };
Variant ret = _get_obj().obj->callp(CoreStringNames::get_singleton()->_iter_init, refp, 1, ce);
if (ref.size() != 1 || ce.error != Callable::CallError::CALL_OK) {
valid = false;
return false;
}
r_iter = ref[0];
return ret;
} break;
case STRING: {
const String *str = reinterpret_cast<const String *>(_data._mem);
if (str->is_empty()) {
return false;
}
r_iter = 0;
return true;
} break;
case DICTIONARY: {
const Dictionary *dic = reinterpret_cast<const Dictionary *>(_data._mem);
if (dic->is_empty()) {
return false;
}
const Variant *next = dic->next(nullptr);
r_iter = *next;
return true;
} break;
case ARRAY: {
const Array *arr = reinterpret_cast<const Array *>(_data._mem);
if (arr->is_empty()) {
return false;
}
r_iter = 0;
return true;
} break;
case PACKED_BYTE_ARRAY: {
const Vector<uint8_t> *arr = &PackedArrayRef<uint8_t>::get_array(_data.packed_array);
if (arr->size() == 0) {
return false;
}
r_iter = 0;
return true;
} break;
case PACKED_INT32_ARRAY: {
const Vector<int32_t> *arr = &PackedArrayRef<int32_t>::get_array(_data.packed_array);
if (arr->size() == 0) {
return false;
}
r_iter = 0;
return true;
} break;
case PACKED_INT64_ARRAY: {
const Vector<int64_t> *arr = &PackedArrayRef<int64_t>::get_array(_data.packed_array);
if (arr->size() == 0) {
return false;
}
r_iter = 0;
return true;
} break;
case PACKED_FLOAT32_ARRAY: {
const Vector<float> *arr = &PackedArrayRef<float>::get_array(_data.packed_array);
if (arr->size() == 0) {
return false;
}
r_iter = 0;
return true;
} break;
case PACKED_FLOAT64_ARRAY: {
const Vector<double> *arr = &PackedArrayRef<double>::get_array(_data.packed_array);
if (arr->size() == 0) {
return false;
}
r_iter = 0;
return true;
} break;
case PACKED_STRING_ARRAY: {
const Vector<String> *arr = &PackedArrayRef<String>::get_array(_data.packed_array);
if (arr->size() == 0) {
return false;
}
r_iter = 0;
return true;
} break;
case PACKED_VECTOR2_ARRAY: {
const Vector<Vector2> *arr = &PackedArrayRef<Vector2>::get_array(_data.packed_array);
if (arr->size() == 0) {
return false;
}
r_iter = 0;
return true;
} break;
case PACKED_VECTOR3_ARRAY: {
const Vector<Vector3> *arr = &PackedArrayRef<Vector3>::get_array(_data.packed_array);
if (arr->size() == 0) {
return false;
}
r_iter = 0;
return true;
} break;
case PACKED_COLOR_ARRAY: {
const Vector<Color> *arr = &PackedArrayRef<Color>::get_array(_data.packed_array);
if (arr->size() == 0) {
return false;
}
r_iter = 0;
return true;
} break;
default: {
}
}
valid = false;
return false;
}
bool Variant::iter_next(Variant &r_iter, bool &valid) const {
valid = true;
switch (type) {
case INT: {
int64_t idx = r_iter;
idx++;
if (idx >= _data._int) {
return false;
}
r_iter = idx;
return true;
} break;
case FLOAT: {
double idx = r_iter;
idx++;
if (idx >= _data._float) {
return false;
}
r_iter = idx;
return true;
} break;
case VECTOR2: {
double to = reinterpret_cast<const Vector2 *>(_data._mem)->y;
double idx = r_iter;
idx++;
if (idx >= to) {
return false;
}
r_iter = idx;
return true;
} break;
case VECTOR2I: {
int64_t to = reinterpret_cast<const Vector2i *>(_data._mem)->y;
int64_t idx = r_iter;
idx++;
if (idx >= to) {
return false;
}
r_iter = idx;
return true;
} break;
case VECTOR3: {
double to = reinterpret_cast<const Vector3 *>(_data._mem)->y;
double step = reinterpret_cast<const Vector3 *>(_data._mem)->z;
double idx = r_iter;
idx += step;
if (step < 0 && idx <= to) {
return false;
}
if (step > 0 && idx >= to) {
return false;
}
r_iter = idx;
return true;
} break;
case VECTOR3I: {
int64_t to = reinterpret_cast<const Vector3i *>(_data._mem)->y;
int64_t step = reinterpret_cast<const Vector3i *>(_data._mem)->z;
int64_t idx = r_iter;
idx += step;
if (step < 0 && idx <= to) {
return false;
}
if (step > 0 && idx >= to) {
return false;
}
r_iter = idx;
return true;
} break;
case OBJECT: {
if (!_get_obj().obj) {
valid = false;
return false;
}
#ifdef DEBUG_ENABLED
if (EngineDebugger::is_active() && !_get_obj().id.is_ref_counted() && ObjectDB::get_instance(_get_obj().id) == nullptr) {
valid = false;
return false;
}
#endif
Callable::CallError ce;
ce.error = Callable::CallError::CALL_OK;
Array ref;
ref.push_back(r_iter);
Variant vref = ref;
const Variant *refp[] = { &vref };
Variant ret = _get_obj().obj->callp(CoreStringNames::get_singleton()->_iter_next, refp, 1, ce);
if (ref.size() != 1 || ce.error != Callable::CallError::CALL_OK) {
valid = false;
return false;
}
r_iter = ref[0];
return ret;
} break;
case STRING: {
const String *str = reinterpret_cast<const String *>(_data._mem);
int idx = r_iter;
idx++;
if (idx >= str->length()) {
return false;
}
r_iter = idx;
return true;
} break;
case DICTIONARY: {
const Dictionary *dic = reinterpret_cast<const Dictionary *>(_data._mem);
const Variant *next = dic->next(&r_iter);
if (!next) {
return false;
}
r_iter = *next;
return true;
} break;
case ARRAY: {
const Array *arr = reinterpret_cast<const Array *>(_data._mem);
int idx = r_iter;
idx++;
if (idx >= arr->size()) {
return false;
}
r_iter = idx;
return true;
} break;
case PACKED_BYTE_ARRAY: {
const Vector<uint8_t> *arr = &PackedArrayRef<uint8_t>::get_array(_data.packed_array);
int idx = r_iter;
idx++;
if (idx >= arr->size()) {
return false;
}
r_iter = idx;
return true;
} break;
case PACKED_INT32_ARRAY: {
const Vector<int32_t> *arr = &PackedArrayRef<int32_t>::get_array(_data.packed_array);
int32_t idx = r_iter;
idx++;
if (idx >= arr->size()) {
return false;
}
r_iter = idx;
return true;
} break;
case PACKED_INT64_ARRAY: {
const Vector<int64_t> *arr = &PackedArrayRef<int64_t>::get_array(_data.packed_array);
int64_t idx = r_iter;
idx++;
if (idx >= arr->size()) {
return false;
}
r_iter = idx;
return true;
} break;
case PACKED_FLOAT32_ARRAY: {
const Vector<float> *arr = &PackedArrayRef<float>::get_array(_data.packed_array);
int idx = r_iter;
idx++;
if (idx >= arr->size()) {
return false;
}
r_iter = idx;
return true;
} break;
case PACKED_FLOAT64_ARRAY: {
const Vector<double> *arr = &PackedArrayRef<double>::get_array(_data.packed_array);
int idx = r_iter;
idx++;
if (idx >= arr->size()) {
return false;
}
r_iter = idx;
return true;
} break;
case PACKED_STRING_ARRAY: {
const Vector<String> *arr = &PackedArrayRef<String>::get_array(_data.packed_array);
int idx = r_iter;
idx++;
if (idx >= arr->size()) {
return false;
}
r_iter = idx;
return true;
} break;
case PACKED_VECTOR2_ARRAY: {
const Vector<Vector2> *arr = &PackedArrayRef<Vector2>::get_array(_data.packed_array);
int idx = r_iter;
idx++;
if (idx >= arr->size()) {
return false;
}
r_iter = idx;
return true;
} break;
case PACKED_VECTOR3_ARRAY: {
const Vector<Vector3> *arr = &PackedArrayRef<Vector3>::get_array(_data.packed_array);
int idx = r_iter;
idx++;
if (idx >= arr->size()) {
return false;
}
r_iter = idx;
return true;
} break;
case PACKED_COLOR_ARRAY: {
const Vector<Color> *arr = &PackedArrayRef<Color>::get_array(_data.packed_array);
int idx = r_iter;
idx++;
if (idx >= arr->size()) {
return false;
}
r_iter = idx;
return true;
} break;
default: {
}
}
valid = false;
return false;
}
Variant Variant::iter_get(const Variant &r_iter, bool &r_valid) const {
r_valid = true;
switch (type) {
case INT: {
return r_iter;
} break;
case FLOAT: {
return r_iter;
} break;
case VECTOR2: {
return r_iter;
} break;
case VECTOR2I: {
return r_iter;
} break;
case VECTOR3: {
return r_iter;
} break;
case VECTOR3I: {
return r_iter;
} break;
case OBJECT: {
if (!_get_obj().obj) {
r_valid = false;
return Variant();
}
#ifdef DEBUG_ENABLED
if (EngineDebugger::is_active() && !_get_obj().id.is_ref_counted() && ObjectDB::get_instance(_get_obj().id) == nullptr) {
r_valid = false;
return Variant();
}
#endif
Callable::CallError ce;
ce.error = Callable::CallError::CALL_OK;
const Variant *refp[] = { &r_iter };
Variant ret = _get_obj().obj->callp(CoreStringNames::get_singleton()->_iter_get, refp, 1, ce);
if (ce.error != Callable::CallError::CALL_OK) {
r_valid = false;
return Variant();
}
//r_iter=ref[0];
return ret;
} break;
case STRING: {
const String *str = reinterpret_cast<const String *>(_data._mem);
return str->substr(r_iter, 1);
} break;
case DICTIONARY: {
return r_iter; //iterator is the same as the key
} break;
case ARRAY: {
const Array *arr = reinterpret_cast<const Array *>(_data._mem);
int idx = r_iter;
#ifdef DEBUG_ENABLED
if (idx < 0 || idx >= arr->size()) {
r_valid = false;
return Variant();
}
#endif
return arr->get(idx);
} break;
case PACKED_BYTE_ARRAY: {
const Vector<uint8_t> *arr = &PackedArrayRef<uint8_t>::get_array(_data.packed_array);
int idx = r_iter;
#ifdef DEBUG_ENABLED
if (idx < 0 || idx >= arr->size()) {
r_valid = false;
return Variant();
}
#endif
return arr->get(idx);
} break;
case PACKED_INT32_ARRAY: {
const Vector<int32_t> *arr = &PackedArrayRef<int32_t>::get_array(_data.packed_array);
int32_t idx = r_iter;
#ifdef DEBUG_ENABLED
if (idx < 0 || idx >= arr->size()) {
r_valid = false;
return Variant();
}
#endif
return arr->get(idx);
} break;
case PACKED_INT64_ARRAY: {
const Vector<int64_t> *arr = &PackedArrayRef<int64_t>::get_array(_data.packed_array);
int64_t idx = r_iter;
#ifdef DEBUG_ENABLED
if (idx < 0 || idx >= arr->size()) {
r_valid = false;
return Variant();
}
#endif
return arr->get(idx);
} break;
case PACKED_FLOAT32_ARRAY: {
const Vector<float> *arr = &PackedArrayRef<float>::get_array(_data.packed_array);
int idx = r_iter;
#ifdef DEBUG_ENABLED
if (idx < 0 || idx >= arr->size()) {
r_valid = false;
return Variant();
}
#endif
return arr->get(idx);
} break;
case PACKED_FLOAT64_ARRAY: {
const Vector<double> *arr = &PackedArrayRef<double>::get_array(_data.packed_array);
int idx = r_iter;
#ifdef DEBUG_ENABLED
if (idx < 0 || idx >= arr->size()) {
r_valid = false;
return Variant();
}
#endif
return arr->get(idx);
} break;
case PACKED_STRING_ARRAY: {
const Vector<String> *arr = &PackedArrayRef<String>::get_array(_data.packed_array);
int idx = r_iter;
#ifdef DEBUG_ENABLED
if (idx < 0 || idx >= arr->size()) {
r_valid = false;
return Variant();
}
#endif
return arr->get(idx);
} break;
case PACKED_VECTOR2_ARRAY: {
const Vector<Vector2> *arr = &PackedArrayRef<Vector2>::get_array(_data.packed_array);
int idx = r_iter;
#ifdef DEBUG_ENABLED
if (idx < 0 || idx >= arr->size()) {
r_valid = false;
return Variant();
}
#endif
return arr->get(idx);
} break;
case PACKED_VECTOR3_ARRAY: {
const Vector<Vector3> *arr = &PackedArrayRef<Vector3>::get_array(_data.packed_array);
int idx = r_iter;
#ifdef DEBUG_ENABLED
if (idx < 0 || idx >= arr->size()) {
r_valid = false;
return Variant();
}
#endif
return arr->get(idx);
} break;
case PACKED_COLOR_ARRAY: {
const Vector<Color> *arr = &PackedArrayRef<Color>::get_array(_data.packed_array);
int idx = r_iter;
#ifdef DEBUG_ENABLED
if (idx < 0 || idx >= arr->size()) {
r_valid = false;
return Variant();
}
#endif
return arr->get(idx);
} break;
default: {
}
}
r_valid = false;
return Variant();
}

@dalexeev dalexeev added this to the 4.3 milestone Mar 18, 2024
@dalexeev dalexeev requested a review from a team as a code owner March 18, 2024 11:54
@dalexeev dalexeev mentioned this pull request Mar 18, 2024
Merged
vnen
vnen requested changes Apr 24, 2024
View reviewed changes
Copy link
Member

@vnen vnen left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good, just need to remove the TODO comments.

modules/gdscript/gdscript_vm.cpp Outdated Show resolved Hide resolved
modules/gdscript/gdscript_vm.cpp Outdated Show resolved Hide resolved
@dalexeev dalexeev force-pushed the gds-fix-object-iterator-opcodes branch from 48a4a0d to 0a4cb17 Compare April 24, 2024 17:00
vnen
vnen approved these changes Apr 25, 2024
View reviewed changes
Copy link
Member

@vnen vnen left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. Thanks!

@vnen
Copy link
Member

vnen commented Apr 25, 2024

There are some weird errors on the CI tests though, not sure what's going on.

@dalexeev dalexeev force-pushed the gds-fix-object-iterator-opcodes branch from 0a4cb17 to 461a9d8 Compare April 26, 2024 05:44
@dalexeev
Copy link
Member Author

There are some weird errors on the CI tests though, not sure what's going on.

@dalexeev dalexeev force-pushed the gds-fix-object-iterator-opcodes branch from 461a9d8 to 2778069 Compare April 26, 2024 06:22
@dalexeev dalexeev added the crash label Apr 26, 2024
@dalexeev
Copy link
Member Author

To fix #74686 and #91155 I added the following:

@@ -2998,11 +2998,10 @@ Variant GDScriptFunction::call(GDScriptInstance *p_instance, const Variant **p_a
 				VariantInternal::initialize(&vref, Variant::ARRAY);
 				*VariantInternal::get_array(&vref) = ref;
 
-				Variant **args = instruction_args; // Overriding an instruction argument, but we don't need access to that anymore.
-				args[0] = &vref;
+				const Variant *args[] = { &vref };
 
 				Callable::CallError ce;
-				Variant has_next = obj->callp(CoreStringNames::get_singleton()->_iter_init, (const Variant **)args, 1, ce);
+				Variant has_next = obj->callp(CoreStringNames::get_singleton()->_iter_init, args, 1, ce);
 
 #ifdef DEBUG_ENABLED
 				if (ref.size() != 1 || ce.error != Callable::CallError::CALL_OK) {
@@ -3332,11 +3331,10 @@ Variant GDScriptFunction::call(GDScriptInstance *p_instance, const Variant **p_a
 				VariantInternal::initialize(&vref, Variant::ARRAY);
 				*VariantInternal::get_array(&vref) = ref;
 
-				Variant **args = instruction_args; // Overriding an instruction argument, but we don't need access to that anymore.
-				args[0] = &vref;
+				const Variant *args[] = { &vref };
 
 				Callable::CallError ce;
-				Variant has_next = obj->callp(CoreStringNames::get_singleton()->_iter_next, (const Variant **)args, 1, ce);
+				Variant has_next = obj->callp(CoreStringNames::get_singleton()->_iter_next, args, 1, ce);
 
 #ifdef DEBUG_ENABLED
 				if (ref.size() != 1 || ce.error != Callable::CallError::CALL_OK) {

@akien-mga akien-mga merged commit 643afab into godotengine:master Apr 26, 2024
16 checks passed
@akien-mga
Copy link
Member

Thanks!

@dalexeev dalexeev deleted the gds-fix-object-iterator-opcodes branch April 26, 2024 09:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@vnen vnen vnen approved these changes

Assignees
No one assigned
Labels
bug crash topic:gdscript
Projects
None yet
Milestone
4.3
3 participants
@dalexeev @vnen @akien-mga