NEWCOMBOSDM quest rule

I've removed it from 2.50.x:

1. It was not correctly incorporated, since it was added to 2.50.x without updating master. It is a complete disaster for the two branches to use the same quest rule bit for different purposes! (all quests saved in 2.50.x would work incorrectly in future ZC versions).

2. The same issue is already fixed in master using a different mechanism (ZASM version metadata, rather than quest rule). Master's mechanism needs to be removed and replaced with whatever 2.50.x does, if 2.50.x is changed to do something different.

3. From the threads here and on PureZC it doesn't sound like anyone will be affected by just rolling back the 2.50.3 COMBOSDM behavior.

If you need to put the quest rule back in for some reason, please ensure points #1 and #2 are fully addressed.

Quote:

---

Originally Posted by DarkDragon

I've removed it from 2.50.x:

1. It was not correctly incorporated, since it was added to 2.50.x without updating master. It is a complete disaster for the two branches to use the same quest rule bit for different purposes! (all quests saved in 2.50.x would work incorrectly in future ZC versions).

---

Of course I did not add it to master. You explicitly told me not to make any internal changes for master until you were done with your own. I can't set up rules changes in ZDefs.h or in FFRules until you finish what you are doing, but I was planning to forward this to master, and to 2.54, later, as that is still an interim goal.

Quote:

---

2. The same issue is already fixed in master using a different mechanism (ZASM version metadata, rather than quest rule). Master's mechanism needs to be removed and replaced with whatever 2.50.x does, if 2.50.x is changed to do something different.

3. From the threads here and on PureZC it doesn't sound like anyone will be affected by just rolling back the 2.50.3 COMBOSDM behavior.

If you need to put the quest rule back in for some reason, please ensure points #1 and #2 are fully addressed.

---

The change to COMBOSDM is critical, as the old code causes ZC to crash if it is used on layers higher than 1. It is not merely there to support 2.50.3RC1 quests, but to address a critical bug. The QR allows us a way to permit this fix in releases until 2.60 is ready, which as far as I can see, is stretching farther into the future, every day. Adding an inclusive statement to check for this old bit or a script header version > blah, should suffice.

When you are done with what you are doing, and I can properly address the rules for 2.60, including porting enemy stuff to the enemy classes, item stuff to the item classes, and all of that fun, this can just sit in there under old_rules as the second-to-last-bit. We also need a bit for bitmap positioning. That was another debacle intended as a silent 'fix' for an old 'bug', that instead became its own huge bug/mistake, introduced in 2.50.2 that fragmented the community by making some very popular quests [U]unplayable[U] in 2.50.2.

I developed a header just to determine the version between 2.50., 2.50.1, and 2.50.2, expressly because of that problem (see: ZVersion.zh) that was used to patch several quests.

I truly want to be able to tell everyone that all quests made in any 2.50.x version are safe in 2.53. I don't want users to need exceptions, or excuses, to avoid updating for fear that Project-X is broken, as this is what happened with absolutely every update to 2.50.0.

Otherwise, what the flidd is the point of spending the time to make it?

I did not handle the change in this manner, on a whim.

Quote:

---

Originally Posted by ZoriaRPG

Of course I did not add it to master. You explicitly told me not to make any internal changes for master until you were done with your own. I can't set up rules changes in ZDefs.h or in FFRules until you finish what you are doing, but I was planning to forward this to master, and to 2.54, later, as that is still an interim goal.

---

You are right, however this kind of change is not a major change to internal quest data structures and the branches cannot be allowed to lose backward-compatibility with respect to each other ("we'll remember to fix it some time in the future" is a dangerous game). If you ask for help I can make sure that changes are properly synched in both branches.

Quote:

---

The change to COMBOSDM is critical, as the old code causes ZC to crash if it is used on layers higher than 1. It is not merely there to support 2.50.3RC1 quests, but to address a critical bug.

---

As I see it there are two separate issues: (i) ZC crashes when passed an invalid layer; (ii) the indexing is off-by-one from the ideal numbering system.

Fixing (i) poses no compatibility issues and requires no quest rule, and can be safely applied to 2.50.x (and master, why not?) Fixing (ii) is the more tricky issue but, as far as I can tell, far from urgent. If you think there really must be a renumbering in 2.50.x, the options are (a) apply the fix in master (based on ZASM versioning) to 2.50.x; (b) remove the fix in master based on ZASM versioning, and add a quest rule (with consistently-placed bits in both branches!!) to both branches. Changing 2.05.x (only) to do its own thing is not a possibility as it breaks quest compatibility.

