About the r8145/r8146 Commit

[sofax222]

To fix the sub for the 0291/"message_string" packet is a good idea.
But I think it will be better that using "parseROLUT" rather than "parseArrayFile".
That is it should include the Msg ID in the msgstringtable.txt file.
And make the fomate like "nnnn#xxxxxxx#".
The "nnnnn" is Msg ID, and the "xxxxxxx" is the message string.

[EternalHarvest]

http://openkore.svn.sourceforge.net/vie ... threv=8145

Any change in $args would mess with additional packet handlers, and here it incremented twice which probably neither correct nor needed. I'd make such @msgTable so message ID in packet can be used as is (for example, shift @msgTable in onLoaded option of addTableFile).

[iMikeLance]

Like this http://pastebin.com/hm8aMwDW ?

[EternalHarvest]

Yes.

You can just inline that sub like this:

Code: Select all

onLoaded => sub { shift @msgTable }

Also, calling T like this is useless:

Code: Select all

T($anyVariable."\n")

Just skip it in this case.

T and TF should only be used when there's any real "text" in source code (not in data) to translate, and all variable data should be passed in additional arguments to TF. Refer to gettext docs if you need details on that.

[sofax222]

I means use Hash is rather than Array to keep the message strings from msgstringtable.txt file.
Using Array, the order and number of lines must be keep well, because the index of the Array is the Msg ID.
Using Hash, the order and number of lines could be changed, because them are in key to value pairs.

[sofax222]

And I think the "sub message_string" in ServerType0.pm should be changed to:

Code: Select all

sub message_string {
	my ($self, $args) = @_;

	if (@msgTable[$args->{msg_id}++]) { # show message from msgstringtable
		warning T(@msgTable[$args->{msg_id}++]."\n");
		if ($args->{msg_id} >= 0x04F2 && $args->{msg_id} <= 0x04F5) {
			$self->mercenary_off ();
		}		
	} else {
		if ($args->{msg_id} == 0x04F2) {
			message T("Mercenary soldier's duty hour is over.\n"), "info";
			$self->mercenary_off ();
	
		} elsif ($args->{msg_id} == 0x04F3) {
			message T("Your mercenary soldier has been killed.\n"), "info";
			$self->mercenary_off ();
	
		} elsif ($args->{msg_id} == 0x04F4) {
			message T("Your mercenary soldier has been fired.\n"), "info";
			$self->mercenary_off ();
	
		} elsif ($args->{msg_id} == 0x04F5) {
			message T("Your mercenary soldier has ran away.\n"), "info";
			$self->mercenary_off ();
			
		} elsif ($args->{msg_id} ==	0x054D) {
			message T("View player equip request denied.\n"), "info";
	
		} elsif ($args->{msg_id} == 0x06AF) {
			message T("You need to be at least base level 10 to send private messages.\n"), "info";
	
		} else {
			warning TF("msg_id: %s gave unknown results in: %s\n", $args->{msg_id}, $self->{packet_list}{$args->{switch}}->[0]);
		}
	}
}

[EternalHarvest]

Using Array, the order and number of lines must be keep well, because the index of the Array is the Msg ID.

Well, original msgstringtable is already like this, and there's no need to change it.

And I think the "sub message_string" in ServerType0.pm should be changed to:

Well, we already have our own strings here, it won't help with anything. Actually, message_string uses not so many strings from msgstringtable, they could just all be put here so we know what kind of events message_string may indicate. As a debug measure usage of msgstringtable is good, but that's all.

[iMikeLance]

Yes.

You can just inline that sub like this:

Code: Select all

onLoaded => sub { shift @msgTable }

Also, calling T like this is useless:

Code: Select all

T($anyVariable."\n")

Just skip it in this case.

T and TF should only be used when there's any real "text" in source code (not in data) to translate, and all variable data should be passed in additional arguments to TF. Refer to gettext docs if you need details on that.

Sorry for this warning T(, it was just a distraction

I means use Hash is rather than Array to keep the message strings from msgstringtable.txt file.
Using Array, the order and number of lines must be keep well, because the index of the Array is the Msg ID.
Using Hash, the order and number of lines could be changed, because them are in key to value pairs.

Thats fine, but if you change anything in msgstringtable you lose the benefits of the file being ready to use. Using an array you just need to extract the file from the GRF and insert it in the proper server's table. With a hash we would just change it from the actual format to anything like this:

(currentline +1)#(string)

I think that's pretty redundant. Unless gravity changes msgstringtable, but then we can think of something about it.

[Technology]

In the following line, T has no use:

Code: Select all

warning T(@msgTable[$args->{msg_id}++]."\n");

(Only staticly known strings can be translated by gettext.)

Also, why is it a warning? I'm convinced it should be debug.
This is what I and Eternal agreed upon on irc:

As a debug measure usage of msgstringtable is good, but that's all.

Meaning, we should probably not rely on it all too much.



Btw, with commit r8149 ST0 & kRO diverged again, this packethander can easily be refactored to receive.pm, so the ST's converge there.


Also I don't like that there are now 2 codepaths with duplicated mercenary logic there (msgstringtable.txt is loaded AND the ID can be found OR not), those code paths are prone to diverging and can thus cause bugs that are hard to find.

Ideally, we want to move all environment and AI stuff out of the servertypes.
We want to be able to attach not 1 but many packet-callback-handlers to a given received packet. (now we only have the message_string callbackhandler)
Brace yourselves, PSEUDOCODE...
$message_string_packet->addEventListener($callbackhandler-that-checks-for-mercenary-logic-stuff-and-takes-action);
$message_string_packet->addEventListener($callbackhandler-that-displays-a-message);
$message_string_packet->addEventListener($callbackhandler-that-checks-other-stuff-and-takes-action);