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

ensemble.init() doesn't do anything #82

Open
meldckn opened this issue Nov 19, 2019 · 1 comment
Open

ensemble.init() doesn't do anything #82

meldckn opened this issue Nov 19, 2019 · 1 comment

Comments

@meldckn
Copy link
Collaborator

meldckn commented Nov 19, 2019

The current code comments and old tutorials say that you need to initialize ensemble by calling ensemble.init(), but removing it from our sampleGame doesn't break anything, and the code doesn't look like it does anything:

The public ensemble.init() function definition:

var init = function() {
  socialRecord.init();		
  return "Ok";
};

where socialRecord.init is defined as:

var init = function(initialTimeStep) {
  if (initialTimeStep !== undefined) {
    currentTimeStep = initialTimeStep;
  }
  //offstageCharacters = [];
};

So any "initialTimeStep" passed as a parameter does not get passed along to socialRecord.init() to initialize the timestep to a particular number. Which means initialTimeStep is always undefined and no code actually runs except return "OK";.

Short story: we could just ignore or delete ensemble.init() and everything will be fine. You can't initialize Ensemble with a different starting time step, but you could achieve essentially the same thing with ensemble.setupNextTimeStep(timestep).


Long story:

If we edit ensemble.init() so that a timestep parameter passed to it is passed on to socialRecord.init(), we create some unintended consequences when we set the initial timestep to anything that isn't 0 or -1 using ensemble.init(timestep). Running with the Lovers and Rivals project (in sampleGame) (with the above modification to ensemble.js) for example, your history won't load and you'll get Uncaught TypeError: Cannot read property 'push' of undefined for the socialRecord[timeStep].push(socialRecordPredicate); (described below).

Based on a cursory look, it seems as though set() (the function to save a "predicate" or piece of state to the socialRecord at the current timestep, as called from addHistory()) will break if the current timestep cannot act as an index into the socialRecord array. This will happen if you initialize the time step to 7, for example, and the loaded history.json has only 1 pos object. Eg.,

{
  "history": [{
    "pos": 0,
    "data": [{
      "category" : "trait",
      "type" : "named hero",
      "first" : "hero",
      "value" : true
    }]
  }]
}

Then the following line in set() (1740 of standalone ensemble.js or 506 of socialRecord.js)

socialRecord[timeStep].push(socialRecordPredicate);

will try to push the new predicate to the array of predicates at the current time index of the socialRecord array. Normally, the first time this is called, socialRecord will be equal to an array with exactly one empty array in it, or [Array(0)] according to Chrome console formatting. And the timeStep will be 0, so it will successfully push the predicate to that empty array. And so on for the rest of the history.

But if we call ensemble.init(7) before loading the history, the first time set() is invoked (by addHistory(), socialRecord will be [empty, Array(0), Array(0), Array(0), Array(0), Array(0), Array(0), Array(0)]. The timestep will still be 0 (which I found surprising—why not 7? Maybe it increments for every pos object in history.json). So socialRecord[0] will not be something you can push to, because it's empty not an array of zero length. Hence the Uncaught TypeError: Cannot read property 'push' of undefined error.

This is all to say that ensemble.init() isn't currently doing anything and it isn't trivial to make it do something at this point.

@mkremins
Copy link
Collaborator

This is a really good catch – it never would've occurred to me to check this. IMO, based on what you've said here, we should probably just remove the ensemble.init() function entirely.

@meldckn meldckn added this to the Next Release (1.0.2?) milestone Nov 21, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants