HerculesWS / Hercules

Hercules is a collaborative software development project revolving around the creation of a robust massively multiplayer online role playing game (MMORPG) server package. Written in C, the program is very versatile and provides NPCs, warps and modifications. The project is jointly managed by a group of volunteers located around the world as well as a tremendous community providing QA and support. Hercules is a continuation of the original Athena project.
http://herc.ws
GNU General Public License v3.0
881 stars 755 forks source link

getvariableofnpc is messy, why not .'npc'.var ? #845

Open AnnieRuru opened 8 years ago

AnnieRuru commented 8 years ago

I'm sure many of you experienced using getvariableofnpc script command will make your script looks messy so, how about I suggest using .'npc'.var instead ?

set getvariableofnpc( "start", .capturedamount ), getvariableofnpc( "start", .capturedamount ) +1; // old
.'start'.capturedamount++; // new
set getvariableofnpc( "start", .capturedid ), getvariableofnpc( "start", capturedid ) | (1<<.@flagid); // old
.'start'.capturedid |= 1 << .@flagid; // new

looks cleaner, right ???

there are 3 reasons why it is .'npc'.var and not .'npc'var

reason no.1 .' <-- start the operation for getvariableofnpc 'npc' means another npc, and .var means a npc variable, and var means a player variable if it were look like .'npc'var, it somehow read as another npc's player variable ... ok even if you say it is possible, there is another reason

reason no.2 npc name can contain ' or '' single quotation or double quotation prontera,100,100,5 script AnnieRuru's ~Poring~ PORING,{ tested, it shows this npc name if it were .'npc'var kind of format, server will read s ~Poring~ and throw error

reason no.3 this format looks nice in Notepad++ :-)

PS: can I add tag: enhancement on this ? :D

Ancyker commented 8 years ago

Why not...

..NPC.var

Or something. The quotes just seem out of place

AnnieRuru commented 8 years ago

can also o.o but just a full stop symbol, if only npc names aren't so unique ...

prontera,155,185,5 script AnnieRuru's. ~Poring~ PORING,{} npc can shows the name ...

errr ... seriously need to think of a better format that npc name can throw error ...

MishimaHaruna commented 8 years ago

Let's stop this fancy prefixes thing, it's already overdue...

Before adding this, I'd like to clean up the variable naming syntax (prefixes/suffixes). It got out of hand several years ago, and the problem was never solved. Now it's biting us in the back, because player variables look the same as constants, with the side effect that we can't validate the constants at parse time. Considering that we don't want to change the way constants are named, we need to change the way variables are named.

The first condition I'd like to place is that all variables should start with the same symbol (at most, we could differentiate symbols when referring to arrays or scalars). The prefix could be a single $ or a single ., I don't care, but it should be something sane. If you like an object-oriented lookalike syntax (seeing the ..NPC.var suggestion above), I'd do something like:

$var        ->  local (scope) variable (current .@var) - those are the majority of variables, and a single-character prefix is a priority
$pc.var     ->  player reg (current var) - those are less common. Adding '$pc.' doesn't hurt
$pc.Zeny    ->  player params could even fit here, so that they don't conflict with constants.
$npc.var    ->  npc variable (current .var) - those are somewhat less common. '$npc.' is a bit lengthy, but unless there are better ideas, this is all I can suggest
$npc("npc name here").var  ->  getvariableofnpc, as per suggestion above (note that the double quotes are necessary, because NPC names can contain any symbol, including things that would conflict with operators)
$tmp.var    ->  player temp variable (current @var) - those have limited use
$acc.var    ->  login variable (current #var) - rarely used
$globalacc.var ->  global login variable (current ##var) - almost never used, long prefix doesn't matter
$inst.var   ->  instance variable (current 'var)
$map.var    ->  mapserver reg (current $var)
$tmpmap.var ->  temporary mapserver var (current @$var)

To make the change more reasonable, I'm thinking about making the engine backwards compatible with the old syntax (because people are lazy to rewrite their scripts right away) for, say, at least a year. The new syntax would be the default, and to use the old one you could specify something like #pragma syntax Athena-compatible at the beginning of the script. Of course, to use shorthands like the new getvariableofnpc thing, you'd need to use the new syntax in your script, because it didn't exist in the old one.

Emistry commented 8 years ago

No please, .'npc'.var it look ugly. Beside HOW you determine which NPC's variable is link to ?

EDIT: ops, accidentally clicked on "Close Issue" =='' my bad sry.

dastgirp commented 8 years ago
No please, .'npc'.var it look ugly. Beside HOW you determine which NPC's variable is link to ?

.'NPCNAME'.var is meant to be variable by annie's suggestion..

But I like the way Haru suggested about variables..

AnnieRuru commented 8 years ago

$tmpmap.var -> temporary mapserver var (current @$var) should be $@var =D

wah ... seriously, if you make this change, I'm sure almost everyone will go back to using rathena because they don't mind adjust source coding everytime, but they will mind update their 100++ custom scripts also, that also makes a lot of legacy type scripts (clucker, disguise event) broken they just rant "just use rathena, our script engine still supports all scripts"

$npc("npc name here").var -> getvariableofnpc, as per suggestion above damn ... I want this now ... my scripts will looks much cleaner if direct variable assignment is possible

actually. I'm guessing, if getvariableofnpc and getelementofarray is no longer needed, I'm quite sure set command can be removed entirely from emulator that's the last reason set command still exist PS: you didn't add getelementofarray in your deprecation of legacy script command list

MishimaHaruna commented 8 years ago

getelementofarray is still necessary when using getarg() to pass an array by reference to a function. We still need some changes in order to make that work (I have a suggestion for that, syntax wise)

When you say

also, that also makes a lot of legacy type scripts (clucker, disguise event) broken

That's not true. As I said above, I'd guarantee backwards compatibility for a quite long time (I said at least a year). The only change needed to load a legacy script, until the author finds the time to convert it (which means a very simple search and replace through the script, a matter of mere seconds) would be to add a #pragma line at the top of the file. Is that so unreasonable?

AnnieRuru commented 8 years ago

oops sry I misread that part, my bad

k-py commented 8 years ago

Since some important changes to the scripting engine are being discussed here, I would like to take the chance to leave this suggestion.

What about using a time-tested, widely adopted, highly expressive, blazing fast and easy to integrate language like Lua or Squirrel? Not taking into consideration scripts written with gotos, it might be possible to implement a reasonable converter to convert scripts from the current scripting language to a much more expressive language like Lua or Squirrel. Although the converter would take quite a bit of development work.

Lua: http://www.lua.org/pil/10.2.html Squirrel: http://squirrel-lang.org/#look

4144 commented 8 years ago

@k-py you forgot also angel script: http://www.angelcode.com/angelscript/

But all this languages have disadvantages if compare to current scripting lanaguge. Need many overhead code for implement existing script language features. For example you cant describe npc with one line, or use event labels inside functions. Also lua syntax a bit strange if compare with simple C like syntax. You can try search eathena fork based on lua. Code script not good for read and not noob friendly.

MishimaHaruna commented 8 years ago

If I were to pick a replacement language, I'd have gone for a sane, widely used language, such as ruby (1.8%), perl (2.2%), python (4.5%), not some exotic language with a very small market share and little support and resources (lua 0.6%; squirrel not in the top 100, angelscript not in the top 100). (note: figures from the TIOBE index, based on the most popular search engines)

I would've done that long ago if I didn't have to care about being backwards compatible or at least make it easy to convert the scripts with little more than a find and replace (assuming we find a way to sugarcoat them in order to make the NPC creation syntax not too ugly to read)

k-py commented 8 years ago

Python would be my top pick among those. I mainly suggested Lua due to its high performance when running under the LuaJIT implementation and the fact that it is designed from the ground up to be embedded.

I guessed the problem was keeping backward compatibility, but since some important backward incompatible changes were being discussed here, well, I had to try. It's just that I think that being able to write scripts in a feature-rich OO language (or at least, a language supporting OO programming, like Lua) could be really powerful and enable a whole lot of new features.

Is there any reason (besides the obvious development effort involved in doing anything else) for limiting the backward incompatible changes to little more than a find and replace? Writing a compiler back end to generate the code in the target language and piggybacking on the current code for the front end would be a viable way to convert existing scripts (if support for goto is dropped). This would be a nice project for someone taking a compilers course :).

EDIT: Possible syntax examples in Python:

@npc("Valkyrie#","valkyrie",48,86,4,CONST_4_F_VALKYRIE)
def script():
    mes("[Valkyrie]",
        "Welcome",
        "to Valhalla,",
        "the Hall of Honor.")
    close()
@mob("Phen",1158,3,"jawaii",207,290)
class MobController(Monster):

    def on_spawn(self):
        self.walk_to(self.x + 1, self.y)

    def on_change_target(self, target):
        if self.distance_to(target) < 7:
            self.use_skill(target, 5, 10)
duplicate("Valkyrie#", "Valkyrie#2","valkyrie",40,86,6,CONST_4_F_VALKYRIE)
@npc("Valkyrie#","valkyrie",48,86,4,CONST_4_F_VALKYRIE)
class NPC(Script):

    START_POINT = Location("valkyrie",123,90)
    EXIT_POINT = Location("valkyrie",100,100)
    TIME_LIMIT = 30 * 60 # seconds

    def on_click(self, pc):
        mes("[Valkyrie]",
            "Are you ready?",
            "If so, we can start.")
        close2()

        if pc.has_party():
            # Iterate over all party members.
            for pmember in pc.party.members:
                # Select party members that are on the same map as this npc.
                if pmember.map.name == self.map.name:
                    pmember.warp_to(self.START_POINT) # Warp player to START_POINT
                    pmember.var.valhalla_quest = 1 # Set quest variable

            # Start a timer and add a callback to be executed on expiration.
            Timer(self.TIME_LIMIT, self.on_timer_expiration).start()

    def on_timer_expiration(self):
        # Warp all players on this map to a single location.
        for pc in self.map.players:
            pc.warp_to(self.EXIT_POINT)
Helianthella commented 7 years ago

if we stick with the Athena language for a while I'd like to suggest a counter-proposal to https://github.com/HerculesWS/Hercules/issues/845#issuecomment-153665668

There would be two prefixes but $ would mean a permanent variable and @ would mean a variable stored in memory only. Thus you'd have:

@var // equivalent to .@var

$pc.var // equivalent to var
$pc(account id).var // equivalent to getvariableofpc(var, account id)
@pc.var // equivalent to @var
@pc(account id).var // equivalent to getvariableofpc(@var, account id)

$acc.var // equivalent to #var
$acc(account_id).var // equivalent to getvariableofpc(#var, account id)

$globalacc.var // equivalent to ##var
$globalacc(account_id).var // equivalent to getvariableofpc(##var, account id)

$param.zeny // param (not under the $pc. prefix, because we could use 'em for any unit)
$param(unit id).level

@npc.var // equivalent to .var
@npc("npc name").var // equivalent to getvariableofnpc(.var, "npc name")

@inst.var // equivalent to 'var
@inst(instance id).var // no equivalent

$map.var // equivalent to $var
@map.var // equivalent to $@var
MishimaHaruna commented 7 years ago

I like this idea, sounds pretty nice and clean. I also like how they would support accessing values from other objects by ID.

By using @ for the temporary variables, we'll lose the possibility to use it to refer to an array (perl-style), but that wouldn't be a problem. We can use a different syntax for copyarray (i.e. @foo[] = $bar[] or something similar)

4144 commented 7 years ago

I think prefixes for some vars bit ugly. Probably better use text names. One char prefixes better use only for pc and npc and map. In other cases they useless Also for constants:

const.var
Helianthella commented 7 years ago

If we want only one prefix we could do

no prefix would mean it's saved, the $ prefix would mean it's temporary, and @ could be used to reference an array

$var // equivalent to .@var

pc.var // equivalent to var
pc(account id).var // equivalent to getvariableofpc(var, account id)
$pc.var // equivalent to @var
$pc(account id).var // equivalent to getvariableofpc(@var, account id)

acc.var // equivalent to #var
acc(account_id).var // equivalent to getvariableofpc(#var, account id)

globalacc.var // equivalent to ##var
globalacc(account_id).var // equivalent to getvariableofpc(##var, account id)

param.zeny // param (not under the $pc. prefix, because we could use 'em for any unit)
param(unit id).level

$npc.var // equivalent to .var
$npc("npc name").var // equivalent to getvariableofnpc(.var, "npc name")

$inst.var // equivalent to 'var
$inst(instance id).var // no equivalent

map.var // equivalent to $var
$map.var // equivalent to $@var

const.foo // constant

or we could use the prefix for only when it would be ambiguous otherwise:

var // equivalent to .@var

pc.var // equivalent to var
pc(account id).var // equivalent to getvariableofpc(var, account id)
$pc.var // equivalent to @var
$pc(account id).var // equivalent to getvariableofpc(@var, account id)

acc.var // equivalent to #var
acc(account_id).var // equivalent to getvariableofpc(#var, account id)

globalacc.var // equivalent to ##var
globalacc(account_id).var // equivalent to getvariableofpc(##var, account id)

param.zeny // param (not under the $pc. prefix, because we could use 'em for any unit)
param(unit id).level

npc.var // equivalent to .var
npc("npc name").var // equivalent to getvariableofnpc(.var, "npc name")

inst.var // equivalent to 'var
inst(instance id).var // no equivalent

map.var // equivalent to $var
$map.var // equivalent to $@var

const.foo // constant

I still believe it would be better to have both @ and $ to make it explicit that a var is either temp or permanent. Although we could even do the following:

$var = retrieve("var", ACCOUNT_DB); // assign the temp `$var` variable to the value of `var` from the account db
$var++; // manipulate the temp variable
store($var, "var", ACCOUNT_DB); // store the temp `$var` variable as `var` in the account db

or we could do explicit declaration:

point $var to var from account; // point directly to the var in the database
$var = 5; // the database is now updated

// other examples:
point $var to scope; // scope variable
point $var to <var> from instance <id>; // instance var
point $var to <var> from npc "<name>"; // npc var
point $var to <var> from global scope; // global var

This way everything with $ is ALWAYS a temp scope variable and everything without $ is ALWAYS a constant.

4144 commented 7 years ago

You forgot constants. Constants is main issue why this vars need. Because constants conflicts with player vars.

Helianthella commented 7 years ago

@4144 edited now

4144 commented 7 years ago

Also this vars should be backward compatible with old vars. Because exists too many custom scripts. And no one will update this scripts. This mean atleast this one is wrong:

var // equivalent to .@var
Helianthella commented 7 years ago

@4144 we could make scripts specify the version on top, and if no version assume legacy.

#version 2
MishimaHaruna commented 7 years ago

My goals are:

The retrieve/store approach makes things much simpler (nothing is saved unless explicitly said), but it might be annoying for scripters. The good part is that it would discourage the use of permanent variables unless they are needed.