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

Shuffle Lists #145

Closed
LarskeM opened this issue Jun 3, 2018 · 2 comments
Closed

Shuffle Lists #145

LarskeM opened this issue Jun 3, 2018 · 2 comments
Labels

Comments

@LarskeM
Copy link

LarskeM commented Jun 3, 2018

Hi,
was playing around with procedural text generation in ink (like https://www.patreon.com/posts/tips-and-tricks-18636280 or https://gist.github.com/joethephish/2b0a291806b3100390fdddd56defd169)
which is a lot of fun, but got a weird problem with inkjs and shuffle lists.

-> conversation_loop

=== conversation_loop 
	+	[Test Shuffle] -> test_shuffle
	+	[Done] -> test_done

=== test_shuffle
    {first()} and {second()}
    -> conversation_loop
    
===  test_done
    Bye :)
    -> END

== function first ==
{~
 - A
 - B
 - C
 - D
 - E
 }
 
 == function second ==
{~
 - A
 - B
 - C
 - D
 - E
 }

When playing this ink in inky or inklecrate you get the expected
A and E
B and C
...

Playing with inkjs you get
E and E
B and B
A and A
...

I traced this down to NextSequenceShuffleIndex()
For both shuffle lists this function gets called with exactly the same state/values, and so returns the same choice. This problem only occurs with lists of the same length, because the length is part of the random seed calculation (so don't use lists of the same length in the same place and you are fine).

I do not understand, why the c# implementation behaves differently. The code nearly looks the same. But I can't debug the c# code.

My problem could be fixed with something like

NextSequenceShuffleIndex(){

	var numElementsIntVal = this.state.PopEvaluationStack();
	if (!(numElementsIntVal instanceof IntValue)) {
		this.Error("expected number of elements in sequence for shuffle index");
		return 0;
	}

	var numElements = numElementsIntVal.value;
			
	let min = 0
	let max = Math.floor(numElements);
	// exclusive MAX
	let chosen=  Math.floor(Math.random() * (max - min)) + min;

	return chosen
}

which works fine for me, but because you are losing the seedable random number and the 'sequence' functionality, this isn't really a patch.

@y-lohse
Copy link
Owner

y-lohse commented Jun 4, 2018

Hi, thanks for the report! This looks like a bug.

We try to keep the C# and JS codebases as close as possible, so as you guessed we can't take that patch as it is :). I'll have to look at the C# code again, there's probably a minor difference somewhere.

@y-lohse y-lohse added the bug label Jun 4, 2018
NQNStudios added a commit to NQNStudios/inkjs that referenced this issue Jun 5, 2018
@NQNStudios
Copy link
Contributor

I've fixed it! @y-lohse I see that by trying to update the ink.js files in the two templates I inadvertently introduced way more changes in my PR than expected, so let me know if that's a problem.

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

No branches or pull requests

3 participants