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

ISS-33: Fixed and expanded array functions #36

Merged
merged 4 commits into from
Aug 1, 2023
Merged

Conversation

Alphish
Copy link
Owner

@Alphish Alphish commented Jul 26, 2023

As it turned out, using plain script_execute_ext(...) with large arrays caused some stack overflow error. More details in issue #33

Also, I added extra offset and length parameters, working similarly to same parameters in native array functions (like array_filter or array_map). Generally:

  • a positive offset is counted forward from the start of the array; a negative offset is counted backwards from the end of the array (with -1 being the last element index, or array length minus one); if absolute value of negative offset exceeds array length, offset of 0 is used
  • a positive length counts items forward from the offset (included); a negative length counts items backwards from the offset (also included); if the length exceeds available items (e.g. from 7th offset onwards there are only 5 items, and length of 10 was given), then only the available items are processed

@Alphish Alphish added the bug 🐞 Something isn't working label Jul 26, 2023
@Alphish Alphish self-assigned this Jul 26, 2023

var _max = -infinity;
for (var i = 0; i < _length; i += 10000) {
var _partial_max = script_execute_ext(max, _array, _offset + i, min(10000, _length - i));

Choose a reason for hiding this comment

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

Idle thought, but given that we ran into stack overflow issues, it's worth considering. I wonder if the stack size is smaller on mobile. 10,000 is likely to be fine on desktop platforms, but it's possible that this will still overflow the stack on a smaller device. I haven't tested it, but food for thought.

Copy link
Owner Author

Choose a reason for hiding this comment

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

I haven't really been involved with mobile development in GM, so I guess I'll use the "we'll think about it once someone finds the problem" approach. If need be, we might implement some script_execute_max_args() function that chooses the number depending on the platform.

Choose a reason for hiding this comment

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

Fair enough. 👍

@Alphish Alphish requested a review from Mercerenies July 31, 2023 07:56
Copy link

@Mercerenies Mercerenies 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 :)

@Alphish Alphish merged commit 7427af5 into main Aug 1, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug 🐞 Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants