RFC: Getter/Setter Design for liblcf vNext

[Ghabry]

• Emit reflection.json file through jinja2 template
• Implement parsing of all field types (e.g. EventCommand not supported yet)
• Remove unused fields + reader struct
• JSON read/write
• Savegame write
• Readd XML support (read)
• Readd XML support (write)

Because lcf vNext will replace the fields with Getters/Setters to gain more flexibility (this allows changing the data structure in the back, I plan JSON).

I will do an in-between step to make transition easier: The fields will stay and I emit the planned Getters/Setters. This allows an easier migration from old API to new API and better regression testing.

My ideas, lets use parts of Actor as an example:

// Functions use UpperCamelCase (PascalCase) as Player does

/* POD (plain old data) is transformed to: */
TYPE GetNAME() const;
void SetNAME(TYPE NAME);

// Examples:
int ID ->
 int GetId() const;
 void SetId(int Id);

int character_index ->
 int GetCharacterIndex() const;
 void SetCharacterIndex(int character_index);

/* bool is transformed to: */
bool IsNAME() const;
void SetNAME(bool NAME);

bool lock_equipment ->
 //  Consider renaming some fields and provide some aliasing via an extra csv
 bool IsLockEquipment() const; // Bad grammar, better: IsEquipmentLocked
 void SetLockEquipment(bool lock_equipment);

/* std::string */
// Using const char* because RapidJSON I want to use uses char* and this avoid copying
// C++17 has std::string_view for using an existing buffer for string...
const char* GetNAME() const;
void SetNAME(const char* NAME) const;
// C++ standard says binding a temporary to a const-ref is valid: https://stackoverflow.com/questions/11560339/
// Will create a copy everytime it is called
const std::string& GetNAME() const;
void SetNAME(const std::string& NAME);

Example: Replace NAME with Name ;)

/* Other types */
const TYPE& GetNAME() const;
// Providing non-const getter otherwise you can't modify members
TYPE& GetNAME();
// Actually the same as "GetNAME() = new_obj" but looks ugly
void SetNAME(const TYPE& NAME);

Example:
const Parameters& GetParameters() const;
Parameters& GetParameters();
void SetParameters(const Parameters& Parameters);

/* Vector type */
const std::vector<TYPE>& GetNAME() const;
std::vector<TYPE>& GetNAME();
// Don't think it makes sense to provide a Setter for a list, you can operator on the list obtained throug Get

const std::vector<uint8_t>& GetStateRanks() const;
std::vector<uint8_t>& GetStateRanks();

/* Extra functionality */
// Because the structs.csv contains more metadata I could emit extra functions, e.g.
// For fields with type Ref<TYPE>, here for class_id:

int class_id ->
 int GetClassId() const;
 void SetClassId(int character_index);
 // Bonus. * because nullptr is required
 const RPG::Class* GetClassInstance() const;
 RPG::Class* GetClassInstance();
 // Dangerous. Needs sanity check: assert if class is not in Data::classes
 // Maybe won't be implemented, deciding this later.
 void SetClassInstance(const RPG::Class& class);

// For fields with type Vector<Ref<>>
std::vector<uint32_t> battle_commands; ->
 const LcfVector<uint32_t>& GetBattleCommands() const;
 LcfVector<uint32_t>& GetBattleCommands();
 // Bonus. * because nullptr is required
 const LcfVector<BattleCommand*>& GetBattleCommandInstances() const;
 // No non-const Get or Set semantics by now, tricky to get this right

[Ghabry]

@fmatthew5876
I would like your opinion here for discussing to make the API "stable":
What do you think about replacing direct field access with (inline) getters/setters. Imo this would give more flexibility as the way all lcf objects are stored could be abstracted away behind this get/set interface and modified without breaking code depending on liblcf. (was thinking about something like "std::any" but is likely not very performance friendly).

This get/set API would also allow e.g. to abstract away "lazy loading" of very memory heavy chunks like the battle system chunks (load on first get-call) and the parsing style could be configured via a "LoadOpt (ParseFull/ParseLazy)"

The JSON-idea was already evaluated and the result was not worth it. I already got this fully working, but it was really inefficient and memory wasting :).

Rewriting whole Player & Editor code is no huge problem, I have a tool which does this automatically by parsing clang compiler error messages (dirty but works for 99% of the code) xD

[mateofio]

@fmatthew5876
I would like your opinion here for discussing to make the API "stable":
What do you think about replacing direct field access with (inline) getters/setters. Imo this would give more flexibility as the way all lcf objects are stored could be abstracted away behind this get/set interface and modified without breaking code depending on liblcf.

Getters and setters are fine. For some of the optimization ideas like #256 , the data would litterally be a huge binary blob and we would use getters to return references for single members and span for arrays.

• https://github.com/Microsoft/GSL/blob/master/include/gsl/span

(was thinking about something like "std::any" but is likely not very performance friendly).

Stay away from std::any, it requires heap allocations for everything. Using it all over lcf could cause performance to tank and memory usage to surge. The RPG:: objects underpins everything and therefore should be performant.

This get/set API would also allow e.g. to abstract away "lazy loading" of very memory heavy chunks like the battle system chunks (load on first get-call) and the parsing style could be configured via a "LoadOpt (ParseFull/ParseLazy)"

Lazy loading battles is a great idea and would be enabled by getters. I'd like to see if we can solve this 8-10x memory overhead issue first. If we got the memory usage way down would we still need to worry about introducing the complexity of lazy load?

The JSON-idea was already evaluated and the result was not worth it. I already got this fully working, but it was really inefficient and memory wasting :).

Not sure what you mean here. If you want to use json instead of xml for on-disk format thats just as well as anything. If you're talking about some kind of dynamic in memory structure using JSON at runtime, performance would be awful and I don't see much benefit.

Rewriting whole Player & Editor code is no huge problem, I have a tool which does this automatically by parsing clang compiler error messages (dirty but works for 99% of the code) xD

I'd want to finish correctness (saving lsd with cleared map defaults) and then also performance optimizations before thinking about stabilization. Once stabilized big refactors will be harder to do and need careful planning and delays.

For a stable API we also need to consider:

• How to support custom RPG_RT patches in a compatible way?
• How to support custom Player patches/data?

[Ghabry]

I'd like to see if we can solve this 8-10x memory overhead issue first. If we got the memory usage way down would we still need to worry about introducing the complexity of lazy load

I agree, when you can get the footprint down far enough we won't need this.

using JSON at runtime, performance would be awful and I don't see much benefit.
That's exactly what I tried and it ran really bad ;)

How to support custom Player patches/data?

This is really a good point! Obviously our editor shall have more functionality added when 2k/2k3 stuff works.

Maybe you saw this "Custom data" (can't remember the name) button in the EasyRPG Editor Actor Tab. Guess the intention for this was that you can fill a table with custom values which are stored ... somewhere (in xml or json?). Together with new event commands this would be really flexible.

How to support custom RPG_RT patches in a compatible way?

Our current policy is: We support everything that can't have incompatible side-effects in unpatched games. For conflicting patches you will need a huge database that lists all games with patches that must be dynamically applied...

[Ghabry]

Thats already 6 years old. Won't happen anymore ^^'