JsonToObject now bugged for armour parts

[Proleric]

To Reproduce

• create two armour instances with different parts (let's call them "old" and "template" and use robe as an example)
• json jArmour = ObjectToJson(oOldArmour, TRUE);
• jArmour = GffReplaceByte(jArmour, "ArmorPart_Robe", GetItemAppearance(oTemplate, ITEM_APPR_TYPE_ARMOR_MODEL, ITEM_APPR_ARMOR_MODEL_ROBE));
• oNewArmour = JsonToObject(jArmour, GetLocation(oPC), oPC, TRUE);
• Notice that oNewArmour still has the old robe, not the new one

Specifics

• Windows 11
• 88.8193.36-13
• Steam formerly Beamdog
• No content overrides
• Regression? Probably the one that introduced armour parts over 255

If needed, describe the bug

This code was working perfectly, now it's broken. Using JsonDump, you can easily see why.

ObjectToJson now creares two fields for every armour part e.g. ArmorPart_Robe (byte) and xArmorPart_Robe (word) which contain the same number (I'm not using parts over 255).

GffReplaceByte correctly replaces ArmorPart_Robe, but xArmorPart_Robe still contains the old value, of course.

The bug is that JsonToObject uses xArmorPart_Robe.

Temporary workaround is to use GffReplaceWord to change xArmorPart_Robe.

[Proleric]

On reflection, it may well be that JsonToObject is working as designed, in which case the bug is at the point in the engine where it decides which model to display. It should only be using xArmorPart_Robe if it's greater than 254.

[mtijanic]

Ugh, I'm sorry that this wasn't spelled out more clearly, but there's a certain class of changes that NWN never guaranteed compatibility for. Few examples:

• Adding a new GFF field manually
• Adding a new column to stock 2DAs
• Adding a completely new 2DA
• Compiling scripts using a modified nwscript.nss

Mostly the issue is that the game might add a GFF/2DA/etc entry using the same name, but with different semantics. I believe the above are all well understood and accepted. However, semantics of existing fields/columns may also change provided that the game can maintain compatibility with existing content. In other words, content generated by version X of the game/toolset will work on versions >=X*.

In your example above, version X generates GFFs without xArmorPart_*** fields. Version X+1 generates them with that field. Version X+1 can read both types of GFFs, but what it cannot do is read a third type that is neither here nor there, which is what you've generated. There are infinitely many ways to generate invalid GFF files; you've stumbled upon one. This can happen with a lot of other GFF fields as well, and more cases will be added in future patches too.

At this point, I think the only remedy is to just explicitly spell out in JsonToObject() description, something like:

// It is only valid to pass in a json created by ObjectToJson() or TemplateToJson() by the game of a version older 
// or equal to the current version. Passing anything else, or modifying the json in any way is Undefined Behavior

I'm also willing to accept that adding JsonToObject() in the first place was a mistake, but removing it now is not realistic.

Leaving the bug open to track documentation changes.

* sometimes it also works on older versions, but this is not a given.

[mtijanic]

By the way, the "proper" way for a nwscript to modify a json object/template would be to have a whitelist of all the GFF fields that you understand at that point in time, and only write those fields. Effectively freezing the GFF format at version X. That way, newly added fields like xArmorPart_*** would be removed after ObjectToJson() and you'd revert back to the old behavior.

This is not entirely future proof either because the game could have just as easily not written the old ArmorPart_* fields at all (this was done as a courtesy to allow players to export their bic and load in older game versions). So the actually foolproof way would be to just construct a version X GFF from scratch, ensuring all the necessary fields are populated.

[Proleric]

I read that a few times to understand your reasoning, but I honestly think you have a bug that needs fixing here.

Like I said, it may well be that JsonToObject is working as designed.

The flaw is in the implementation of armour parts over 254. It's public knowledge that it wasn't tested in the toolset when first released. There must be an associated change to the engine code which decides which part to use when both ArmorPart_Robe and xArmorPart_Robe are specified, whose logic is equally flawed.

I'm concerned that your proposed clarification to the documentation of JsonToObject() is not going to deter builders from doing much as I did.

• I'm passing in a Json created by the current version
• I'm not modifying the Json in any way (other than values of fields that already existed in earlier versions - your whitelist)

Remember, builders have no way of knowing what fields you might introduce in future. Our reasonable expectation is that code written in version X-1 will continue to work in version X (backward compatibility).

If you really believe JsonToObject was a mistake, a better documentation change would be "unsupported - do not use"; but that's not realistic - builders have doubtless written a lot of "modify object" code by now. It would be disheartening if Json could only be used to expose fields to the toolset on a read-only basis.

You may or may not agree - but set that aside for a moment.

This probably isn't about JsonToObject - it's a flaw in how armour parts >254 were implemented, given that ArmorPart_Robe was already open to builder modification.

[mtijanic]

This probably isn't about JsonToObject - it's a flaw in how armour parts >254 were implemented, given that ArmorPart_Robe was already open to builder modification.

No. It may not have been well documented in the final patch notes (preview had more info), but the logic of it is Working As Intended. In short:

• Game reads xArmorPart_XXX first, always. If it is found, that is used.
• If xArmorPart_XXX is not found, it uses the old ArmorPart_XXX
• When saving to GFF (save game, serializing to DB, ObjectToJson, etc), it will always write both xArmorPart_XXX and ArmorPart_XXX
  • This is done specifically so that your BIC from a newer game version can be loaded into the older one
• If the armor part is over 255, it will write the full value to xArmorPart_XXX and zero to ArmorPart_XXX

All of this is functioning exactly as intended. Your scripts where written in a way that is not forward compatible and broke because of that. I am sorry about this, and the issue very clearly lies in the lack of documentation that led you to believe writing such scripts was safe/supported in the first place. This will get addressed.

However, there is no functional change planned, and there can be no possible change that fixes this, because (A) The issue is wider than just armor parts and applies to other GFF fields that have been added and not even documented (B) Any possible engine level 'fix' would likely end up breaking modules that implemented systems that did take into account the above xArmorPart behavior.

I'm not modifying the Json in any way (other than values of fields that already existed in earlier versions - your whitelist)

No, this is not the whitelist I mentioned. Had you used the whitelist, after modifying ArmorPart_XXX you'd have removed all fields that you didn't know about at the time of writing that code, and that would have included xArmorPart_XXX. You simply cannot modify a single GFF field blindly and expect that it would leave the structure in a consistent state, in all future versions of the game.

As another example, in 1.69 modifying the PrimaryAbility column of classes.2da could change the primary spellcasting ability of a class. In EE, this was moved to a separate SpellCastingAbil column. In order to preserve custom content behavior, if a classes.2da was missing the new SpellCastingAbil column, the game would use whatever was in PrimaryAbility. However, you cannot in the latest EE version just take the stock shipping classes.2da and modify only the PrimaryAbility column and expect the same effect of that process as in 1.69 - you need to either modify or remove the new column as well.

(A plausible hack could be for the engine to keep track of all objects that were converted to json with ObjectToJson(), and then when someone does JsonToObject(), compare the difference and if someone changed ArmorPart_XXX but not xArmorPart_XXX then make an educated guess as to what they were actually trying to do and change the game behavior to accommodate that. But, such hacks are incredibly brittle and are likely to cause more issues than they solve)

[Proleric]

I'd welcome a second opinion on this.

When Json was introduced, no one said anything about whitelists. The functions and examples implied that the Json functions were there to be used as described, without qualification. Who could possibly know that using them was "not forward compatible"?

The principle is much deeper than this one example. There surely needs to be an onus on developers not to break existing modules.

[mtijanic]

I'd welcome a second opinion on this.

So would I. I'd also like to hear what you'd recommend as a general rule here (rather than "this one specific usecase should be supported"). Should GFF be both backwards and forwards compatible always? That would make a lot of improvements impossible, but would not trigger this issue.

[niv]

Addressed with documentation update to nwscript.nss.