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

Massive var-array fix (CAUTION: affects all vars) #856

Merged
merged 4 commits into from Oct 30, 2014

Conversation

sorlok
Copy link
Contributor

@sorlok sorlok commented Oct 27, 2014

This branch contains a lot of changes to arrays-inside-variables. It brings arrays closer to GM feature parity.

  1. Removed placement new; this will (hopefully) make the code more readable and make memory leaks easier to trace.
  2. Implemented the various array length functions.
  3. Added printing for arrays; if a var's value type is multi-dimensional, it is printed as a 2-D or 3-D array.
  4. Fixed (x,y) storage for arrays; GM does it backwards.

Please be careful with this branch; I am fairly confident in it, but it does affect the "var" class extensively.

@RobertBColton
Copy link
Contributor

@sorlok Excellent work, as usual.

I can confirm your changes conform to ECMA standards and so do GM: Studio's with the following test.

myarr[1] = 50;
myarr[56] = 29;
myarr2[56] = 29;
myarr2[1] = 50;
show_message(string(array_length_1d(myarr))); // should be 57
show_message(string(array_length_1d(myarr2))); // should be 57

However I must note two discrepancies. If it is not too much to ask, can you resolve these two issues before I merge?

  1. You forgot is_array, like is_real, and is_string
  2. Additional warnings now appear in the console.
Universal_System/lua_table.h:78:10: warning: 'lua_table<variant>::mx_size' will be initialized after [-Wreorder]
   size_t mx_size; //Actual max_element+1
          ^
Universal_System/lua_table.h:72:10: warning:   'size_t lua_table<variant>::dn_reserve' [-Wreorder]
   size_t dn_reserve; //Total available units.
          ^
Universal_System/lua_table.h:160:3: warning:   when initialized here [-Wreorder]
   lua_table<T>() : dense(new T[1]), mx_size(1), dn_reserve(1) {
   ^

Otherwise your pull request is excellent and again I am ecstatic about having you as a contributor here sorlok. I can also confirm that several games continue to work as expected with your changes, including Project Mario where Project K is utilized to load collision data with arrays from d3d model files.

I have also taken the liberty of documenting the new functions on the Wiki for you.
http://enigma-dev.org/docs/Wiki/Data_Structure_Functions

@sorlok
Copy link
Contributor Author

sorlok commented Oct 27, 2014

Excellent feedback; I'll fix the warnings and add the is_array function.

@RobertBColton RobertBColton mentioned this pull request Oct 28, 2014
6 tasks
@sorlok
Copy link
Contributor Author

sorlok commented Oct 30, 2014

I've added is_array(), and fixed the reorder warnings.

A note about is_array(); due to the way that we handle arrays, the following won't work right:

a[0] = 10; //is_array() prints "false", since arrays with one element are considered to be scalar.
a[1] = 10; //Now is_array() prints "true"
a = 10; //is_array() still prints "true".

I can't think of any clever ways of fixing this right now, but normal array usage probably won't encounter these errors.

@RobertBColton
Copy link
Contributor

@sorlok I expanded your test to the following, I got false, true, true in ENIGMA and true, true, false in Studio.

a[0] = 10; //is_array() prints "false", since arrays with one element are considered to be scalar.
show_message(string(is_array(a)));
a[1] = 10; //Now is_array() prints "true"
show_message(string(is_array(a)));
a = 10; //is_array() still prints "true".
show_message(string(is_array(a)));

I can confirm the compile errors have been resolved. If there is anything else you would like to add please do so, otherwise I am prepared to merge this pull request, the is_array() function at least partially works. Josh says ECMA compliance will side with YoYo on this one, but that's fine for now.

@RobertBColton
Copy link
Contributor

After testing on several games, I am approving this commit because it meets our expectations and improves the functionality of the engine. Great work @sorlok!

RobertBColton added a commit that referenced this pull request Oct 30, 2014
Massive var-array fix (CAUTION: affects all vars)
@RobertBColton RobertBColton merged commit d6ab62b into enigma-dev:master Oct 30, 2014
@sorlok
Copy link
Contributor Author

sorlok commented Oct 30, 2014

Excellent! If we ever decide to do a massive rewrite of variables (probably with the new parser), let me know and I'll see about fixing is_array().

@RobertBColton
Copy link
Contributor

@sorlok Alright! I don't exactly understand much about it, but yeah bets are on the new parser.

@TheExDeus
Copy link
Member

When I have 2D array, I can't get correct size. Like so:

inputs[0,0] = "Number of images";
inputs[0,1] = 0;
inputs[0,2] = 0;
inputs[0,3] = -1;

inputs[1,0] = "Directory";
inputs[1,1] = 0;
inputs[1,2] = 0;
inputs[1,3] = -1;

All combinations in array_length_2d(inputs) always either return 4 or 0.

@sorlok
Copy link
Contributor Author

sorlok commented Nov 16, 2014

Interesting; I'll have a look. It's possible something got mixed up in the length code.

@TheExDeus
Copy link
Member

Just to be more specific - I need it to return 2 at some point. I think it would at array_length_2d(inputs,0) or array_length_2d(inputs, 1).

@sorlok
Copy link
Contributor Author

sorlok commented Nov 19, 2014

Just figured it out; you actually want to use the function "array_height_2d(inputs)". The function "array_length_2d()" is used for the length of the second column.

Confirmed this behavior in GM:S. So we're doing the right thing already.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants