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 an option to dump the player's options file. #740

Closed
wants to merge 8 commits into
base: master
from

Conversation

Projects
None yet
3 participants
@NormalPerson7
Contributor

NormalPerson7 commented Apr 19, 2018

Since last December, the speedrunning community has realised the potential for the use of multi-action macros made in-game, and lua macros constructed in the RC file, and come to the consensus that this should not be allowed in the rules of realtime speedrunning. However, there has been no way to actually check that players saying they are not using such multi-action macros are not.

This PR aims to go one step towards achieving this goal, by adding an option to dump the options file.
At some point another PR will need to be made, in order to dump a player's in-game macros into the RC file.

I will probably have to do a little more work on this to deal with extra options from the command line, but this isn't strictly necessary to get this option to work. (Done)

NormalPerson7 added some commits Apr 18, 2018

Dump the player's options file
This commit adds the RC file of the player to the character dump, to add
provisions for working out whether a game should count as a realtime
speedrun according to the consensus among the speedrunning community.

A subsequent commit will add an option regarding whether the RC file
should be dumped, and make the default for this option to be turned off.
Add an option for dumping the options file
This commit adds an option, dump_options, which, when set to true, will
dump the contents of the options file in the character dump. The option
will default to false, in order not to spam the character dump when not
required.

This will allow the realtime speedrunning community to properly assess
whether the contents of the RC file are permitted to count as a valid
human speedrun.
@@ -1748,7 +1751,7 @@ void save_player_name()
void read_options(const string &s, bool runscript, bool clear_aliases)
{
StringLineInput st(s);
Options.read_options(st, runscript, clear_aliases);
string test = Options.read_options(st, runscript, clear_aliases);

This comment has been minimized.

@NormalPerson7

NormalPerson7 Apr 19, 2018

Contributor

Feel free to tell me of a better way to do this, because it seems incredibly messy to create a variable which is never used again.

This comment has been minimized.

@aidanholm

aidanholm Apr 19, 2018

Contributor

I'm not sure why you create a variable here; can't you just ignore the return value?

In any case, I'm not a huge fan of teetering abstraction, but you could create a class deriving from FileLineReader and overriding get_line that additionally fills a string with the file contents. The API would look like this:

CopyingFileLineReader cflr(f, Options.file_contents); // second param is the string to fill
Options.read_options(cflr, runscript);

If you do this, please get the file size and pre-expand the string before filling it.

This comment has been minimized.

@NormalPerson7

NormalPerson7 Apr 19, 2018

Contributor

Is this necessary, because this would probably take a while for me to properly understand?

This comment has been minimized.

@NormalPerson7

NormalPerson7 Apr 19, 2018

Contributor

Ok so if I understand this correctly, that would actually just be adding those two lines instead of Options.read_options(blargh)? I will do that then.

This comment has been minimized.

@NormalPerson7

NormalPerson7 Apr 19, 2018

Contributor

Never mind, it isn't that easy; I'll leave it how it is currently, and just add the command line extra options into Options.file_contents.

This comment has been minimized.

@aidanholm

aidanholm Apr 19, 2018

Contributor

If you're less familiar with C++, creating such a class might be more difficult.

A third option, probably better than the current, is to read the file contents into a string, construct a StringLineInput from that, and pass that to Options.read_options instead of the FileLineReader currently used; then you have the file contents as a string as a side effect.

@@ -1748,7 +1751,7 @@ void save_player_name()
void read_options(const string &s, bool runscript, bool clear_aliases)
{
StringLineInput st(s);
Options.read_options(st, runscript, clear_aliases);
string test = Options.read_options(st, runscript, clear_aliases);

This comment has been minimized.

@aidanholm

aidanholm Apr 19, 2018

Contributor

I'm not sure why you create a variable here; can't you just ignore the return value?

In any case, I'm not a huge fan of teetering abstraction, but you could create a class deriving from FileLineReader and overriding get_line that additionally fills a string with the file contents. The API would look like this:

CopyingFileLineReader cflr(f, Options.file_contents); // second param is the string to fill
Options.read_options(cflr, runscript);

If you do this, please get the file size and pre-expand the string before filling it.

@@ -1788,6 +1791,9 @@ void game_options::read_options(LineInput &il, bool runscript,
trim_string(str);
content += str;

This comment has been minimized.

@aidanholm

aidanholm Apr 19, 2018

Contributor

content += str + "\n";

This comment has been minimized.

@NormalPerson7

NormalPerson7 Apr 19, 2018

Contributor

You think it's better on one line than two? I mean I can do that... I just thought it really didn't matter.

NormalPerson7 added some commits Apr 19, 2018

Add options from the command line to Options.file_contents
This commit allows the recording of command line extra options as well
as those from the init/RC file.
Add some extra line breaks
Just a bit of an oversight...
Read in options file manually rather than using FileLineInput (aidanh)
This commit reads in the options file as a whole, creating a char* from
it which is then added to Options.file_contents and passed to
read_options to be converted to a StringLineInput for
game_options::read_options.

This commit also removes reference to the content variable in
game_options::read_options and changes the function back to a void.

Further cleanup may be necessary if it is decided there is a better way
than new char[] to read in the file and to add provisions for if the
options file cannot be found (ie, if (!f)).
@NormalPerson7

This comment has been minimized.

Contributor

NormalPerson7 commented Apr 24, 2018

I saw there was some discussion on ##crawl-dev about the implications of using StringLineInput -- is the first method acceptable or do you want me to try the second method, seeing as it appears the StringLineInput method isn't good enough?

@aidanholm

This comment has been minimized.

Contributor

aidanholm commented Apr 24, 2018

Yeah; I think out of the options, reverting the most recent commit is probably best.

Revert "Read in options file manually rather than using FileLineInput…
… (aidanh)"

This reverts commit e1e9d6c. It was
decided that this could cause issues, and that the StringLineInput
method was intended for use with the command line. Some testing using
the new message also produced strings of random characters at the end of
the options file, which is undesirable.
@@ -1632,7 +1637,8 @@ void read_init_file(bool runscript)
if (f.error())
return;
Options.read_options(f, runscript);
string option_file_contents = Options.read_options(f, runscript);
Options.file_contents += "\n" + option_file_contents + "\n\n";

This comment has been minimized.

@brianlheim

brianlheim Apr 24, 2018

this will result in 2 copies of option_file_contents - one for the first +, and one for the +=. It would be better to append each element separately using +=. The other uses of + in this diff will similarly do some extra allocation you could avoid.

sorry for lurking, I just really like this project and sometimes take a peek at what y'all are up to :)

This comment has been minimized.

@NormalPerson7

NormalPerson7 Apr 24, 2018

Contributor

Are you saying that there will be two copies of the options file in the dump? Because some simple testing reveals that is not the case... Otherwise I don't really understand your point.

Glad to hear you like the project.

This comment has been minimized.

@NormalPerson7

NormalPerson7 Apr 24, 2018

Contributor

Just to clarify my understanding (which I believe to be correct, I'm no expert):
If options_file_contents contains one thing, eg "test", then line 1641 creates the string "\ntest\n\n", and appends this to Options.file_contents. I don't know where you think it is being duplicated.

This comment has been minimized.

@aidanholm

aidanholm Apr 24, 2018

Contributor

I believe @brianlheim is referring to temporary copies of the string containing the file contents, caused by the order of operations used: the string "\n" is created, then the file contents is copied onto the end of that, then "\n\n" is copied onto the end of that, and the result is then copied onto Options.file_contents; so the full contents of the file are copied twice. They are absolutely correct that this is less than maximally efficient, but this is somewhat less of a concern in this particular case.

This comment has been minimized.

@brianlheim

brianlheim Apr 24, 2018

@aidanholm - that is correct. You're right this is not a huge concern in this particular instance, but being neglectful of temporary copies can lead to bad performance overall if the mistake is made repeatedly. And it is made here several times already: for example, content += str + "\n" in the worst case may cause a reallocation when "\n" is appended to str, which is pessimistic when both are going to be copied to content in the end anyway.

This comment has been minimized.

@NormalPerson7

NormalPerson7 Apr 24, 2018

Contributor

Right, that makes sense, thanks guys.

This comment has been minimized.

@brianlheim

brianlheim Apr 24, 2018

Np! Sorry, I know it's kind of rude of me to interject out of the blue like this. I should say that in general I think you guys do a better job keeping this codebase clean than I probably would. :) It gives me hope that if/when I start to contribute it'll be fun and easy!

This comment has been minimized.

@aidanholm

aidanholm Apr 24, 2018

Contributor

If you think that's bad, just look at line 1798 of this diff :)

@NormalPerson7

This comment has been minimized.

Contributor

NormalPerson7 commented Apr 27, 2018

There has been some discussion about what this PR's overall goal is, and what is necessary to address the realtime speedrunning issues. The speedrunning community has come up with some specific requirements, which can be found here: https://pastebin.com/raw/kVSscaLC.

@NormalPerson7

This comment has been minimized.

Contributor

NormalPerson7 commented Jul 13, 2018

I am closing this PR now, because the realtime speedrunning community will now use video evidence to vet the very fastest of runs as had previously been suggested by many of the development team, so such option file dumping is less of a priority (even though the basic functionality may be useful for other purposes). It seemed like the best way forward for dumping the options file for other purposes was to use hashes to check when (and how) the options file was changed, and I don't feel I am the right person to fiddle around with that.

The basic functionality will remain in this PR, and I will not delete the NormalPerson7/crawl:speedrun-rc branch in case anyone else wants to pick up the pieces for this in the future. It is largely no longer required for the realtime community though.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment