Daggerfall Workshop Forums

Weapons shop "The %rt's Arsenal" In Newtale (Bhoriane)

In Daggerfall classic the name is "The Count's Arsenal" (%rt = Ruler Title?)

(new gamesave done without any mods)

Thanks for all your recent reports pango. All very clear with good information.

Will work through these as I'm able.

Been checking this issue out and it's not just a single bug. The %rt is replaced by specific code in building names class rather than using the Macro Handler. I've fixed three separate bugs in this code so it now works. (region name misspelled, wrong faction id used, one based index used to array)

Not sure we should keep two implementations though, Interkarma do you have any reason or desire to keep the specific code for %rt macro on the BuildingNames class? If so, I will submit the 3 bugfixes that make it work. Alternatively the macro handler RegentTitle can be called instead.

I also tried the macro handler and unfortunately that is not working properly either, although I am very sure it did when Nystul added it. I think it's might be a victim of the change to the faction region index done some point in the last few months (I was not paying attention, so I could be way off) Anyway I think this because I traced the problem to the call it makes to PersistentFactionData.FindFactionByTypeAndRegion() which has a 2nd parameter called oneBasedRegionIndex - "Region index to match. Must be ONE-BASED region index used by FACTION.TXT" but when called with a region of 37 (matching FACTION.TXT) it doesn't find & match Bhoraine which has a region of 36 in the PersistentFactionData for me.

I don't know how wide ranging this index mis-match might be and I don't want to make it worse so need some guidance here from whoever changed it.

Interkarma, let me know how you would like me to proceed.

Hazelnut wrote: ↑Mon Oct 15, 2018 9:43 pm Not sure we should keep two implementations though, Interkarma do you have any reason or desire to keep the specific code for %rt macro on the BuildingNames class? If so, I will submit the 3 bugfixes that make it work. Alternatively the macro handler RegentTitle can be called instead.

I'm happy for this handling to be moved out of BuildingNames. Ideally we can get that macro working again.

The gist of Allofich's change to faction handling is to normalise them all to be zero-based. This is an improvement over using one-based in some places and zero-based in others. Part of this change was to normalise faction data saved with one-based values on next load. This all works fairly well, but we're still catching spots where migration was missed. It could be the use of this method was one of those places.

When you send the PR, maybe ping @allofich in the comments and he can offer any insight from his end. I'm at work and can't be much help right now.

Edit: My feeling is that all callers using FindFactionByTypeAndRegion() should stop adding +1 to the region index passed, and FindFactionByTypeAndRegion() param should be renamed to just "regionIndex", instead of "oneBasedRegionIndex" which isn't used anymore.

Great, that's what I thought the right solution would be - wanted to confirm. Thanks.

PR submitted.