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

Upstream DynRpg Parser #2282

Merged
merged 18 commits into from Oct 1, 2020
Merged

Upstream DynRpg Parser #2282

merged 18 commits into from Oct 1, 2020

Conversation

Ghabry
Copy link
Member

@Ghabry Ghabry commented Aug 17, 2020

This PR has the goal to upstream the DynRPG Parser.
This one was written as part of commisioned work in 2017/18 for the Deep8 game.

The main motivation for the submission is that this code bitrots and always updating to latest Player code is a huge task and this is imo useful code.
Right now I won't submit any plugins.
The only plugin worth upstreaming right now is the DynTextPlugin, all others are of terrible code quality or custom modifications for Deep8.

Obviously there is no plugin system in the Player, a "plugin" is just a class inheriting from DynRpgPlugin and registred in create_all_plugins.

The code is not very invasive, I only added the following "Hook points":

  • Comment Event (parses and executes DynRPG commands)
  • Load
  • Save
  • Update (Spriteset graphics update)
  • Reset

Most code in dynrpg.cpp is the parser, which works by parsing the line char-by-char and keeps track of the current state through an enum. Nothing special, typical way to implement a parser. The parser is tested as it is used in Deep8 since 2 years but this is code you better don't ever touch again :).

A sample plugin (dynrpg_easyrpg) is provided. When DynRPG is enabled (dynloader.dll present) it will export the following functions:

  • @call: This is a forwarder for RPGSS to get better warning output. It just takes the first argument as a function name and forwards the other arguments to the function
  • @easyrpg_add: 1st argument is the output variable, further arguments are the values that are added, example: @easyrpg_add 5, 10, V2, 30 <- V[5] = 10 + V[2] + 30.
  • @easyrpg_output: Outputs to the console. 1st arg: Debug, Info, Warning, Error, 2nd vararg string, 3+ varargs, example: @easyrpg_output Info, "Hello $1! Var5 is $2", World, V5 (this is a unique feature, DynRpg itself has no vararg support)

Basic DynRpg function syntax:

  • The function name is always prefixed with a @
  • An argument is either a Token (a string without ""), a string (in "") or a Variable (Token prefixed with V)

How the Parser in the Player interprets different things: When parsed for simplicity everything is stored as a string and converted to int on demand.

  • Hello - This is a Token, stored as "Hello"
  • Hello World - This is a Token, stored as "HelloWorld" (DynRpg compatible - Whitespace is removed)
  • "Hello World" - stored as "Hello World"
  • V1 - Substituted with the variable, then the result stored as string. Is a valid number.
  • VV1 - Variable Indirection, as above. Is a valid number.
  • 10 - This is a Token, stored as "Hello". Is a valid number.

The Sample Plugin writes savedata. The save format of DynRPG is very simple:

  • Header: DYNSAVE1
  • Size of the Plugin Identifier (4 byte)
  • Plugin Identifier
  • Size of the Plugin data
  • Plugin data
  • [Repeat until EOF]

The sample plugin writes "EasyRpgPlugin" as identifier and the Player version as 4 byte as an indicator that EasyRPG wrote this file and not the official DynRpg.

Loading works by looking for a plugin that has the same identifier as the one in the save file.

src/player.h Outdated
/** DynRPG */
PatchDynRpg = 1,
/** ManiacPatch */
PatchManiacPatch = 2
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why not just PatchManiac

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

no particular reason, Patch is the enum prefix and ManiacPatch the patch name ;). But I changed it.

@Ghabry Ghabry force-pushed the maniac-patch2 branch 2 times, most recently from 9a7bcee to 3fa3258 Compare August 18, 2020 11:11
@Ghabry Ghabry added the Has PR Dependencies This PR depends on another PR label Aug 18, 2020
@fdelapena fdelapena added this to the 0.6.3 milestone Aug 18, 2020
src/dynrpg_easyrpg.h Show resolved Hide resolved
@fdelapena fdelapena added Awaiting Rebase Pull requests with conflicting files due to former merge and removed Has PR Dependencies This PR depends on another PR labels Aug 24, 2020
@Ghabry Ghabry removed the Awaiting Rebase Pull requests with conflicting files due to former merge label Aug 27, 2020
@Ghabry Ghabry changed the title Upstream DynRpg Parser, Stub ManiacPatch Upstream DynRpg Parser Sep 2, 2020
Copy link
Contributor

@fmatthew5876 fmatthew5876 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As you mentioned this has been working for years and is "don't touch" code. I am approving without too thorough of a review.

One thing I would suggest, which doesn't have to be done now, is getting a good suite of unit tests around this parser to make sure future code changes and refactors don't break it.

@Ghabry
Copy link
Member Author

Ghabry commented Sep 21, 2020

Yeah Unit tests is a good idea.

I also consider replacing the macros with templates as a little challenge ^^'

@Ghabry
Copy link
Member Author

Ghabry commented Sep 21, 2020

Replaced the vector with Span (nice API) and the macro parsing with a std::tuple-integer-sequence-.....stuff.
Think I will add some tests :)

@Ghabry
Copy link
Member Author

Ghabry commented Sep 25, 2020

@CherryDT

Quick dynrpg question:

@ABc "bla",N2,123

and this plugin:

// This handler is called when an event comment was encountered
bool onComment(const char *text, const RPG::ParsedCommentData *parsedData,
	RPG::EventScriptLine *nextScriptLine, RPG::EventScriptData *scriptData,
	int	eventId, int pageId, int lineId, int *nextLineId)
{
	printf("Cmd: %s %s\n", parsedData->command, text);
	printf("Args: %d\n", parsedData->parametersCount);
	for (int i = 0; i < parsedData->parametersCount; ++i) {
		const auto& p = parsedData->parameters[i];
		if (p.type == RPG::PARAM_NUMBER)
			printf("Num: %d %f\n", i, p.number);
		else
			printf("%s: %d %s\n", p.type == RPG::PARAM_TOKEN ? "Tok" : "Str",  i, p.text);
	}
	return true; // If we weren't able to handle the command, we return true!
}

