alexbatalov / fallout2-ce

Fallout 2 for modern operating systems
Other
1.74k stars 119 forks source link

Function/variable/enum symbols dont match known original symbols #290

Open phobos2077 opened 1 year ago

phobos2077 commented 1 year ago

This seems to be the case for a subset of functions, but there are a lot of them with names that look like camelCased renames of original.

I believe that for code conservation and easy of use, it's better to revert to original names. These are known from debug symbols present in original mapper2.exe, the community used these names for 2 decades. Having the expected names for functions will reduce some cognitive load when navigating around the code for people with preexisting knowledge or when comparing with IDA database, etc.

Since there are many engine remakes, but this is the only project that can be considered "engine restoration", it would be great to strive to get code to match the original as much as possible, given known naming. Apart from symbols in mapper EXE, we got script headers. Scripts in general match very closely to the engine, so it's a safe bet to assume all ENUM's etc present in released script sources (they came along with mapper) will be the same as enums in the original (yet unreleased) source code.

phobos2077 commented 3 months ago

For reference, you can find script headers here: https://github.com/BGforgeNet/Fallout2_Unofficial_Patch/blob/master/scripts_src/headers/define.h (mostly original, maintained for compatibility with modern preprocessors)

I think it is best for easier modding experience to use the same symbols in engine as we do in scripts.

phobos2077 commented 3 months ago

Counter-arguments:

JanSimek commented 3 months ago

My counter-argument is that keeping the original names is useful only until Sfall implementation is finished. However, I do not have a strong preference either way.

It would be great if at least a single case style was enforced because this is a mess:

    interpreterRegisterOpcode(0x821D, opGetMouseY);
    interpreterRegisterOpcode(0x821E, op_get_mouse_buttons);
    interpreterRegisterOpcode(0x8220, opGetScreenWidth);
    interpreterRegisterOpcode(0x8221, opGetScreenHeight);
    interpreterRegisterOpcode(0x8224, op_create_message_window);
    interpreterRegisterOpcode(0x8228, op_get_attack_type);
    interpreterRegisterOpcode(0x8229, op_force_encounter_with_flags);
    interpreterRegisterOpcode(0x822D, opCreateArray);
    interpreterRegisterOpcode(0x822E, opSetArray);
phobos2077 commented 3 months ago

My counter-argument is that keeping the original names is useful only until Sfall implementation is finished.

But, I don't think Sfall implementation will ever be finished. There's just so many features and more are added every year. It will take a lot of time and effort for general public to start using CE in favor of sfall. Right now it is really bare bones. People will still use sfall code and IDA DB for reference, so there's a good reason to stay consistent. Renaming functions with known original names only because you're not used to this style serve no purpose.

phobos2077 commented 3 months ago

For reference, a list of all(?) vanilla function names: https://raw.githubusercontent.com/sfall-team/sfall/master/sfall/FalloutEngine/FunctionOffsets_def.h

Actually, quite a lot of functions use camelCase. A few even PascalCase, it's a mess. I guess I worked mostly with parts of code that use snake_case and got used to it.