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

Class / BattleCommands -related fixes #1287

Merged
merged 5 commits into from Nov 13, 2017

Conversation

Projects
None yet
5 participants
@20kdc
Contributor

20kdc commented Oct 18, 2017

This hopefully fixes Class BattleCommands issues.

A potential further improvement would be to research what happens when battle commands space is exhausted, but as this should almost never come up, I believe my current solution (sacrifice the Row command first, then just give up and become non-compliant with the 7 length bound) works.

Test cases to be used with Ib English Translation's RPG_RT v1.1.0 for comparison against Player, along with documentation on more or less what they do:
https://20kdc.duckdns.org/PR1TestCase.zip

Recommended further steps (maybe nitpicky, but still):

In Player:

ChangeClass logic to SetClass: that's for if/when SetClass gets an std::vector<uint32_t> return value or something, necessary because the skill list needs to be preserved.

In liblcf:

changed_class ought to be battle_commands_changed - it only indicates if the battle commands array is initialized.

class_id is as it says, but starts at -1, and battle_commands is initialized to [-1, -1, -1, -1, -1, -1, -1]. If this was it's default I would be unsurprised. Setup is doing the wrong thing with these two.

@JenkinsRPG

This comment has been minimized.

JenkinsRPG commented Oct 18, 2017

Can one of the admins verify this patch?

@20kdc

This comment has been minimized.

Contributor

20kdc commented Oct 18, 2017

