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

Destiny Patch support #3131

Merged
merged 11 commits into from
Nov 18, 2024
Merged

Destiny Patch support #3131

merged 11 commits into from
Nov 18, 2024

Conversation

drxgb
Copy link
Contributor

@drxgb drxgb commented Oct 25, 2023

I'm going to start implementing Destiny Patch support on EasyRPG.
I hope can help.
Awaiting for a feedback. Thanks! =D

@jetrotal
Copy link
Contributor

jetrotal commented Oct 25, 2023

Nice!

Some Documentation, to help contextualize this patch:
Original Destiny Patch 1.0, made by Bananen-Joe:
DestinyScript_Eng.pdf
image

Destiny Patch 2.0 (also from Bananen-Joe) + Maniacs Update, by Kotatsu Akira:
https://dev.makerpendium.de/docs/destiny_help/main-de.htm?page=object_command
image

@Ghabry
Copy link
Member

Ghabry commented Oct 25, 2023

Hello. As adding the rest of destiny will be a huge task I have to ask:

Your code looks good but do you feel confident enough in C++ to implement the rest of it?

  1. You will have to write a parser for the entire destiny syntax (imo this is the hard part)
  2. Implement the functions offered by destiny (this is more tedious than hard imo, the tricky ones like sockets can be skipped for now)

@Ghabry
Copy link
Member

Ghabry commented Oct 25, 2023

Hm actually the grammar doesn't look too hard to parse because the language lacks flow control like if and for. Very straightforward.

I can offer my help here if you encounter problems.

src/destiny.cpp Outdated Show resolved Hide resolved
@drxgb
Copy link
Contributor Author

drxgb commented Oct 25, 2023

Hello. As adding the rest of destiny will be a huge task I have to ask:

Your code looks good but do you feel confident enough in C++ to implement the rest of it?

  1. You will have to write a parser for the entire destiny syntax (imo this is the hard part)
  2. Implement the functions offered by destiny (this is more tedious than hard imo, the tricky ones like sockets can be skipped for now)

Thanks for the feedback!
I'm basing on the Assembly source code made by Bananen-joe. There's a lot of work to do yet. But I'm going step by step while I'm studying the EasyRPG infrastructure.

Maybe the interpreter I could use OOP.
Besides we can create some parsing interfaces (like parser, scanner and handler) that can be implemented by another patches like DynRPG and Maniacs. Idk if this idea can be acceptable but I have this wish.

@Ghabry
Copy link
Member

Ghabry commented Oct 25, 2023

If you think you can implement such a parser then go for it :).

When it is flexible enough it can also be used for other parts as you proposed

(Maniac has nothing to parse on the player side but there is an expression parser needed for the editor.)

@Ghabry Ghabry marked this pull request as draft November 2, 2023 15:17
@Ghabry Ghabry added this to the 0.8.1 milestone May 26, 2024
@Ghabry Ghabry modified the milestones: 0.8.1, 0.8.2 Jul 1, 2024
@fdelapena fdelapena added the Awaiting Rebase Pull requests with conflicting files due to former merge label Jul 2, 2024
@Ghabry Ghabry removed the Awaiting Rebase Pull requests with conflicting files due to former merge label Nov 11, 2024
@Ghabry Ghabry modified the milestones: 0.8.2, 0.8.1 Nov 11, 2024
@Ghabry Ghabry marked this pull request as ready for review November 11, 2024 06:29
@Ghabry
Copy link
Member

Ghabry commented Nov 11, 2024

Jenkins: test this please

length = code.length() + 1;
destinyScript = new char[length];
strcpy_s(destinyScript, length, code.c_str());

Copy link
Member

Choose a reason for hiding this comment

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

strcpy_s is a Microsoft extension.

In general you do not need new or malloc in modern C++ code. You could just change the type of _destinyScript to std::string and do destinyScript = code.

You can still do the _scriptPtr stuff via _scriptPtr = &destintyScript.data().

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Like this?

private:
	// Member data
	std::string _destinyScript;
const char* Interpreter::MakeString(SaveEventExecFrame& frame)
{
	std::string code;

	int32_t& current = frame.current_command;
	const std::vector<EventCommand>& cmdList = frame.commands;
	std::vector<EventCommand>::const_iterator it = cmdList.begin() + current++;

	code = ToString((*it++).string);

	while (it != cmdList.cend() && it->code == static_cast<int32_t>(EventCommand::Code::Comment_2))
	{
		code += '\n';
		code += ToString((*it++).string);
		++current;
	}

	_destinyScript = code;
	return _scriptPtr = _destinyScript.data();
}

Copy link
Member

@Ghabry Ghabry Nov 11, 2024

Choose a reason for hiding this comment

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

Yes, this _scriptPtr assignment is legal here because _destinyScript is a member variable. (in case of a local variable this would return a dangling pointer because the memory is freed)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Nice! I'll push again to see whether the code passes.

src/player.cpp Outdated Show resolved Hide resolved
@Ghabry
Copy link
Member

Ghabry commented Nov 11, 2024

Honestly when these two issues are resolved I would be fine with merging it for now. Just to avoid regular rebasing because of conflicts in the other files.

Of course its currently not useful as is a stub and the coding style is different but most of the upcoming patch commits will be pretty isolated (only in game_destiny) and style issues can be fixed at the end as this will be huuuge when completed.

(Also we had a DynRpg interpreter but 0 implemented plugins for years xD)

@drxgb
Copy link
Contributor Author

drxgb commented Nov 11, 2024

I forgot to note: later I want to do unit tests in test_runner_player. So I'm studying its structure to apply tests to game_destiny.

@Ghabry
Copy link
Member

Ghabry commented Nov 11, 2024

Thats fine. Just create the file and add the file at the correct location in Makefile.am. CMake grabs it automatically. There shouldn't be any conflicts later as we usually don't edit that part of the file :)

@Ghabry
Copy link
Member

Ghabry commented Nov 18, 2024

Jenkins: test this please

@Ghabry Ghabry removed the Building label Nov 18, 2024
src/game_destiny.cpp Outdated Show resolved Hide resolved
@Ghabry Ghabry merged commit d472587 into EasyRPG:master Nov 18, 2024
14 checks passed
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.

4 participants