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.

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

Fix a problem where Object.callv() and Callable.callv() would not correctly invoke when a read-only Array was passed. #915

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

ydipeepo
Copy link

@ydipeepo ydipeepo commented Jan 1, 2025

As mentioned in the title, I made a small patch because the behavior was unexpected in the following case:

func f(a: int) -> void:
    print(a) # "1" its okay

func g(a: int, b: int) -> void:
    print(a, b) # "11" but it should be "12"

func _ready() -> void:
    const A := [1]
    const B := [1, 2]
    f.callv(A)
    g.callv(B)

When passing a read-only Array, every argptrs in callv() address to same memory, so in cases where the bad conditions within callv() are met, I changed it to copy the arguments.

  • Variant &Array::operator[](int p_idx) {
    if (unlikely(_p->read_only)) {
    *_p->read_only = _p->array[p_idx];
    return *_p->read_only;
    }
    return _p->array.write[p_idx];
    }
    const Variant &Array::operator[](int p_idx) const {
    if (unlikely(_p->read_only)) {
    *_p->read_only = _p->array[p_idx];
    return *_p->read_only;
    }
    return _p->array[p_idx];
    }

It would be more appropriate to modify Array or ScriptInstance::callp(), but in that case, the impact would be extensive so this is a mitigation measure.

@SkogiB
Copy link
Contributor

SkogiB commented Jan 18, 2025

Known issue in Godot as well, but no real fixes
godotengine/godot#93600

Chasing links down, it seems they decided to try fixing the read only arrays instead and got stuck in the weeds there
Here's that further link discussing fixing read only arrays: godotengine/godot#93702

Here they had a PR where it duplicates the array, but didn't merge it over performance concerns: godotengine/godot#93636

@ydipeepo
Copy link
Author

Thank you for the related links and research, and I apologize for the trouble.
I also read a little of the original source code, but it seems to be class layouts that cannot be easily fixed without changing quite a few files and/or the Array API itself. 🫠
It doesn't seem like there are any simple solutions, so I will just keep an eye on it.

By the way, since it seems to be tracking the original, can I close this PR?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: Open
Development

Successfully merging this pull request may close these issues.

3 participants