Huh. Do not merge yet - apparently a std::bad_alloc got thrown, somehow (EDIT: While checking Batter's status in OFF). Checking now if this is in base.
(EDIT2: Furthermore, this doesn't happen for the test case. Odd.)

@20kdc

This comment has been minimized.

Contributor

20kdc commented Oct 18, 2017

Ok, the issue appears to be fixed now, and I've run through OFF's first and second battles without incident, so it at least doesn't seem to break anything basic-yet-missable.

@EasyRPG EasyRPG deleted a comment from JenkinsRPG Oct 18, 2017

@EasyRPG EasyRPG deleted a comment from JenkinsRPG Oct 18, 2017

@EasyRPG EasyRPG deleted a comment from JenkinsRPG Oct 18, 2017

@EasyRPG EasyRPG deleted a comment from JenkinsRPG Oct 18, 2017

@EasyRPG EasyRPG deleted a comment from JenkinsRPG Oct 18, 2017

@carstene1ns

This comment has been minimized.

Member

carstene1ns commented Oct 18, 2017

jenkins: ok to test

Seems like each pull request builder job reacts to the hook now... hnnng.

@Ghabry

This comment has been minimized.

Member

Ghabry commented Oct 18, 2017

And looks like github logs now when a comment was deleted. How stupid is that, I always delete my "Jenkyns: Test this pls" comments to not pollute the PR m(

}
void Game_Actor::SetClass(int _class_id) {
GetData().class_id = _class_id;
GetData().changed_class = _class_id > 0;
GetData().changed_class = true; // Any change counts as a battle commands change.

This comment has been minimized.

@Ghabry

Ghabry Oct 18, 2017

Member

Isn't this value reset to false when the passed class_id is 0?

// when the class was changed by the ChangeClass event, otherwise it uses
// the normal actor attributes.
int n = GetData().changed_class && GetData().class_id > 0
int n = GetData().class_id > 0

This comment has been minimized.

@Ghabry

Ghabry Oct 18, 2017

Member

So you want to tell us that the bug is liblcf which copies the class_id from Actor to SaveActor?

@Ghabry Ghabry added this to the 0.5.4 milestone Oct 18, 2017

@Ghabry

This comment has been minimized.

Member

Ghabry commented Oct 18, 2017

Could you split the PR so that the first 3 commits are in a different PR? Because they are probably easier to review than the ClassChange stuff.

@20kdc 20kdc changed the title from work on #1268, #1198, two undocumented GetActors uses, Bitmap::Create refactor to Class fixes Oct 18, 2017

@20kdc 20kdc changed the title from Class fixes to Class / BattleCommands -related fixes Oct 18, 2017

@20kdc

This comment has been minimized.

Contributor

20kdc commented Oct 18, 2017

Updated the PR accordingly.

@Ghabry

This comment has been minimized.

Member

Ghabry commented Oct 18, 2017

Based on these observations I agree that "class_changed" is bad naming. But by now I would stick to this name until we are 99% sure that nothing except battle commands toggles it. Otherwise we have to rename the field twice 👎

@20kdc

This comment has been minimized.

Contributor

20kdc commented Oct 19, 2017

Well, class change enables it, but class change modifies battle commands.

@Ghabry Ghabry added the Needs Rebase label Oct 30, 2017

@Ghabry

This comment has been minimized.

Member

Ghabry commented Oct 30, 2017

needs a rebase

@Ghabry

This comment has been minimized.

Member

Ghabry commented Oct 31, 2017

Solution without PadBattleCommands and some C++ algorithms:

diff --git a/src/game_actor.cpp b/src/game_actor.cpp
index 1a28287c..511200b7 100644
--- a/src/game_actor.cpp
+++ b/src/game_actor.cpp
@@ -48,8 +48,7 @@ Game_Actor::Game_Actor(int actor_id) :
 	// - LSD readout is showing this happens.
 	GetData().class_id = -1;
 	GetData().battle_commands.clear();
-	for (int i = 0; i < 7; i++)
-		GetData().battle_commands.push_back(-1);
+	GetData().battle_commands.resize(7, -1);
 
 	Setup();
 }
@@ -933,57 +932,43 @@ void Game_Actor::SetSprite(const std::string &file, int index, bool transparent)
 	GetData().sprite_flags = transparent ? 3 : 0;
 }
 
-void Game_Actor::PadBattleCommandsArray(bool include7) {
-	std::vector<uint32_t> ncommands;
-	for (size_t i = 0; i < GetData().battle_commands.size(); ++i) {
-		int32_t command_index = (int32_t) (GetData().battle_commands[i]);
-		if (command_index != -1) {
-			if (command_index != 0) {
-				ncommands.push_back((uint32_t) command_index);
-			}
-		}
-	}
-	if (include7) {
-		if (ncommands.size() < 7) {
-			ncommands.push_back(0);
-			for (int i = ncommands.size(); i < 7; i++) {
-				ncommands.push_back((uint32_t) -1);
-			}
-		}
-	}
-	GetData().battle_commands = ncommands;
-}
-
 void Game_Actor::ChangeBattleCommands(bool add, int id) {
-	// If changing battle commands, *that* is when RPG_RT will replace the -1 list with a 'true' list.
+	auto& cmds = GetData().battle_commands;
+
+	// If changing battle commands, that is when RPG_RT will replace the -1 list with a 'true' list.
 	// Fetch original command array.
 	if (!GetData().changed_class) {
-		GetData().battle_commands = Data::actors[GetId() - 1].battle_commands;
+		cmds = Data::actors[GetId() - 1].battle_commands;
 		GetData().changed_class = true;
 	}
+
+	// The battle commands array always has a size of 7 padded with -1. The last element before the padding is 0 which
+	// stands for the Row command
 	if (add) {
-		if (std::find(GetData().battle_commands.begin(), GetData().battle_commands.end(), id)
-			== GetData().battle_commands.end()) {
-			PadBattleCommandsArray(false);
-			GetData().battle_commands.push_back(id);
-			std::sort(GetData().battle_commands.begin(), GetData().battle_commands.end());
-			PadBattleCommandsArray(true);
-		}
-	}
-	else if (id == 0) {
-		GetData().battle_commands.clear();
-		PadBattleCommandsArray(true);
-	}
-	else {
-		PadBattleCommandsArray(false);
-		{
+		if (std::find(cmds.begin(), cmds.end(), id)	== cmds.end()) {
+			std::vector<uint32_t> new_cmds;
+			std::copy_if(cmds.begin(), cmds.end(),
+						 std::back_inserter(new_cmds), [](uint32_t i) { return i != 0 && i != -1; });
+			// Needs space for at least 2 more commands (new command and row)
+			if (new_cmds.size() >= 6) {
+				return;
+			}
+			new_cmds.push_back(id);
+			std::sort(new_cmds.begin(), new_cmds.end());
+			new_cmds.push_back(0);
+			cmds = new_cmds;
+		}
+	} else if (id == 0) {
+		cmds.clear();
+		cmds.push_back(0);
+	} else {
 		std::vector<uint32_t>::iterator it;
-			it = std::find(GetData().battle_commands.begin(), GetData().battle_commands.end(), id);
-			if (it != GetData().battle_commands.end())
-				GetData().battle_commands.erase(it);
-		}
-		PadBattleCommandsArray(true);
+		it = std::find(cmds.begin(), cmds.end(), id);
+		if (it != cmds.end())
+			cmds.erase(it);
 	}
+
+	cmds.resize(7, -1);
 }
 
 const std::vector<const RPG::BattleCommand*> Game_Actor::GetBattleCommands() const {
diff --git a/src/game_actor.h b/src/game_actor.h
index 7c876026..c571530c 100644
--- a/src/game_actor.h
+++ b/src/game_actor.h
@@ -754,12 +754,6 @@ private:
 	 */
 	void RemoveInvalidEquipment();
 
-	/**
-	 * Either pads GetData().battle_commands to 7 entries including the 0 and -1 used by RPG_RT,
-	 * or cuts them off for use while adjusting the array.
-	 */
-	void PadBattleCommandsArray(bool include7);
-
 	int actor_id;
 	std::vector<int> exp_list;
 };
@Ghabry

This comment has been minimized.

Member

Ghabry commented Oct 31, 2017

The type of "battle_commands" must be changed in liblcf from std::vector<uint32_t> to std::vector<int32_t>. Otherwise this generates warnings when inserting the -1.
Problem: This is a quite complex change because we don't have templates in reader_struct for this >.>

EDIT: DONE

GetData().class_id = -1;
GetData().battle_commands.clear();
for (int i = 0; i < 7; i++)
GetData().battle_commands.push_back(-1);

This comment has been minimized.

@Ghabry

Ghabry Nov 5, 2017

Member

48 to 52 can be removed as liblcf handles the correct initialisation now (or soon)

@20kdc

This comment has been minimized.

Contributor

20kdc commented Nov 7, 2017

Will rebase and apply diff tagging Ghabry as author, I'm a bit worried about the PR being dependent on a liblcf PR, but will go ahead regardless (can always undo)

@carstene1ns

This comment has been minimized.

Member

carstene1ns commented Nov 7, 2017

This is fine. We always keep the liblcf PR open until a proper fix for Player is available.

20kdc and others added some commits Oct 17, 2017

Hopefully make handling of classes, class changes, and battle command…
… changes better. [P]

This fixes handling of classes, with the new consideration that "changed_class" is actually more "changed_battle_commands", and what this implies.
Specifically that any code relying on changed_class's old meaning is based on a lie.
It also fixes handling of battle command changes, now that '-1 in the array means check LDB' is pretty clearly false

(But '-1 as a LSD class_id means check LDB' is true, which suggests possible miscommunication somewhere.)
More updates to catch up to liblcf PR
BattleCommands uint32_t -> int32_t in game_actor.cpp,
removing the workaround reset code
@Ghabry

This comment has been minimized.

Member

Ghabry commented Nov 8, 2017

@20kdc please change in my liblcf branch battle_commands.resize(-1, 7); to battle_commands.resize(7, -1);, messed up the order.

Fixing this later when I'm at my pc

Ghabry referenced this pull request in Ghabry/easyrpg-liblcf Nov 8, 2017

@Ghabry

This comment has been minimized.

Member

Ghabry commented Nov 9, 2017

@20kdc upon further investigation I'm 99% certain that your observation is correct and that the variable is indeed "changed_battle_commands". Good job 👍

@Ghabry Ghabry removed the Needs Rebase label Nov 9, 2017

@Ghabry

This comment has been minimized.

Member

Ghabry commented Nov 9, 2017

Only Linux and Web use liblcf-pr jobs. Build failure is normal.
Looks good to me, will do some final test then this can be merged 👍

@carstene1ns

This comment has been minimized.

Member

carstene1ns commented Nov 10, 2017

jenkins: test this please (fingers crossed)

@carstene1ns

This comment has been minimized.

Member

carstene1ns commented Nov 10, 2017

  ..\..\src\game_actor.cpp(946): error C2039: 'back_inserter': is not a member of 'std' [C:\Jenkins\workspace\player-win32-pr\builds\vs2015\PlayerLib.vcxproj]
  ..\..\src\game_actor.cpp(946): error C3861: 'back_inserter': identifier not found [C:\Jenkins\workspace\player-win32-pr\builds\vs2015\PlayerLib.vcxproj]
@Ghabry

This comment has been minimized.

Member

Ghabry commented Nov 10, 2017

Maybe can besolved with header <iterator>

@Ghabry

This comment has been minimized.

Member

Ghabry commented Nov 12, 2017

When the battle command array is e.g. "2 3 3 0" and you remove "3" it is only removed once. But this array layout is impossible to get without an external edit, so that behaviour is okay for me.

The NClass test case crashes because a new skill has an out of bounds skill ID... My sanitity check branch will handle this.

@Ghabry

Ghabry approved these changes Nov 12, 2017

@carstene1ns carstene1ns merged commit d1e6e22 into EasyRPG:master Nov 13, 2017

6 checks passed

Android (armeabi-v7a) Build finished.
Details
GNU/Linux Build finished.
Details
OSX Build finished.
Details
Windows (x64) Build finished.
Details
Windows (x86) Build finished.
Details
web Build finished.
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment