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
891 stars 756 forks source link

deprecate UNITTYPE_ constants, switch to hardcoded BL_ constants #1677

Open Helianthella opened 7 years ago

Helianthella commented 7 years ago

For the sake of simplicity I believe we should deprecate the UNITTYPE_ constants so we only have the BL_ constants.

Helianthella commented 6 years ago

This will require making buildins that use UNITDATA_ to check the type of the argument (constant or integer) and then check that the constant begins with BL_, else fallback to legacy behaviour for backward-compatibility

AnnieRuru commented 5 years ago

https://github.com/rathena/rathena/commit/26720f041a3cd0edbaa975bfc70345a30e9bf706

meko @Helianthella , let's do this again ? I saw this https://github.com/HerculesWS/Hercules/pull/2196

remember the point of having UNITTYPE_ constants on getmapxy is because we can simple use getunittype on the getmapxy script command https://annieruru.blogspot.com/2019/01/the-reason-why-getmapxy-uses-unittype.html

I think all we have to do change the number value away from the getmapxy, then use the value of BL instead of UNITTYPE means we are going to break very old scripts that doesn't use constants

    comment__: "unit types"
    UNITTYPE_PC: {
        Value: BL_PC
        Deprecated: true
    }
    UNITTYPE_NPC: {
        Value: BL_NPC
        Deprecated: true
    }
    UNITTYPE_PET: {
        Value: BL_PET
        Deprecated: true
    }
    UNITTYPE_MOB: {
        Value: BL_MOB
        Deprecated: true
    }
    UNITTYPE_HOM: {
        Value: BL_HOM
        Deprecated: true
    }
    UNITTYPE_MER: {
        Value: BL_MER
        Deprecated: true
    }
    UNITTYPE_ELEM: {
        Value: BL_ELEM
        Deprecated: true
    }
AnnieRuru commented 5 years ago

https://rathena.org/board/topic/111451-quests-games-event-manager/?do=findComment&comment=359018 yup rathena breaks legacy script so do the same as them ? @Helianthella ?

Helianthella commented 5 years ago

sounds good but it would still be nice to have script directives like how gcc/clang use preprocessor directives so we could put the script engine version on top of scripts and selectively enable/diable features

#version 2

ra_in01,384,246,3   script  Vincent#ra_in01 1_M_01,{
    if (BaseLevel < 60) {
        mes "[Vincent]";
        mes "You're inside Sir Zhed's";
        mes "looking for new employees,";
...

kind of what I wanted to do here and here and I still have the branch locally where I started the work on adding directives to the script engine

we'd just bump the script engine version whenever we make breaking changes and in the script documentation we could put use version >=X to use feature Y

AnnieRuru commented 5 years ago

we'd just bump the script engine version whenever we make breaking changes and in the script documentation we could put use version >=X to use feature Y

that sounds like a bad idea to me ... we should always make scripts backward compatible ... using a version > X, we don't have a clear definition when to increase the version anyway

this actually reminds me long ago I talked with Haru, that we should have a server revision date in constant, just like PACKETVER what I proposed was, have the hercules bot automatically #define SERVER_DATE 20190323 in mmo.h but Haru dislike the idea having the hercules bot to spam the commits so this never happens

better be like

#if SERVER_DATE >= 20190323

ra_in01,384,246,3   script  Vincent#ra_in01 1_M_01,{
    if (BaseLevel < 60) {
        mes "[Vincent]";
        mes "You're inside Sir Zhed's";
        mes "looking for new employees,";
...