Quote:

---

We also need a bit for bitmap positioning. That was another debacle intended as a silent 'fix' for an old 'bug', that instead became its own huge bug/mistake, introduced in 2.50.2 that fragmented the community by making some very popular quests [U]unplayable[U] in 2.50.2.

---

I do not know the details about this issue. My preferences, in descending order, are (i) restore 2.50.2 behavior and drop whatever was incorrectly changed in 2.50.3 completely. If this breaks 2.50.3 quests in a non-moot way (somebody's work will actually be affected by the rollback), add compatibility logic to master so that 2.50.3 quests will play correctly on master. (ii) carefully add logic to both 2.50.x and master that preserves behavior of 2.50.3 quests while fixing the 2.50.2 compatibility bug.

Quote:

---

I did not handle the change in this manner, on a whim.

---

I understand, yet regardless of the intent with which it was made, the status quo is unacceptably broken code, for the reasons I explained above and in my original post. The current state of the repository (quest rule completely rolled back) is acceptable in my mind provided that nobody steps forward with concrete evidence that a real-world quest will be affected. If you are worried, you may send a new patch request that fixes the 2.50.3 solidity command and addresses the three concerns in my original post.

Quote:

---

Originally Posted by DarkDragon

I do not know the details about this issue. My preferences, in descending order, are (i) restore 2.50.2 behavior and drop whatever was incorrectly changed in 2.50.3 completely. If this breaks 2.50.3 quests in a non-moot way (somebody's work will actually be affected by the rollback), add compatibility logic to master so that 2.50.3 quests will play correctly on master. (ii) carefully add logic to both 2.50.x and master that preserves behavior of 2.50.3 quests while fixing the 2.50.2 compatibility bug.

---

In 2.50.1 and before, when switching drawing targets between the screen and an offscreen bitmap, the subscreen offset wouldn't always be set properly, so drawing would sometimes incorrectly be shifted by 56 pixels. 2.50.2 fixed that without keeping it for old quests; 2.50.3 does restore it in old quests.
That's currently handled by a bit set at the end of readrules() (in 2.50.x, it's set at qst.cpp:2549 and read at script_drawing.cpp:2159). It's based entirely on the version and build; there's no quest rule or anything else saved with the quest file. Quests made in 2.50.3 will function the same in 2.50.2 in this regard.

The issue then is that the fix wasn't also applied to master? Because that behavior sounds ideal to me.

No, it's also in master.

I'm afraid I'm confused about what the problem is then... @ZoriaRPG can you elaborate?

In any case, Zoria, the TL;DR of this thread is that any change must

1. preserve backward-compatibility of 2.50.x with respect to quests saved in old versions of ZQ;
2. preserve backward-compatibility of master with respect to 2.50.x (from the perspective of master, 2.50.x *is* an "old version" even though it's not released yet!)

These are "iron laws" in the sense that a change that breaks one is almost always a bad change, regardless of its other merits. I rolled back the NEWCOMBOSDM rule because it ran afoul of (2).

Quote:

---

Originally Posted by DarkDragon

I'm afraid I'm confused about what the problem is then... @ZoriaRPG can you elaborate?

In any case, Zoria, the TL;DR of this thread is that any change must

1. preserve backward-compatibility of 2.50.x with respect to quests saved in old versions of ZQ;
2. preserve backward-compatibility of master with respect to 2.50.x (from the perspective of master, 2.50.x *is* an "old version" even though it's not released yet!)

These are "iron laws" in the sense that a change that breaks one is almost always a bad change, regardless of its other merits. I rolled back the NEWCOMBOSDM rule because it ran afoul of (2).

---

Is it safe to edit zdefs.h, ffscript.cpp, and the jwin panes to add this rule to master? If not, then this is just delaying 2.53 indefinitely, until it is safe to edit those files.

If so, I want to get the bitmap offset fix rule in at the same time, as that also needs to go into 2.53.

It would have been nice to alert me of this issue before adding a reversion push. I will need to re-do the same work, again, to reverse it, although given that I need to add in a rule for bitmaps, that is less of an added workload. Still, if it was safe to edit master, then you could have asked me to fix it there, instead. It is a very simple fix, and migration later is not applicable here, as this is a compatibility rule for the engine, that need be read-only in future builds. ( Setting it for 2.60 makes no sense.)

I will probably invert it though.

Quote:

---

Originally Posted by Saffith

In 2.50.1 and before, when switching drawing targets between the screen and an offscreen bitmap, the subscreen offset wouldn't always be set properly, so drawing would sometimes incorrectly be shifted by 56 pixels. 2.50.2 fixed that without keeping it for old quests; 2.50.3 does restore it in old quests.
That's currently handled by a bit set at the end of readrules() (in 2.50.x, it's set at qst.cpp:2549 and read at script_drawing.cpp:2159). It's based entirely on the version and build; there's no quest rule or anything else saved with the quest file. Quests made in 2.50.3 will function the same in 2.50.2 in this regard.

---

Unfortunately, this did not make it into the 2.50.x branch, from the reports that I am getting. I think that having both a version check, and a QR is the safest way, with the version check setting the bit on quest init. This applies both to bitmap drawing, and COMBOSDM. That way, the bit is all that matters for future versions; and if we use a script header for some of these fixes, then the bit reading can be an inclusive or condition to evaluate alongside the script header. If the scritpt header is < n, check for the bit. If the bit is off, use the older of the two methods.

I do not know why the fix that was in 2.50.3 is not here, or not working. Is this another case of my files missing a commit, or is it truly missing in the repo?

I got the line numbers from the repo, so it's definitely there.

Here's a test quest made in 2.50.1: https://www.dropbox.com/s/8ku2k4hku7...pTest.qst?dl=0
It tries to draw three tiles at the same Y position.
In 2.50.1, they're misaligned.
In 2.50.2, they're aligned.
In 2.50.3, they're misaligned.
In 2.53 beta 2, the're misaligned.
So it appears to be working as expected in both 2.50.3 and 2.53.


I'm also seeing that the middle tile is sometimes drawn with a black background and sometimes not, at least in 2.50.2 and up. That should probably be looked into...

Quote:

---

It is a very simple fix, and migration later is not applicable here, as this is a compatibility rule for the engine, that need be read-only in future builds.

---

No, I don't understand what you're saying.

There is nothing wrong with combo solidity in 2.50.2 and earlier, other than (i) failure to bounds-check the value (easy fix that does *not* require any quest rules), and (ii) a slightly aesthetically-displeasing convention for how the parameter is numbered. (ii) is an extremely minor issue and is *not* worth a quest rule to fix if that is the only reason for a change, especially since the numbering has *already* been fixed in master. That change will roll out whenever we publish the next version of ZC. There is no need to rush it out for 2.53.

If you feel that changing the numbering is an urgent issue, back-port the fix in master to 2.50.x. Do not add new, different, and incompatible mechanics for fixing the same bug. Do not add a new quest rule.

Similarly, quest rules are added to fix fully-understood compatibility bugs in the code. Not sprinkled here and there "just in case" a bug may or may not be squashed. Saffith says the bitmap alignment issue is resolved (modulo the color problem he pointed out). If this is not the case, please explain the situation where bitmap alignment is still broken, and/or point to a test quest, and we will fix it properly.

Quote:

---

Originally Posted by DarkDragon

No, I don't understand what you're saying.

There is nothing wrong with combo solidity in 2.50.2 and earlier, other than (i) failure to bounds-check the value (easy fix that does *not* require any quest rules), and (ii) a slightly aesthetically-displeasing convention for how the parameter is numbered. (ii) is an extremely minor issue and is *not* worth a quest rule to fix if that is the only reason for a change, especially since the numbering has *already* been fixed in master. That change will roll out whenever we publish the next version of ZC. There is no need to rush it out for 2.53.

If you feel that changing the numbering is an urgent issue, back-port the fix in master to 2.50.x. Do not add new, different, and incompatible mechanics for fixing the same bug. Do not add a new quest rule.

Similarly, quest rules are added to fix fully-understood compatibility bugs in the code. Not sprinkled here and there "just in case" a bug may or may not be squashed. Saffith says the bitmap alignment issue is resolved (modulo the color problem he pointed out). If this is not the case, please explain the situation where bitmap alignment is still broken, and/or point to a test quest, and we will fix it properly.

---

I'm talking about these notations:

* Fixed Game->Get/SetCombo* failing on maps greater than 1. ( Saffith, 2016-02-05 14:29:20 )
* Fixed incorrect screen calculation and possible crash in Game->SetComboSolid(). ( Saffith, 2015-12-19 19:41:38 )

I know for a fact that SetComboSolid in 2.50.1 and 2.50.2 could cause crashes. If this fix was entirely different to what we are discussing now, then I apologise.
@Saffith : Was this unrelated?

There were three distinct issues fixed at once:
- Arguments were not validated
- Maps numbers started at 0, not 1
- Screen calculation was based on 128 screens per map, not 136

The map numbering is only a minor nuisance, but the incorrect screen calculation isn't something users should be expected to work around.

Quote:

---

Originally Posted by Saffith

There were three distinct issues fixed at once:
- Arguments were not validated
- Maps numbers started at 0, not 1
- Screen calculation was based on 128 screens per map, not 136

The map numbering is only a minor nuisance, but the incorrect screen calculation isn't something users should be expected to work around.

---

Am I correct in my belief that this COMBOSDM change was also what fixed the 'possible crash', which is what I originally reported happening with SetComboSolid/SetLayerComboS crashing 2.50.2 ?

There is a reason that I wanted to enforce this distinct behaviour here. Backporting the script headers to this would violate the policy that the script system itself should not differ to 2.50.x for 2.53. Changing the parser and all to use the new system would be problematic, at best, for this release. That is why I went with a QR bit that is set automatically.

Quote:

---

Originally Posted by Saffith

There were three distinct issues fixed at once:
- Arguments were not validated
- Maps numbers started at 0, not 1
- Screen calculation was based on 128 screens per map, not 136

The map numbering is only a minor nuisance, but the incorrect screen calculation isn't something users should be expected to work around.

---

Agreed; however I don't see this change as part of https://github.com/ArmageddonGames/Z...08edffdcb999d6

or as being incorporated in the current master code:

Code:

---

case COMBOSDM:

{

int pos = (ri->d[0])/10000;

int sc = (ri->d[2]/10000);

int m = (ri->d[1]/10000)-1;

if (curscript->version < 2)

m++;

if(pos>=0 && pos<176 &&

sc>=0 && sc<MAPSCRS &&

m>=0 && m<map_count)

{

long scr = m*MAPSCRS+sc;

int layr = FFScript::whichlayer(scr);

if(scr==(currmap*MAPSCRS+currscr))

ret=(combobuf[tmpscr->data[pos]].walk&15)*10000;

else if(layr>-1)

ret=(combobuf[tmpscr2[layr].data[pos]].walk&15)*10000;

else ret=(combobuf[TheMaps[scr].data[pos]].walk&15)*10000;

}

else

ret = -10000;

}

    break;

---

Do you know when the calculation was changed, and the current state (and need) for compatibility support for it?

Quote:

---

Originally Posted by ZoriaRPG

There is a reason that I wanted to enforce this distinct behaviour here. Backporting the script headers to this would violate the policy that the script system itself should not differ to 2.50.x for 2.53. Changing the parser and all to use the new system would be problematic, at best, for this release. That is why I went with a QR bit that is set automatically.

---

I'm not sure what "policy" you're referring to. It's true we should avoid risky changes; whether adding ZASM version information to .qst file qualifies is up for debate.

In any case, I've already explained why changes to 2.53 that make quests saved there load incorrectly in master are unacceptable. I'm a little surprised this wasn't self-evident; but in any case, if you send a pull request that fixes the COMBOSDM numbering in 2.53 and fully addresses the three points in my original post, I'd be happy to merge it in. You may want to wait for Saffith to respond, though, since it sounds like there's a third issue with COMBOSDM behavior in old quests that hasn't been addressed in either 2.50.x or master.

Sorry, I guess I misremembered things. It never used the wrong number of screens per map. Maybe I'm thinking of something else.

So I guess it's just the bounds checking that needs added now.

I'm sure no one cares anymore, but I found what I was thinking of: Game->Get/SetScreenState(). The compiler multiplies the map number by 136, but screen state is stored in game->maps, which uses 128 screens per map. That was accounted for way back in build 1326.

Quote:

---

Originally Posted by Saffith

I'm sure no one cares anymore, but I found what I was thinking of: Game->Get/SetScreenState(). The compiler multiplies the map number by 136, but screen state is stored in game->maps, which uses 128 screens per map. That was accounted for way back in build 1326.

---

So, this is a lingering bug? If so, I may as well correct it, as long as doing that does not affect old quests. I might roll it forward to 2.54, if it might need a rule.

It's working correctly. No need to do anything.

Quote:

---

Originally Posted by Saffith

It's working correctly. No need to do anything.

---

Ah, fantastic. Good to know, and thank you for informing us of this. I always find your insights useful, despite what you may have believed.