chore: 105009: code cleanup - update some instances of objGetProperties to new obj@ alias

[FlufflesTheMicrosaur]

Also updates a bunch of old objGetProperty calls to obj@

[gcabbage]

Aren't properties supposed to be replacing the older objData and equivalents? If you've decided that properties just don't work and want to revert back to objData then we need a clear statement in the style guide or similar

I can see why you might want to have objData taking priority certain situations at the moment, but I think it could be very confusing in practice. In all(?) other cases where something might be either a property or data it is the legacy code that used objData, and the property will take priority if it is set. I'd be worried you might get a situation where objData is now overriding properties which are overriding objData...

Is it possible to achieve the same just using properties?

• With xml-defined properties you can change them to a modifiable form (Global or Data) if you want scripts to be able to change them
• With engine-defined properties you could add code to allow *Set@ to modify them if appropriate
  • hmm. do xml-defined properties take priority over engine-defined ones if you use the same name?
• This would be more limited in that the end-user TLisp script would only be able to modify properties we allow them to modify (but that might be safer anyway!)

[FlufflesTheMicrosaur]

The ability for external code to inject data from the side is mandatory for chronicles, and not all objects are going to have awareness of this.
Additionally, some objects define non-modifiable properties which, as it turns out, do actually need to be modified/overridden externally for Chronicles compatibility purposes.

Because typData/objData is 100% flexible, and is immune to certain problems with accessing it during onGlobalTypesInit (though that is less relevant to this specific change, as the player is not looking at dockscreens there), it is absolutely critical that we maintain it as an a last-ditch override for expansions and mods.

[FlufflesTheMicrosaur]

A more detailed response after testing and digging around in the codebase some more (pasted in from contributor chat on discord)

properties cannot retrieve data directly, you have to use 'data

this is done nowhere in the code codebase (all instances of 'data are accessing structs)

• as far as I can tell, its not that the rpg library reading properties is doing it to override the old *Data values, its just outright ignoring them
• if there IS a name conflict (which I dont think is the case), we can have the overrides use a different name (ex (cat key ".override")) but the ability to override is 100% mandatory for chronicles/sandbox library
• it SHOULD be a standard to avoid using <Definition>, <Constant>, or <Variant> except where absolutely necessary - doing things like storing military ID checks in a <Definition> or <Constant> should be considered an anti-pattern
• There are some extremely advanced legitimate uses for those, but I doubt that they would show up in places outside of custom module-loaders (ex, how Sandbox library manages its modules) or in cases where self-contained things are meant to be grouped together for ascension or deletion purposes to avoid loss of data or a memory leak.
• however that doesnt fix the problem of old mods and expansions that copy-pasted the anti-pattern containing core content, especially if they are using overwrites or overrides thus the ability to override using *Data is mandatory

[FlufflesTheMicrosaur]

I have made a ticket for converting the inappropriate use of <Constant> and <Definition>: https://ministry.kronosaur.com/record.hexm?id=105034 - it doesn't solve the problems that chronicles/sandbox run into (because they need to touch everything), but its not a pattern that should be encouraged/copy-pasted around regardless

[gcabbage]

Are you saying:

• The property system is broken, cannot be fixed, so we should move back to an objData based system?
• The property system (if restricted to <Global> and <Data>) is adequate for basic usage, but objData must be the primary system for chronicles / sandbox?
• The property system has some limitations, this is a TLisp suggestion to work around some of those limitations

If it’s one of the first two then fair enough...
However, until now I have been assuming that the property system is the new way to do things: it is relatively new, so hasn’t been used much outside the core code, and may need adjusting to meet the needs of modding.

For example:

1. If the main limitation is that you can’t create properties at runtime - i.e. (objSet@ obj ‘unknownKey 0) then:

• Setting an unknown key could fallback to objSetData (in the engine, not in TLisp)
• obj@ / typ@ will fallback to returning objData if the property does not exist

1b) If it if preferable to keep properties and data separate, then do we need another property type "runtime" which basically works like the current objData

2. If you basically want a *SetOverride@ function but are otherwise happy with properties could we do something like:

• obj@ / typ@ etc will return the (dynamic) object/type data if set, otherwise return property as normal
• objGetData is redundant for most use cases, other than checking if a property override is in place
• objSetData can then used to override any property (could alias it to objSetOverride@ etc)
• In this example we do not allow objSet@ to modify constants etc. so it is very clear from reading the code that we are overriding something. Also probably want behavior like (objSetOverride@ obj key Nil) to remove the override and restore the default value

properties cannot retrieve data directly, you have to use 'data

I don’t think anyone has requested this before (being able to access "old" objData via obj@)

As far as I remember the 'data property was implemented to meet a user request for a way to access all the data currently stored on an object. It was not required for core, and core code has no reason to use it.

[gcabbage]

it SHOULD be a standard to avoid using <Definition>, <Constant>, or <Variant> except where absolutely necessary - doing things like storing military ID checks in a <Definition> or <Constant> should be considered an anti-pattern

I would agree for the current use of properties on ships / stations - i.e. check military ID / check radioactive.

However, a lot (most?) of the use of properties has been in missions, RPG membership types. Here I think you might cause a lot of problems by recommending use of data or global.

e.g. there is very little you can safely do to modify the rankTables. You can't add extra levels without significant changes to the rest of the Type, so would have to write a complete Type override anyway. You could possibly adjust the xpNeeded for each level, but that doesn't seem very useful

With most missions I would not want the definitions / constants / variants to be easily modified by external code as it's likely to break the logic and may require modifying the language / events too. In this case it would be better to just create a new version of the mission (possibly inheriting from the first) which changes the script as required.

• We might want an engine property adding so you could disable certain missions (or allow the priority to be modified)

[FlufflesTheMicrosaur]

The property system is broken, cannot be fixed, so we should move back to an objData based system?

Its definitely broken, and some parts cant be fixed, but I dont think we should wholesale move back. I believe its fine to continue promoting the use of properties, with some exceptions (particularly for dynamic data in early-load code, where its likely not possible to change due to the order in which things need to be initialized)

The property system (if restricted to <Global> and <Data>) is adequate for basic usage, but objData must be the primary system for chronicles / sandbox?

Yes, specifically in that Chronicles/Sandbox have some early-load code that must use typData due to the aforementioned limitations, AND they need to override properties that aren't set, or are Constant/Definition. However the suggested override option which secretly uses typ/obj data behind the scenes is adequate for overriding properties that aren't set. I think if we transparently set the names to "core.properties.override.[name]" in typ/instance data it should be safe. We would need *Override@Keys though to retrieve those keys without having to specifically parse the data keys, but thats a 'nice to have' and not immediately needed.

The ability to make unsafe changes using explicit override functionality (ex, typSetOverride@) I think is perfectly acceptable: we already have xml editing functions that can make major changes.

If you basically want a *SetOverride@ function but are otherwise happy with properties could we do something like

Outside of early-load code, pretty much this.

Early-load code still requires *data but I think thats acceptable, its not something most people need to worry about - its probably more niche than even type create and xml functions.

However, a lot (most?) of the use of properties has been in missions, RPG membership types. Here I think you might cause a lot of problems by recommending use of data or global.

Thats fine w/ an override function so that if you are confident you know what you are doing, you can override it.
The majority of <Properties> however are actually in items, ships, and stations though, and the vast majority of those are inappropriately set to <Definition> or <Constant> or <Variant>

[FlufflesTheMicrosaur]

I'd be worried you might get a situation where objData is now overriding properties which are overriding objData...

I think I finally get what you mean - is this about extremely old code that gets things like <StaticData> overridden by a <Global>/<Constant> or <Data>/<Variant> property of the same name, because they store their data in the same location? (ie, its all just the same typData staticData and objData structs behind it all)

I think forcing a special key on overrides will prevent any collision, as mentioned above

[FlufflesTheMicrosaur]

Working on a dedicated implementation for overrides here: #262

Going to convert this branch into just an update to the new property functions