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

In memory only variables #1049

Closed
bi0qaw opened this issue Jan 7, 2018 · 15 comments
Closed

In memory only variables #1049

bi0qaw opened this issue Jan 7, 2018 · 15 comments
Labels
bug An issue that needs to be fixed. Alternatively, a PR fixing an issue. enhancement Feature request, an issue about something that could be improved, or a PR improving something. priority: low Issues that are not harmful to the experience but are related to useful changes or additions.

Comments

@bi0qaw
Copy link
Contributor

bi0qaw commented Jan 7, 2018

Suggestion 1

All variables starting with a "-" sign will not be saved in a database. A config option can be added to allow/prevent this behaviour (for backwards compatibility).

Suggestion 2

Fix the variable pattern matching to prevent saving of variables that do not match any pattern.

Issue

The current behaviour of Skript is to save all non-local variables in a database. Even variables that do not match any pattern specified in the config file are written to the database (the last one you specify). They are only removed once the variable file is reloaded.

Suggestion 1 would add an easy way to prevent all the outlined issues. The (probably) easiest solution would be to check for the prefix in the setVariable method in the Variables.class and simply not call the saveVariableChange method if the prefix is present.

Suggestion 2 could also be considered as a bug fix.

@TheBentoBox
Copy link
Member

TheBentoBox commented Jan 8, 2018

Just for clarification, do you mean that the "-" prefixed variables would be temporary to that instance of server uptime (i.e. until a restart/shutdown) but still globally scoped otherwise? If so that definitely sounds super useful for vars that only need to be set for a couple of ticks but or so but also need to persist outside of an event. I encounter that more often than you'd expect with the type of stuff I work on.

I'll label as both enhancement and bug since I think it fits both parameters but if anyone else (@Snow-Pyon ) thinks it could be labelled better (perhaps as just enhancement) then feel free!

@TheBentoBox TheBentoBox added bug An issue that needs to be fixed. Alternatively, a PR fixing an issue. enhancement Feature request, an issue about something that could be improved, or a PR improving something. labels Jan 8, 2018
@bi0qaw
Copy link
Contributor Author

bi0qaw commented Jan 8, 2018

Exactly. They would just be global variables that are lost once skript/the server is stopped or reloaded.

Some examples where you might use it:

  • Minigames (they never persist over reloads)
  • Variables that need constant updating. They would otherwise just make your database grow constantly. (examples are: last position of player after every tick, counter variables that increase / decrease every tick)
  • Entity variables that should not persist over reloads
  • Being able to pass variables across events/functions by index
  • Creating large number of variables without running into the "variables cannot be written fast enough to database" type errors.
  • Variables from configs / json / sql queries and all this kind of stuff

In fact, many if not most variables should be of that type. The user should make the decision whether a variable needs saving or not.

I chose the "-" as a prefix because it is similar to the underscore used for local variables. So you would have these four variable types:

  • local: {_var}
  • global: {-var}
  • global saved: {var}
  • option: {@var}

I am against giving the user a choice which prefix to use for global variables as it would make sharing scripts more difficult.

@TheBentoBox
Copy link
Member

That sounds so unbelievably useful!! Hope to see someone tackle it soon.

@Syst3ms
Copy link
Contributor

Syst3ms commented Jan 8, 2018

I think I'll tackle this.

@bi0qaw
Copy link
Contributor Author

bi0qaw commented Jan 8, 2018

@Syst3ms look at my first post. It links to the class and method where you would have to implement it. Then just add a config option to enable/disable it.

@Syst3ms
Copy link
Contributor

Syst3ms commented Jan 8, 2018

I already did that, see the memory-variables branch on my fork. I added the check in a few more places just to make sure, but it's only missing the config option

@TheLimeGlass
Copy link
Collaborator

TheLimeGlass commented Jan 12, 2018

This was talked about a long time ago back when Njol still developed. It should be a storage option rather than an option altogether, that way the user can define the configuration pattern. Good for backward compatibility as well, just in case the user already has a database that uses the - pattern or doesn't want this feature. This isn't too hard to implement since Skript already saves all variables in a runtime hashmap. I did a system similar to this for SkellettProxy/Skungee with Network Variables. I also played around with reloading and resetting all values of Skript's HashMap (Being able to have variables similar on all servers). So I have a vast knowledge of Variables.

@bi0qaw
Copy link
Contributor Author

bi0qaw commented Jan 12, 2018

It should be a storage option rather than an option altogether, that way the user can define the configuration pattern. Good for backward compatibility as well, just in case the user already has a database that uses the - pattern or doesn't want this feature.

