Armour editing via Json

Seems like there’s a significant bug in existing code since provision was made for armour parts over 255 (even if you’re not using parts in that range).

As expected, ObjectToJson now creates two fields for every armour part, e.g. ArmorPart_Robe (byte) and xArmorPart_Robe (word).

Presumably xArmorPart_Robe provides for the case that a value over 255 is required.

If you have existing code that changes ArmorPart_Robe using GffReplaceByte, obviously xArmorPart_Robe does not change.

The problem is that creating the new armour with JsonToObject picks up xArmorPart_Robe, so the armour does not change as intended.

Temporary workaround is to change xArmorPart_Robe using GffReplaceWord.

Reported on Github.

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 probably be using xArmorPart_Robe only if it’s greater than 254.

xArmorPart_Robe and ArmorPart_Robe both get deserialized to the same field when the creature is constructed, I guessing, just like the Wings_new that was added in 1.69. Something like the below.

  if (!archive.get_to("xArmorPart_Robe", self.armor.robe, false)) {
        byte old = 0;
        archive.get_to("ArmorPart_Robe", old, false);
        self.armor.robe = old;  // self.armor.robe is a word
    }

Ideally, the games’ GFF reader would have been improved to handle integer promotion and maybe a sane-ish default and warning on accidental demotion. And frankly, editing a serialization format whether it’s this or letoscript or whatever has always been not a great idea, since it was never designed to be edited in situ. The ship’s sailed so people just have to be mindful of this kind of stuff.

Well, to get this in proportion, at the moment it’s only armour part editing that’s broken.

We have a workaround, and most of the broken modules are likely still in development, since Json editing hasn’t been around that long.

So, the immediate issue is containable.

Having said that, I’m disappointed with the developer response to date.

When Json editing was introduced, to the best of my knowledge, no one said

It was natural to assume that Json was there to be used.

The new code for xArmorPart_* could have been specified in a way that supported existing scripts, but it was not, so, in a very narrow sense, we’re told it’s “working as designed”. Back in the day, we used to call this “designed to fail”.

I wouldn’t want to make a mountain out of a molehill - certainly not just over this issue.

What concerns me more is the mindset that breaking things is OK.

That is an absolutely unfair characterization of the development process. In fact, probably the most common complaint we get is that we spend way too much effort on maintaining compatibility. We had abandoned numerous enhancements because there was no reasonable way to make them without breaking hypothetical existing SP modules. While not officially documented (something I keep putting off, sorry), our compatibility guarantees are based on those of the Linux kernel which is the golden industry standard.

Our compatibility guarantees are orders of magnitude stronger than what Bioware had for the original patches, and stronger than 99+% of the games with modding support (I’m sure there’s some that have us beat, I’m just not aware of any).

All that said, there are things that simply can’t be supported while continuing the development of the game. The most obvious extreme case is that we don’t support “nwncx” and similar exe hacks, because that would mean that we couldn’t ever recompile the game at all. I doubt this is contentious.

