[2.50.3] Something Strange with SetComboSolid

Reported by @Mitsukara ... I hope that she will clarify the issue. What follows is a transcript from Skype. It concerns this quest:
http://www.purezc.net/forums/index.p...a#entry1009767

AFAIK, the extremely hardcoded scripts are all in the buffer, if you want to look at them.

Message Transcript

Mitsukara: also,Zoria
Mitsukara: it changed the solidity of the wrong combos
Mitsukara: for some reason combo 278 stopped being solid
Mitsukara: here's the screen that's supposed to get changed (it doesn't have combo 278 on it at all)

http://timelord.insomnia247.nl/zc/zc_dev/mitsolid/0.png

Mitsukara: here's the original solidity before changes
http://timelord.insomnia247.nl/zc/zc_dev/mitsolid/1.png

Mitsukara: here's what the changes are supposed to be
http://timelord.insomnia247.nl/zc/zc_dev/mitsolid/2.png

Mitsukara: here's what it would do, though
http://timelord.insomnia247.nl/zc/zc_dev/mitsolid/3.png

Mitsukara: and the same thing changed combo 278, which is some mountains
http://timelord.insomnia247.nl/zc/zc_dev/mitsolid/4.png


ZoriaRPG: @Mitsukara , do you know by how much it's offsetting?'

Mitsukara: No, because all those water combos it's supposed to change and aren't are in a pretty wide range of positions
Mitsukara: the earliest one is combo 8 and the latest one is 694
Mitsukara: if the player has item 126 in their inventory, it checks if the water combos on that screen (aside from the sideview water, over on the left) are solid, and if they are, it makest hem all non-solid (0000b)

ZoriaRPG: this was without re-saving, right?
ZoriaRPG : FWIW f you recompilke and save, the solidity bug may go away
ZoriaRPG : but we want to fix stuff like that, not advise makin a new quest'

Mitsukara: Yes, it was a version I'd saved in 2.50.2
Mitsukara: as I start to type this report I realize that I can't document it very well without spending the better part of an hour on the report,
Mitsukara: and I'm very tirede and have a bunch of stuff to go out and do tomorrow in less than 12 hours
Mitsukara: so I think I should save my draft on this report, and do it right tomorrow night, instead of an incomplete report tonight

I see what the problem is, but it's actually a bug in older versions. Game->SetComboSolid() treated maps as being numbered from 0 instead of 1. Every other function got it right.
This will probably be handled with an extra rule, meaning it will depend on the version the quest was saved in.

Quote:

---

Originally Posted by Saffith

I see what the problem is, but it's actually a bug in older versions. Game->SetComboSolid() treated maps as being numbered from 0 instead of 1. Every other function got it right.
This will probably be handled with an extra rule, meaning it will depend on the version the quest was saved in.

---

I should have looked at the 2.50.1 source...

Code:

---

case COMBOSDM:
    {
        int pos = (ri->d[0])/10000;
        long scr = (ri->d[1]/10000)*MAPSCRS+(ri->d[2]/10000);
       
        if(pos < 0 || pos >= 176 || scr < 0) break;
       
        combobuf[TheMaps[scr].data[pos]].walk=(value/10000)&15;
    }
    break;

---

whereas we have

Code:

---

case COMBOSDM:
    {
        int pos = (ri->d[0])/10000;
        int sc = (ri->d[2]/10000);
        int m = (ri->d[1]/10000)-1;
       
        if(pos<0 || pos>=176 ||
          sc<0 || sc>=MAPSCRS ||
          m<0 || m>=map_count)
            break;
       
        long scr = m*MAPSCRS+sc;
        combobuf[TheMaps[scr].data[pos]].walk=(value/10000)&15;
    }
    break;

---

For whatever reason, the handler is completely different to the rest of the set. Must have been revised somewhen along the history of this, missing the setter for COMBOSDM.

I wasn't aware that you had rewritten the ZScript handlers between versions, and I didn't check that far back; but it seems that you further refined them from the 2.50.1 incarnations.

Whatever way you want to do it sounds good to me as I should think it would be fairly easy to reconcile, although there aren't any quests being made yet in 2.50.3, whether it is worth changing how it worked in the past to justify adding the version change baggage elsewhere is probably debatable.

I seem to recall there being a ZSCRIPT_VERSION definition, so perhaps just updating that and writing the rule around it would be the best way. Just keep me informed please on what you decide to do, and how you do it so that I stay synchronised.

I suppose if you have a specific implementation in mind, please post a diff to the boards, so that I can merge them and make everyone happy; as if that'll ever happen. If you put it on Git, just mark the change in the git comments and drop a link to me. I haven't checked in lately to see if the other recent bugfixes are live up there, but I need to integrate those, too.

Sorry for not getting back to report this sooner; stuff I had to do yesterday wound up being more complicated and exhausting than I expected, so I didn't get anything at all done on the computer when I got back (mostly I hid under a blanket and fell asleep).

Quote:

---

but it's actually a bug in older versions. Game->SetComboSolid() treated maps as being numbered from 0 instead of 1. Every other function got it right.

---

I vaguely recall thinking that was weird! ...multiple years ago when I made the original version of the water boots, that is. That perfectly explains why the cliffs are getting changed.

I think the order of my quotes to the screenshots is mismatched (the one with the cliffs and leevers is showing the combo that got changed unintentionally) but it sounds like this mystery is already solved.

Quote:

---

This will probably be handled with an extra rule, meaning it will depend on the version the quest was saved in.

---

That sounds like a good backwards compatibility fix.

Do you still want a copy of my quest for anything related to this? If so, I can send you my current prerelease (which is still the one I was testing this stuff in). The actual release version will also be out in a few days, and I have no intention of passwording it.

ZScript version would work, too... That's determined when the quest is saved rather than when scripts are compiled, and that should probably change. But neither would stop older versions from opening new quests and running them incorrectly, so ultimately, it doesn't seem to matter much. If no one feels strongly about it, I'll just do whatever.

Quote:

---

Originally Posted by Mitsukara

Do you still want a copy of my quest for anything related to this?

---

Nah, I think we're good.

Quote:

---

Originally Posted by Saffith

ZScript version would work, too... That's determined when the quest is saved rather than when scripts are compiled, and that should probably change. But neither would stop older versions from opening new quests and running them incorrectly, so ultimately, it doesn't seem to matter much. If no one feels strongly about it, I'll just do whatever.


Nah, I think we're good.

---

@Mitsukara & @Saffith : Was it mandatory when using SetComboSolid() to make an adjustment at the script level to compensate for the issue?

I didn't think that was mandatory, but now I remember that there was an issue with LayerMap in 2.50.2 and earlier that I had reported, which is probably what resulted n this change.
@Saffith : I think that ZScript_Version would be best, as that ensures that if there are any other issues, we can do e same, and bump the version. The only way to preserve forward-compatibility for playing 2.50.3 quests in older versions is to revert the change entirely. Forward-compatibility is just flipping impossible anyway. There are already 2.50.2 quests that don't play properly in 2.50.1/0, 2.50.1 quests that don't play properly in 2.50.2 or 2.50.0, but will in 2.50.3; 2.50 quests that don't play properly in 2.50.1, but do in later versions, and so forth.

As long as quests from 2.50.0 through 2.50.2 play properly in the latest version, that is all that you can ultimately do. Trying to make older players able to handle the newer formats will just drive you mad. It might be good insert some functions to check version differences for this sort of thing into the programme as a utility set for the future though, into one of the headers, as a generic compatibility set.

The problem with checking the quest version is that it fixes old quests, but breaks quests that are loaded into ZQuest and then re-saved, without recompiling the scripts. And it's a silent failure, without any warning to the author that they are about to corrupt their quest.

If by "Zscript version number" you mean V_FFSCRIPT and CV_FFSCRIPT, this is not especially useful, since it also is set at the time the quest is saved, and does not necessarily match the version of ZScript at the last time the scripts were compiled (and moreover, the scripts might have been compiled at different times using different version of ZQ, due to the behavior of preserving missing scripts that are not found anymore on the hard drive).

A quest rule is one option, as Saffith suggests. We could also modify ZASM to store more metadata with each script, including a version number, which could be used for these kinds of changes in behavior.

What is the "LayerMap issue" and how is it related to ComboSolid?

Quote:

---

Originally Posted by DarkDragon

The problem with checking the quest version is that it fixes old quests, but breaks quests that are loaded into ZQuest and then re-saved, without recompiling the scripts. And it's a silent failure, without any warning to the author that they are about to corrupt their quest.

If by "Zscript version number" you mean V_FFSCRIPT and CV_FFSCRIPT, this is not especially useful, since it also is set at the time the quest is saved, and does not necessarily match the version of ZScript at the last time the scripts were compiled (and moreover, the scripts might have been compiled at different times using different version of ZQ, due to the behavior of preserving missing scripts that are not found anymore on the hard drive).

A quest rule is one option, as Saffith suggests. We could also modify ZASM to store more metadata with each script, including a version number, which could be used for these kinds of changes in behavior.

---

Good point. I'll write a quest rule and rewrite the function in the near future. I used the quest header's ZC_VERSION, which is what Saffith originally advised, because I had issues with V_FFSCRIPT. This instead, caused problems when the quest author, @Mitsukara recompiled the scripts withotu changing the input values.

The issue with LayerMap, and SetComboSolid, was that it was targeting invalid map IDs with its old calcs. A quest rule seems the best solution to that for the present, although given the state of flux of ZC< I hesitate to add them--we have far too many as is, for things that should be handled by the object editors... I would be in favour of storing that kind of datum, and on consideration, it's probably the best option.

You need to chat with @Grayswandir and ensure that you both keep on the same page with the parser though. I don't want to see you two working on things that are mutually-exclusive, given that the script parser, at the least, should be maintained as directly compatible between main and 2.future; so that exporting between versions is always possible. (We already do this, for 2.50.x compatibility exporting.)

I believe I've said this before, but I strongly urge y'all to send in pull request, and as soon as possible, for features you want incorporated into the main fork.

I'm happy to discuss how best to harmonize changes to the scripting engine, and I don't want to intentionally cause trouble for you or grayswandir, but fixing bugs that corrupt quests has a certain amount of urgency to it.

Here is the proposed refactoring. Old ZASM data structure:

Quote:

---

struct ffscript
{
word command;
long arg1;
long arg2;
char *ptr;
};

---

New one:

Quote:

---

struct zasm
{
int16_t command;
int32_t arg1;
int32_t arg2;
};

struct ZAsmScript
{
// ZASM script types
// Would be an enum, if enums were portably serializable
const int16_t ZA_GLOBAL = 0;
const int16_t ZA_FFC = 1;
const int16_t ZA_ITEM = 2;
const int16_t ZA_GUY = 3;
const int16_t ZA_WPN = 4;
const int16_t ZA_LINK = 5;
const int16_t ZA_SCREEN = 6;

// Version of ZASM this script was compiled for
int16_t version;

// Type of the script (one of ZA_ values above)
int16_t type;

// Name of the script, if the script was compiled from ZScript
// For debugging and logging errors, etc.
int16_t name_len;
char *name;

// The ZASM itself
int16_t commands_len;
zasm *commands;
};

---

This fix has been implemented. Specifically:
- Saffith's fix will take effect the next time a quest's script is recompiled. Old quests still use the old behavior. Old quests loaded into ZQ and saved, without recompiling scripts, will preserve the old behavior.
- All ZASM scripts now carry with them some metadata about their ZASM version, type, etc. Might be useful in the future.