Doesn't matter what I pass for "N" the string is always empty. Shouldn't I receive the actor name?

@CherryDT
Copy link

Yes you should receive name of actor 2... (as type string)

Maybe this works only if the name was changed (not the DB default) but that would be a bug then. The N feature didn't seem to receive much use... Which DynRPG version are you testing against?

@Ghabry
Copy link
Member Author

Ghabry commented Sep 25, 2020

@CherryDT
I get an empty "type string" string.

I tested with PepsiOtaku 0.32 and with your release.

In both I'm not receiving anything.

Also rewrote the database string with the Change Name event command.

@Ghabry
Copy link
Member Author

Ghabry commented Sep 27, 2020

hmm, testing the official Cherry parser actually shows many "interesting" edge cases that I don't want to support before they are relevant in existing code.

E.g. "V1.5" is interpreted as "Read value from var 2" because 1.5 is rounded to two?! Wat.

@Ghabry
Copy link
Member Author

Ghabry commented Sep 28, 2020

@fmatthew5876

I found a strange behaviour in Variables (and Switches).
https://github.com/EasyRPG/Player/blob/master/src/game_variables.h#L122
Here and a few lines below it accesses lcf::rpg::Variables. Is this intentional? That's the only place where this global data is accessed. Considering that the class should be decoupled from lcf::rpg::variables this is strange?

When I do a unit test with the following code:

Main_Data::game_variables = std::make_unique<Game_Variables>(-99999, 99999);
std::vector<int32_t> vars = {100, 4, 2, 1};
Main_Data::game_variables->SetData(vars);
CHECK(DynRpg::ParseCommand("@FunC V2,VV3,VVVV3", args) == "func");

All arguments of DynRPG will be 0 because the check !Main_Data::game_variables->IsValid(number) fails.

Can I fix this or is the code in variables/switches correct?

@Ghabry
Copy link
Member Author

Ghabry commented Sep 28, 2020

hm wait. This is just not intuitive. IsValid checks if it is in the database range (for the Debug dialog).
So not a bug, I just have to remove the check from the dynrpg code (no idea why it is even there, there is no var limit o.O)

@Ghabry
Copy link
Member Author

Ghabry commented Sep 28, 2020

This is ready now when I can get macOS to accept my unit test...

Found actually two bad bugs in my parser:

  • Tokens (everything that is not a string literal) must be lowercased
  • @bla , was not parsed as "one empty string arg"

@Ghabry
Copy link
Member Author

Ghabry commented Sep 28, 2020

ohh, that error is interesting and shows that unit tests are useful:

To check if it is a float I do:

std::istringstream iss(str);
iss >> f;
parse_okay = !iss.fail();

libstc++ and MSVC do not set the fail-bit for "1.5x" but libc++ (clang) does. I relied on the first behaviour.

Though I was lucky here: The bug is only triggered by "x", not by "y"

see https://bugs.llvm.org/show_bug.cgi?id=17782
and https://stackoverflow.com/questions/19725070/discrepancy-between-istreams-operator-double-val-between-libc-and-libstd

@Ghabry
Copy link
Member Author

Ghabry commented Sep 28, 2020

Well fixing this is tricky so I attempted to make the problem less likely to appear instead:
The int parsing is not done via float parsing anymore (also faster this way).

It will now only fail when:

  • The parameter is a float AND
  • The float contains trailing text, e.g. "1.5x" (but "1.5 x" is fine).
    So only "wrong" input to float parameters will trigger this behaviour.

Imo for DynRPG it is fine to have some non-conforming behaviours for incorrect arguments. We can fix them whenever games (ab)use this.

Different behaviour (there are unit tests for this):

  • I make no distinction between a TOKEN and a STRING. In fact I handle everything as string and convert on-the-fly depending on the parameter type. Gives more flexibility e.g. an empty string is now for int and float a 0. A token is just provided as a string with whitespace removed and lowercased (except when N or V are seen). Shouldn't matter in practice because all plugins are part of EasyRPG and this relaxes the requirements for arguments.
  • I reject empty function names, dynrpg does not: @ aaa. Not relevant because we invoke DynRPG functions by name and empty is not a registred function.
  • DynRPG strips extra data after string literals, I reject them: @FunC "Arg"junk <- DynRPG parses Arg here. I report parse error. Note that a"Ar g"ju nk is a token and parsed as a"arg"junk :) (DynRPG compatible)
  • @FunC V2.5. DynRPG parses this as V3, I parse this as int -> V2 (result is content of Var 2)

@Ghabry
Copy link
Member Author

Ghabry commented Sep 30, 2020

rebased. Imo this PR is done.

@fdelapena fdelapena self-requested a review September 30, 2020 19:23
tests/dynrpg.cpp Outdated Show resolved Hide resolved
tests/dynrpg.cpp Outdated Show resolved Hide resolved
@fmatthew5876 fmatthew5876 merged commit 636b7b0 into EasyRPG:master Oct 1, 2020
@Ghabry Ghabry deleted the maniac-patch2 branch May 27, 2021 12:24
@Ghabry Ghabry restored the maniac-patch2 branch May 27, 2021 12:29
@Ghabry Ghabry deleted the maniac-patch2 branch February 22, 2022 13:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

Successfully merging this pull request may close these issues.

None yet

4 participants