Here are some other things that we don’t support:

  • Scripts compiled with a modified nwscript.nss or 3rd party compilers. I have actually seen in the wild NCS files that try to call a non-existent nwscript function and expect to be terminated when that happens. Since functions are identified by their ordinal number, simply adding new nwscript functions has broken those scripts and any modules that use them. However, we are not going to stop adding new functions just because of them, what they are doing is unsupported.

  • Adding new columns to engine 2DA files. We reserve the right to add new columns to any 2DA and assign certain engine behavior to those. We go well out of our way to make sure that if a module has modified those 2DAs somehow and the new columns are missing that existing-behavior-preserving defaults are used (this results in a lot of table hardcoding in the engine). However, if a module just added random columns of the same name but with different semantic meaning, it can have unintended consequences. We cannot detect this, so this is unsupported.

  • Similarly, we sometimes add new data files that the engine expects. This follows the established naming conventions, so for some of them, such as scripts and blueprints we use nw_ prefix, but 2DA, MDL, SHD and other file format don’t have a naming format. It is possible that one of those additions conflicts with what is in a module’s custom content and cause undefined behavior.

  • File formats are only forwards compatible - Newer game version can always read files produced by older game/toolset version. Going in the other direction is not supported. e.g. MDLs have been extended in ways that makes them not work on 1.69 and older EEs. This includes GFF file schemas. The game is only guaranteed to work with formats that were produced by official tools of version less-or-equal to the current one. If you were to manually build a GFF file with letoscript/nwn_gff/etc and produce an output that does not match output produced by any official tool at any point in time, then the behavior of parsing that output is not defined.

  • Shader includes are not stable. This one is an unfortunate side effect of exposing customizable shaders before all renderer work has finished, since shaders are tightly coupled to the renderer. We try to keep them compatible in various ways (including rewriting shaders on load), however sufficiently beneficial graphics improvements might result in breakage. If you want to avoid that, best to never #include any stock .shd files and just manually copy the bits you want to use.

  • The game’s internal resman order and layout is not stable. We could shuffle files around in the .bifs. Modules should not depend on a certain resource being at a specific location; instead they should just assume that it is somewhere that is available. This includes the behavior of ovr/, override/, development/ and similar global folders too - these are not for a module to use.

  • Various toggles in settings.tml are not stable. These are not part of the module API, and a module should not require a certain value for some setting to be playable (e.g. “auto fail on 1” may theoretically be removed).

  • Everything in ruleset.2da is not stable and can be removed or change meaning. This is documented plainly at the top of that file.

  • EDIT2: Anything performance-sensitive can change. For example, we don’t consider it a regression that loading screens now finish too fast for a player to see the image/text.

The above is a non-exhaustive list, but it should cover 90% of it. If there is a specific bit anywhere above that you consider unreasonable, I would like to hear why.


EDIT:
The above is about compatibility being broken on purpose. Bugs do unfortunately happen, but if it’s not one of the above, we will strive to fix them when reported. Unfortunately, what sometimes happens is that a regression stays unreported for a few years and new modules are made with the new behavior in mind, and then even reverting to the old one means breakage. Then, it’s a judgement call to pick the lesser of two evils.

Also, there may come a time when a specific thing will get broken on purpose, because the benefit of doing so is just too good to pass. In these cases, we will try to communicate it as clearly as possible, well in advance, and offer assistance in migrating the code to an equivalent solution that is supported.

1 Like

What concerns me more is the mindset that breaking things is OK.

As a module developer I share this concern.

Most of the things that have been added or changed of late have required me to spend time I should be working on content instead trying to fix things that broke because I updated the build; some examples to wit: lighting in several scenes changed (some broke entirely), SSAO started causing a relatively low poly area to crash (Black Stone Inn in the module, go and look, there’s nothing in there that should be a performance concern), and this has largely been in the pursuit of features I neither need nor would any of my players use (“toon” shader comes to mind).

If you think that’s harsh, I apologise.

I can see we’re never going to agree about this, but the bug I reported was created with the official tools.

In the developer bubble, there are evidently notions of how Json gff functions should be used, which are unknown in the Builder bubble. I understand that some further documentation is proposed, to bridge that gap, which is helpful.

In the meantime I hope this won’t deter builders from using the Json functions.

Why wouldn’t writing the xArmorParts_*** gff entries iff the value was over 255 or whatever, else write to ArmorParts_. If over 255 or whatever, write out xArmorParts_ and 0 to ArmorParts_, work as a solution to this problem? On read NWN:EE checks both anyway, so it won’t care.

1 Like

More precisely, when the engine decides which armour part to display, use Word iff exists and over 255, else use Byte.

For future field extensions, at least, it could be done that way, thereby maintaining backward compatibility with older scripts that set Byte (only) via the official functions.

Not sure why we can’t have the original variable and another of a larger size for the extension when neccesary; the executable should only be allocating the memory when the variable first used anyways unless we’re being extremely memory-unsafe.

I agree, there should be no Wings_new or xArmorParts_*** in the future. Only the same variable with a wider integer type. This is what I was referring to as integer promotion. I’m not so sure about demotion tho. Here is how I do it, but it’s just one way.

