Dev build/Linux: weapons shop "The %rt's Arsenal" [RESOLVED]

Post Reply
User avatar
pango
Posts: 285
Joined: Wed Jul 18, 2018 6:14 pm
Location: France
Contact:

Dev build/Linux: weapons shop "The %rt's Arsenal" [RESOLVED]

Post by pango » Sun Oct 14, 2018 5:24 pm

Weapons shop "The %rt's Arsenal" In Newtale (Bhoriane)
Last edited by pango on Sun Oct 14, 2018 5:33 pm, edited 1 time in total.
When a measure becomes a target, it ceases to be a good measure.
-- Charles Goodhart

User avatar
pango
Posts: 285
Joined: Wed Jul 18, 2018 6:14 pm
Location: France
Contact:

Re: Dev build/Linux: weapons shop "The %rt's Arsenal"

Post by pango » Sun Oct 14, 2018 5:32 pm

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

(new gamesave done without any mods)
Attachments
SAVE69.zip
(223.83 KiB) Downloaded 1 time
When a measure becomes a target, it ceases to be a good measure.
-- Charles Goodhart

User avatar
Interkarma
Posts: 3495
Joined: Sun Mar 22, 2015 1:51 am

Re: Dev build/Linux: weapons shop "The %rt's Arsenal"

Post by Interkarma » Sun Oct 14, 2018 8:56 pm

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

Will work through these as I'm able.

User avatar
Hazelnut
Posts: 856
Joined: Sat Aug 26, 2017 2:46 pm
Contact:

Re: Dev build/Linux: weapons shop "The %rt's Arsenal"

Post by Hazelnut » Mon Oct 15, 2018 9:43 pm

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. :)

User avatar
Interkarma
Posts: 3495
Joined: Sun Mar 22, 2015 1:51 am

Re: Dev build/Linux: weapons shop "The %rt's Arsenal"

Post by Interkarma » Mon Oct 15, 2018 11:37 pm

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.

User avatar
Hazelnut
Posts: 856
Joined: Sat Aug 26, 2017 2:46 pm
Contact:

Re: Dev build/Linux: weapons shop "The %rt's Arsenal"

Post by Hazelnut » Tue Oct 16, 2018 7:53 am

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

User avatar
Hazelnut
Posts: 856
Joined: Sat Aug 26, 2017 2:46 pm
Contact:

Re: Dev build/Linux: weapons shop "The %rt's Arsenal"

Post by Hazelnut » Tue Oct 16, 2018 7:43 pm

PR submitted.

Post Reply