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

Add ListSupport (+Ink Proof completed) #17

Merged
merged 124 commits into from
Aug 7, 2021
Merged

Add ListSupport (+Ink Proof completed) #17

merged 124 commits into from
Aug 7, 2021

Conversation

JBenda
Copy link
Owner

@JBenda JBenda commented Feb 11, 2021

Add List support and tweak output and logic to pass ink-proof and run complex ink stories.

@brwarner
Copy link
Collaborator

A couple thoughts I had while reading

Since each story has a fixed number of possible lists/values which we know at runtime, could lists be stored as one giant bit mask instead of a set? For example, suppose we open a story file and see it has 5 lists each with 4 possible values. We know that means there's only 20 possible flags for each variable and thus any list variable only has to be a single 32-bit uint. However, if there's 20 lists of 10 values, we'd know each list variable needs to be 8 32-bit uints. I guess the downside of this is that each list variable would be the largest it possibly has to be (even if it only ever holds one flag). It would mean however that changing the value of a list requires less memory management.

But that brings me to another question... which is, are lists variables immutable? Would we ever actually modify one? I guess it depends how we handle things. If I set a variable equal to another variable's value, does it copy or simply copy the ref to our list value in the table? If it copies the ref, we basically can never mutate them and any modifications result in a copy and mutate.

Anyway, none of these are really concrete requests to go one way or another. Just me thinking aloud. Either way we need to have a list table.

@JBenda
Copy link
Owner Author

JBenda commented Feb 11, 2021

An example of the encoding desrcipt in the notes, just for clarification ^^

InkCode:

LIST animals = tiger, cat, (dog), bird
LIST rooms = kitchen, garage, (living_room)

InkJson:

{
	...,
	{
		"global decl": [
			"ev",
			{
				"list": {
					"animals.dog": 3
				}
			},
			{
				"VAR=": "animals"
			},
			{
				"list": {
					"rooms.living_room": "3"
				}
			},
			{
				"VAR=": "rooms"
			}
			...
		]
	}
	"listDefs": {
		"animals": {
			"tiger": 1,
			"cat": 2,
			"dog": 3,
			"bird": 4
		},
		"rooms": {
			"kitchen": 1,
			"garage": 2,
			"living_room": 3
		}
	}
}

Encoding:

keys: ["tiger", "cat", "dog", "bird", "kitchen", "garage", "living_room"]
fid: [0,4]
entries: [0b100010000..., 0b010000001...]

encoding:
entry[0] = contains flags from list animals?
entry[1] = contains flags from list rooms?
entry[2] = tiger?
entry[3] = cat?
entry[4] = dog?
entry[5] = bird?
entry[6] = kitchen?
entry[7] = garage
entry[8] = living_room

@JBenda
Copy link
Owner Author

JBenda commented Feb 11, 2021

The mutable thing is a good point, It seems like that's each result of an operation results in a new list, and assignment is just an assignment of reference

@JBenda JBenda self-assigned this Aug 3, 2021
@JBenda JBenda marked this pull request as ready for review August 3, 2021 10:22
This was referenced Aug 3, 2021
@brwarner
Copy link
Collaborator

brwarner commented Aug 5, 2021

Got it to compile and run the intercept on my machine successfully. I saw a few whitespace issues (newlines where there shouldn't be) but I got no errors. I did some looking over the code (I'd looked over a lot of the new value stuff in the old PR) but honestly it's just way to much for me to review personally. I think at this point, unless there's something specific you're worried about you want me to look at, I can just merge this in and we see what happens. I'm confident in your judgements.

@JBenda JBenda changed the title List Support Add ListSupport (+Ink Proof completed) Aug 7, 2021
@JBenda
Copy link
Owner Author

JBenda commented Aug 7, 2021

It feels quite completed for me. The string output logic is quite a mess with the gluing 😅.
And with the tests now also running on Windows, it should be fine. 👍 pls merge ^^

to avoid permission erros on windows
@brwarner brwarner merged commit 7d9eb08 into JBenda:master Aug 7, 2021
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.

2 participants