The GffReplace<some integer type> should also be able to write a value to equal or bigger (and of the same signedness?), maybe with a warning. There’s no harm in replacing the value of a word with the value of byte, computers do it all the time.

This all seems like it would allow forward compatibility, without adding to the burden on the dev team. Hopefully, this conversation sparks some ideas.

Nice idea, but gff format (which no one wants to mess with) regards byte and word versions of the same field name as different fields, so older scripts wouldn’t be able to read it.

Having byte and word fields is the best solution IMO. The >255 rule we discussed above would be sufficient.

Changing the GFF behavior in case of duplicate fields sounds like it could break many external tools and invoke all sorts of subtle bugs in the future. The other “>255 rule” would be a much safer change.

Had we foreseen this usecase back in 2022 when the support was added, it would have probably been something like this. The extra complexity is worth it to avoid breakage, even if the behavior is strictly speaking not supported (no one likes breaking things for the heck of it). Instead, we just followed the bioware convention for doing this, as with Wings_New. In fact, originally it was ArmorParts_Robe_New, but GFF field names get silently truncated to 16 characters so it was quickly switched to the x- prefix.

But it just never came up during development nor beta testing. This behavior has been in the beta branches for about two years, and in stable for one year, and it was only now caught. Actually, I believe the current behavior (with regards to JsonToObject) has been around longer than the original behavior that Proleric’s code relied on.

We could change it to the “>255 rule” now. It would break all modules that rely on current behavior, which is also technically unsupported. If this rule (in isolation) was objectively superior enough to warrant the effort and risk it is something that we can consider. I do not feel that it is. “All new content just uses xArmorParts_XXX” is a simple and straighforward rule, that has less potential for things to go wrong. Does anyone disagree?

Note that this discussion has been focused on ArmorParts_ fields and the change to just extend the size of the field. Other GFF level changes are not so easy to handle, so in the general case the statement remains that committing to full backwards+forwards compatibility for GFF format would be prohibitively expensive. Off the top of my head, I think LERP-ing of visual transforms would not have been feasible if this was a requirement.

1 Like

Proleric: Temporary workaround is to change xArmorPart_Robe using GffReplaceWord.

This seems insufficient to me, wasn’t able to get anything going with a simple json edit. (object2json, gffreplaceword, json2object) I never got around to doing armor/robes on my customization stuff, so curious what works besides copyitemandmodify for this or copy everything else to something that already has a robe.

Seems to work for all cases. The key lines of code are

  json   jArmour      = ObjectToJson(oOldArmour, TRUE);
  jArmour = GffReplaceByte(jArmour, "ArmorPart_Robe",  GetItemAppearance(oTemplate, ITEM_APPR_TYPE_ARMOR_MODEL, ITEM_APPR_ARMOR_MODEL_ROBE));
  jArmour = GffReplaceWord(jArmour, "xArmorPart_Robe", GetItemAppearance(oTemplate, ITEM_APPR_TYPE_ARMOR_MODEL, ITEM_APPR_ARMOR_MODEL_ROBE));
  oNewArmour = JsonToObject(jArmour, GetLocation(oPC), oPC, TRUE);

I’ve also been using this technique to modify NPC appearances - head, skin colour, gender etc - without any issues. I’ve read that others are doing the same.

2 Likes

Thx, didn’t think to try setting both.

…except heads now have the same issue as armour parts… easily fixed by setting both byte and word fields in the same way.

A key reason for using Json to create random NPCs is that the appearance and clothing can be configured before the object is created. This is much superior to the old method of modifying parts and inventory, then issuing ActionEquip, after object creation, because the engine really struggles with rendering all that correctly (aka the “headless” problem).

Hey now, Headless are one of my favourite Ultima series enemies.
image

Nice. But it’s not so cool when it’s your random commoners there to add ambience to your city. The headlessness throws that off a bit :slight_smile:

I randomize heads with onSpawn (taking the value from several json array, according to npc’s local variables, and never had issues with headless npcs as long as the value is correct.