Access global Ink variables [v2] #14

JBenda · 2021-02-07T23:27:56Z

Enables reading and writing of INK global variables from C++ code.

Use template function in class global to access them.

class {
	// @brief access global variable from ink story
	// @param name name of variable (as defined in Ink Script)
	// @return nullptr if there is no such variable
	// @return pointer to variable
	template<typename T>
	T* get_global(const char* name);
}

JBenda · 2021-02-07T23:35:54Z

So here we are again ^^

There is still the thing with strings, a read access is no problem, therefor we can just stash them and handle the pointer, a write access is quite a tricky thing I would postpone to a later point.

I will think about it the next few days, but I am not optimistic.
Maybe a String access class which only give read access, but you can ask to privilege it to write access (which then will make a static string to an allocated and then edit this, but what happens when you want to set it to a static string, than you are not allowed to free it later on, etc. [maybe a maximal string size? -> no reallocation, but memory cost])

brwarner · 2021-02-08T00:30:09Z

I'm fine with string writing being added later. There's a bunch of holes in the runtime right now concerning strings. Thinking about how to implement it though...

I think you're right about returning some kind of special "container" class. It would support read-access (maybe via overloading []/==/etc.). For writing, maybe we'd just overload the assignment operator. When you assign, it asks the globals object to create a new string using its string_table (not sure if you've looked into it yet, but it's how I deal with dynamically allocating strings during runtime when it's necessary to do so) and then throw that pointer into the variable.

You don't have to worry about GC with the string table. The runtime periodically runs through all variables and deletes any entries in the string table it can't find in any of them.

I think we just wouldn't allow write access that wasn't assignment. I can hardly see a use case where someone wants to read a variable and just muck with a few of its characters. If they really want to do that, they can copy the value into a std;:string or something and just assign it back to our special string container.

JBenda · 2021-02-08T09:05:00Z

That's sounds good, I will try it this week ^^

JBenda · 2021-02-08T15:39:43Z

It's an idea of how it can work, it's only a scratch now, but it seems to work

brwarner · 2021-02-11T17:11:14Z

inkcpp/include/globals.h

+		friend class ink::runtime::internal::globals_impl;
+		global_string(const internal::globals_impl& globals, const char* string);
+
+		const internal::globals_impl& _globals;


This should actually use the shared_ptr globals defined in types.h. Remember that it's possible for the globals object to be destroyed while one of these global_strings is in scope (imagine the user reads the value of a variable, stores that somewhere, then lets the ink story and runners die). The shared pointer can guard you against that. Just make sure you check it for validity and maybe throw an exception if they try to do a write when it's not valid.

brwarner · 2021-02-13T19:50:00Z

inkcpp/globals_impl.cpp

+	}
+
+	const char * const * globals_impl::get_str(hash_t name) const {
+		return fetch_variable<const char*, &value::as_str_ptr>(


This will fail if the value is a string composed of multiple concatenated data types that aren't strings. Example 5 + "hi" + 4. get_data_type() just returns the type of the first piece of data in the value. It's type() that does the resolution to realize that a value with a mix of numbers and strings is actually a string.

Oh, yes, this is a thing, because:
The getter is const, but modifying a value is not, maybe we can make the _variables mutable? I don't realy see a other way to solve this problem,
creating each time a new string table entry and write it is also possible, but then we must parse it every time ...

also the get<const char*> should contain a warning that the value is only valid until the next runner call ^^

shared/public/system.h

brwarner · 2021-02-13T19:54:03Z

Wasn't sure if this was ready for review yet but I went through it and only really found one issue.

JBenda · 2021-02-13T20:18:46Z

No problem, I would wrap it up this weekend.
I always appreciate when someone else (especially when he knows the code base better) take a look at my code.
But often I hesitate to ask, because it is a time investment. So I'm very grateful that you review so frequently, thank you.

brwarner · 2021-02-13T20:36:03Z

But often I hesitate to ask, because it is a time investment

Just leave a comment saying you'd like a review before proceeding and I'll do it when I can. Don't worry about sounding pestering. You're making this project move faster, not slower. I actually need to dedicate less time to it because of you :)

brwarner · 2021-02-13T23:00:11Z

inkcpp_test/Globals.cpp

+{
+	GIVEN ("a story with global variables")
+	{
+		story* ink = story::from_file("ink/GlobalStory.bin");


I actually just merged in a PR that makes sure the Inklecate compiler is available on all OSs so you can go back to loading from the ink file directly.

#21

JBenda · 2021-02-14T16:15:01Z

This should be functional know :)
Pls check if I don't have done something silly.

brwarner · 2021-02-15T21:37:18Z

Cool. I'll take a look tomorrow :)

brwarner

Everything looks good and so do the test cases. Great job on this. I saw one errant FIXME comment which I think you can just remove now? Other than that, it's good to merge.

inkcpp/value.h

brwarner · 2021-02-17T06:21:42Z

If you fix the merge conflict in value.cpp I'll merge the branch in.

supported: + float + int + uint missing: + string + knodes

Avoid mess with string class, and maybe not existing global memory. Getter returns optional

+ replace auto with explicit type

JBenda · 2021-02-17T10:18:19Z

Thank you for the review.
The FIXME slipped me ^^

The branch is now rebased to master.

JBenda marked this pull request as draft February 7, 2021 23:41

brwarner reviewed Feb 11, 2021

View reviewed changes

brwarner reviewed Feb 13, 2021

View reviewed changes

shared/public/system.h Show resolved Hide resolved

brwarner reviewed Feb 13, 2021

View reviewed changes

JBenda marked this pull request as ready for review February 14, 2021 16:38

JBenda requested a review from brwarner February 15, 2021 11:25

brwarner approved these changes Feb 17, 2021

View reviewed changes

inkcpp/value.h Outdated Show resolved Hide resolved

JBenda added 12 commits February 17, 2021 11:15

Add interface for access global variables

0339d36

Add editable global variables

f6b386c

supported: + float + int + uint missing: + string + knodes

Remove mutable

baad887

Adapt coding convention

9243078

Add string support for Globals

52660df

Allow access lower privileged string

bf6e985

Implement Idea with getter and setter for variable

cf3ee16

Avoid mess with string class, and maybe not existing global memory. Getter returns optional

Fix Microsoft Compiler error

d979f9b

+ replace auto with explicit type

Compile inkBin to test time from ink

cd26501

Add test case for contacted string value

b78a98a

Add functionality to finalize string values

f9dba1c

Remove outdated FIXME

f0fced3

brwarner merged commit 8c3f506 into JBenda:master Feb 17, 2021

JBenda deleted the feature/GetGlobals branch February 22, 2021 11:55

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Access global Ink variables [v2] #14

Access global Ink variables [v2] #14

JBenda commented Feb 7, 2021

JBenda commented Feb 7, 2021 •

edited

Loading

brwarner commented Feb 8, 2021

JBenda commented Feb 8, 2021

JBenda commented Feb 8, 2021

brwarner Feb 11, 2021

brwarner Feb 13, 2021

JBenda Feb 13, 2021

brwarner commented Feb 13, 2021

JBenda commented Feb 13, 2021

brwarner commented Feb 13, 2021

brwarner Feb 13, 2021

JBenda Feb 14, 2021

JBenda commented Feb 14, 2021

brwarner commented Feb 15, 2021

brwarner left a comment

brwarner commented Feb 17, 2021

JBenda commented Feb 17, 2021

Access global Ink variables [v2] #14

Access global Ink variables [v2] #14

Conversation

JBenda commented Feb 7, 2021

JBenda commented Feb 7, 2021 • edited Loading

brwarner commented Feb 8, 2021

JBenda commented Feb 8, 2021

JBenda commented Feb 8, 2021

brwarner Feb 11, 2021

Choose a reason for hiding this comment

brwarner Feb 13, 2021

Choose a reason for hiding this comment

JBenda Feb 13, 2021

Choose a reason for hiding this comment

brwarner commented Feb 13, 2021

JBenda commented Feb 13, 2021

brwarner commented Feb 13, 2021

brwarner Feb 13, 2021

Choose a reason for hiding this comment

JBenda Feb 14, 2021

Choose a reason for hiding this comment

JBenda commented Feb 14, 2021

brwarner commented Feb 15, 2021

brwarner left a comment

Choose a reason for hiding this comment

brwarner commented Feb 17, 2021

JBenda commented Feb 17, 2021

JBenda commented Feb 7, 2021 •

edited

Loading