I am against using a pattern for this. The reason is that everyone will choose a different one, making scripts less compatible with each other. Also, there is no reason to have different patterns. It is really just an on/off choice. If everyone uses his own pattern it will become a huge mess. Furthermore it is easier to read other people's code and give support if everyone can easily see what is going on.

In fact, in the current config file it is already suggested that you should use the following variable pattern for unsaved variables: (?!-).*

PS: If you don't want some variables to be saved in any database (e.g. variables that contain an %entity% which usually despawn when the server is shut down) you can modify the last database's pattern to not match all variables, e.g. use '(?!x_).' to match all variables that don't start with 'x_'. Be very cautious when doing this however as unsaved variables cannot be recovered after the server has been stopped. I recommend to use a single character to denote unsaved variables (similar to local variables' '_'), e.g. '-', in which case the last database's pattern should be '(?!-).'.

The problem is, as I explained in the initial post, that it only prevents variables from being loaded from the database but not from being saved. This is pretty pointless and that is why I suggested that it should get fixed (Suggestion 2).

Telling everyone that they should use the (?!-).* pattern or making it a default is not really nice. Especially because you give people the chance to screw things up. Just adding an option that toggles saving for all variables prefixed with a - is, in my opinion, much cleaner.

@TrademarkTM
Copy link

This must be implemented, seriously.
I use variables to create lists from files on load, and they keep being spammed in variables.csv... forever.

Also, the add %objects% to {variable::*} performance issue has to be fixed, it slows down the variables way too much over time.

@Snow-Pyon
Copy link

Also, the add %objects% to {variable::*} performance issue has to be fixed, it slows down the variables way too much over time.

Easier said then done, I'll look for a fix but no promises here, the current variable system is kinda messy but not as much as the parser so we'll see.

@Syst3ms
Copy link
Contributor

Syst3ms commented Jan 14, 2018

I actually made this, but because of major Git fuckup and lack of time, I abandoned it and the code is nowhere to be found.

@bi0qaw
Copy link
Contributor Author

bi0qaw commented Jan 25, 2018

Good news, I think I found the bug why the patterns are not working properly. It seems that everything works perfectly fine and variables are correctly saved (or not saved) as long as the server is running. Once you stop the server variables get saved all over the place. I found the issue in the FlatFileStorage file. The save() method is not working as it should. The issue is that it does not correctly check whether the variable should be saved in that file or in another one:

// ... some code
for (final VariablesStorage s : Variables.storages) {
					if (s != this && s.accept(name))
						continue outer;
				}
// ... else save in the this variable storage

Here is a possible fix for the method:

	@SuppressWarnings("unchecked")
	private final void save(final PrintWriter pw, final String parent, final TreeMap<String, Object> map) {
		outer: for (final Entry<String, Object> e : map.entrySet()) {
			final Object val = e.getValue();
			if (val == null)
				continue;
			if (val instanceof TreeMap) {
				save(pw, parent + e.getKey() + Variable.SEPARATOR, (TreeMap<String, Object>) val);
			} else {
				final String name = (e.getKey() == null ? parent.substring(0, parent.length() - Variable.SEPARATOR.length()) : parent + e.getKey());
				for (final VariablesStorage s : Variables.storages) {
					if (s.accept(name)) {
                        if (s == this) { 
                            final SerializedVariable.Value value = Classes.serialize(val);
            				if (value != null)
            					writeCSV(pw, name, value.type, encode(value.data));
                        }
                        continue outer;
                    }
				}
            }
		}
	}

Edit: Don't blame me for the weird indentation something went wrong...

@Snow-Pyon
Copy link

With the merging of #1073, shall I close this?

@Snow-Pyon Snow-Pyon added the priority: low Issues that are not harmful to the experience but are related to useful changes or additions. label Feb 11, 2018
@HYPExMon5ter
Copy link
Contributor

No, this should still be added

@bi0qaw
Copy link
Contributor Author

bi0qaw commented Feb 12, 2018

I think you can close it.

@HYPExMon5ter I fixed variable patterns and variables are now saved to the right databases (see #1073). So if you want to have in memory only variables I suggest that you change the variable pattern for all your databases from .* to (?!-).*. This will prevent saving of variables starting with a hyphen. (example: {-variable}).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug An issue that needs to be fixed. Alternatively, a PR fixing an issue. enhancement Feature request, an issue about something that could be improved, or a PR improving something. priority: low Issues that are not harmful to the experience but are related to useful changes or additions.
Projects
None yet
Development

No branches or pull requests

7 participants