NEWCOMBOSDM quest rule

[DarkDragon]

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.

[ZoriaRPG]

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?

[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.

[ZoriaRPG]

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.

[DarkDragon]

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:

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?

[DarkDragon]

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.

[Saffith]

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.

[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.

[ZoriaRPG]

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.

[Saffith]

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