Redo (fosnola)

[fosnola]

So I just created a new pull request.

Here is the current list of commits that add new files ( and where you have to modify the different projects to include them ) :

• redo/https://github.com/calref/cboe/commit/fd334ecacf625c40a763a27d83ca6cd98730c82c gfx/texture.hpp
• redo/e325d9bc game/minimap.{hpp,cpp}
• redo/5f7ae60a scenario/area.cpp

Notes

• the commits redo/https://github.com/fosnola/cboe/commit/977731e7ef8df28a88b681309e129fe32f3b5292 and redo/https://github.com/fosnola/cboe/commit/064a4932387a181f157204dd5ff0f85d703947e7 must be pushed together
• redo/https://github.com/fosnola/cboe/commit/3b752eeec41dfa4c0668aebd47249dbf90bb60ca changes the file format to store some image as a (pictNum,pictType): look for std::ostream& operator<< (std::ostream& out, cPictNum pic) at the end of estream.cpp
• I tried in master/https://github.com/fosnola/cboe/commit/dff2c4b24c8580ac2b085087d226a8f808a742a5 to create some basic cmake files, I didn't get this code back.
• I didn't recover the master/https://github.com/fosnola/cboe/commit/ff6096ac8ebc82d53b2096ad49a923428de9f7c1 commit because it added a tint to cPictNum (which is probably not a good idea), but there may be some small parts that can be recovered later (e.g. storing a cPictNum in monster's structure can help simplify the code and the added fragment shader can be interesting).

[CelticMinstrel]

So, if I understand correctly, your texture class means that a larger image can be loaded but the game will effectively treat it as the same size, is that correct?

[fosnola]

Yes, that is the goal. More precisely, the low level functions rect_draw_some_item of gfx/render_image.cpp are modified to take into account that an image can be larger.

Note:

• if you look in my graphicsUnditheredByHand repository, you will find some images that are larger than the original. I used them when testing my code.

[CelticMinstrel]

Are you planning to add everything that was in the original PR? That's not exactly what I was hoping for and definitely makes it unlikely to get merged fast, though it's still an improvement over the previous one in that I can look at a cleaner diff.

[fosnola]

There were almost ~270 commits in my master branch, so it was too much of a hassle to not get the changes in order. But I try

• to avoid committing something that I will revert later (even if I did it once or twice)
• to delete some useless commits ( modification of the image repository, cmake, adding a tint to the image : not a good idea )
• to anticipate some changes ( for some, I even include changes which are in my special_code branch, which contains my new code ).

Notes:

• I'm almost done recovering the changes made in my master branch, I have 2 more commits I need to recover, then I'll have to do a diff to make sure I didn't miss something.
• there are also a lot of interesting changes in the special_code branch, but there are a lot of changes in the scenario formats, so ...

[CelticMinstrel]

Okay. Then once you've added those two commits, don't add any more, or this will never get merged. You can continue any other work you want to do on another branch.

[CelticMinstrel]

(Note: If you find one or two commits you missed, it's fine to add those too, as long as it stops there.)

[fosnola]

Normally, I have added the last two commits. Tomorrow I'll look at the remaining differences between this branch and my master branch to make sure I haven't forgotten anything important (and maybe do some tests).

Note:

• it will also remain to correct the problem when objects call dispell_barrier and unlock. Probably by replacing in void cast_town_spell(location where) "short adj = spell_freebie ? 1 : " by "short const adj = "

[CelticMinstrel]

We can deal with any additional changes needed once all this stuff is handled. I can't review everything at once.

[x-qq]

Please split this PR into topical PRs, explaning the intent of the each changeset in their descriptions.

[CelticMinstrel]

That would definitely be preferred. In its current state I can merge it, but it could take a very long time. If split up into more specific PRs, things will likely get merged much faster.

I'd also recommend (for now at least) leaving out anything that's only fixing address sanitizer or static analyzer errors, because there's just so much stuff to review, and that sort of thing is something I can easily fix myself by running address sanitizer or static analyzer locally. That doesn't mean I won't ever accept such fixes; it's just that they get in the way of the real changes.

[fosnola]

I've already lowered the number of commits from ~270 to 156.

Most of the first half of the commits are bug fixes:

• game crash, (*)
• wrong data saved/recovered,
• now I believe I'm supposed to do that, I can't(**); so I had to take a look at the original sources to see if it's normal, ....

or concerns frequent display problems, 100% CPU usage for no particular reason. Some of them try to solve a specific problem on Mac : if you accept apple events, you have to manage them in a delayed way (otherwise SFML will crash).

In the second part, there are more initiatives:

• allow to use the keyboard a bit more, escape key and arrows,
• allow to load more detailed images without touching the code,
• introduction of a junk bag (optional),

and a bit of code restructuring:

• access to the elements of an array via a get function that checks the bounds, ...
• added some namespace porting, minimap,
• try to reduce code's redundancies, ...

Then yes, when I reread some commits, I told myself that they were useful because they were partially solving a problem but that I had after strongly improved this code (improvements difficult to commit without patch telling me that my next patches FAIL, what he already often told me).

(*) asan was indeed a great help, but I had to run it on several scenarios to find the majority of the errors. (**) like, I'm supposed to go this way, but I can't because block_entry(true) is retrieved but not block_entry(false) or the game gives me a flying scroll, but I can't use it outdoor, ...

[fosnola]

I just checked the differences between this version and my master branch, there are one or two left (*); they don't seem to be essential so I leave the redo branch as is (**).

(*) if we exclude those related to changes I anticipated or reverted in the special_code branch. (**) I added a note in the first post of this page.

[CelticMinstrel]

I've merged a bunch of the commits that look like obvious and small fixes. That doesn't mean the other commits have been rejected, though. I will probably do another pass of this sometime in the future. There are also some things in here that I might prefer to handle in a different way, so that's one reason I skipped over some commits.

However, there are some commits that I do reject, which I've listed below.

8dcc387d15541df24efb76eb16b07e5c5970161a is wrong – that function is supposed to only be called with positive values. If you want to decrease SP you call drain_sp instead. 4d2805619ef38c7a54491019b17ce30097fdcf7c is simply incorrect – those three magic values are documented and expected to work. 772f0e18419a09ffc32bd21569c50ab11f34d3e5 – why would you want a dialog with a title but no strings? If those are being called by code, that code is probably the issue. 9298cd2a090d10f33e9e95257208bf31b72ab774 – assuming this was always large dialogs with an icon and up to 6 strings, I recently located the real cause of this and fixed it properly, so this fix is no longer needed.

Also, it would be nice if you could update your custom dialogs (editor preferences and also 7432c5cd00c68cf2bd4bcd1391753065e5f6f34a) to use relative positioning for the widgets instead of absolute positioning.

---

Now that I've done this, I'd like to request that you rebase this onto my latest master so as to remove all the commits that were already merged. That'll make things much easier when I get around to going through the list again.

As a bonus, if you could split all the UI scale commits into a separate PR, that would enable me to merge it separately in a unit and perhaps get it done much sooner. I'll still merge it if you don't (unless I find a terrible bug in it!), but it'll just probably end up getting left until last. In general this applies to anything that takes more than one commit – I skipped over lots of commits in here simply because they looked like they were part of something bigger, while basically all the commits I did merge were small, simple ones.

[fosnola]

A quick answer concerning these commits:

However, there are some commits that I do reject, which I've listed below.

8dcc387 is wrong – that function is supposed to only be called with positive values. If you want to decrease SP you call drain_sp instead.

Actually, restore_sp can be called with amt<0 by boe.specials.cpp:

case eSpecType::AFFECT_SP:
			if(spec.ex1b == 0)
				pc.restore_sp(spec.ex1a);
			else pc.restore_sp(-spec.ex1a);

( so yes, it is probably wrong to reset cur_sp to max_sp when (cur_sp>max_sp) && (amt>0), but this seems a minor problem...)

Note: In boe.combat.cpp

				pc_target->cur_sp *= abil.second.gen.strength;
				pc_target->cur_sp /= 100;

does not look very safe, and must probably replaced by pc_target->cur_sp = short(int(pc_target->cur_sp)*abil.second.gen.strength/100);

4d28056 is simply incorrect – those three magic values are documented and expected to work.

The original code was:

		case 182:
			for (i = 0; i < T_M; i++)
				if (c_town.monst.dudes[i].number == spec.ex1a) {
					c_town.monst.dudes[i].active = 0;
					}			
			*redraw = 1;
			break;

so when spec.ex1a=0,-1,-2, we don't want to kill anybody. Note: we also need to fix case 183: to avoid killing some innocent monsters if ex1a=-1,-2,...

772f0e1 – why would you want a dialog with a title but no strings? If those are being called by code, that code is probably the issue.

I remember seeing a game that brings up "0str,... does not exist" (but I don't remember which game).

9298cd2 – assuming this was always large dialogs with an icon and up to 6 strings, I recently located the real cause of this and fixed it properly, so this fix is no longer needed.

Probably ok (ie. I can not be sure without doing some tests)

[CelticMinstrel]

Actually, restore_sp can be called with amt<0 by boe.specials.cpp:

I'd have to double-check, but possibly that call could be replaced with drain_sp.

so when spec.ex1a=0,-1,-2, we don't want to kill anybody. Note: we also need to fix case 183: to avoid killing some innocent monsters if ex1a=-1,-2,...

Ah, okay, so it looks like I misunderstood what that commit was doing. In that case, I'll retract my rejection and go ahead and add that commit now. But in case 183…

		for (i = 0; i < T_M; i++)
			if ((c_town.monst.dudes[i].active > 0) &&
				(((spec.ex1a == 0) && (1 == 1)) || 
				((spec.ex1a == 1) && (c_town.monst.dudes[i].attitude % 2 == 0)) || 
				((spec.ex1a == 2) && (c_town.monst.dudes[i].attitude % 2 == 1)))){
				c_town.monst.dudes[i].active = 0;
				}

…looks like the current code is correct? In current BoE, 0 kills all monsters, -1 kills friendly monsters, and -2 kills hostile monsters. I can't tell just from that code whether 1 or 2 kills friendly monsters. Either 1 kills friendly monsters and the current code is correct, or 2 kills friendly monsters and the current code needs to map 1 to -2 and 2 to -1.

I remember seeing a game that brings up "0str,... does not exist" (but I don't remember which game).

Yeah, this should not happen. The fix isn't to create those dialogs though. Instead, it should not show a dialog at all if both strings are blank or missing.

Note: we also need to fix case 183: to avoid killing some innocent monsters if ex1a=-1,-2,...

[CelticMinstrel]

Oh, also, if you notice that one of the commits I merged from here fixes a known bug in the issues section, please comment on that bug so we can close it.

[fosnola]

Actually, restore_sp can be called with amt<0 by boe.specials.cpp:

I'd have to double-check, but possibly that call could be replaced with drain_sp.

The problem is more complex, ie if you look at the initial code 83:

		case 83:
			for (i = 0; i < 6; i++)
				if ((pc < 0) || (pc == i))
					adven[i].cur_sp = minmax(0,	adven[i].max_sp,
						adven[i].cur_sp + spec.ex1a * ((spec.ex1b != 0) ? -1: 1));
			break;

even when ex1b=0, it does not match the restore_sp function (i.e. this code does not check if cur_sp>max_sp). Also, there is no test to check if ex1a is positive => ex1a=-3, ex1b=0 means drain_sp (and this happens in several places in the old code).

so when spec.ex1a=0,-1,-2, we don't want to kill anybody. Note: we also need to fix case 183: to avoid killing some innocent monsters if ex1a=-1,-2,...

…looks like the current code is correct? In current BoE, 0 kills all monsters, -1 kills friendly monsters, and -2 kills hostile monsters. I can't tell just from that code whether 1 or 2 kills friendly monsters. Either 1 kills friendly monsters and the current code is correct, or 2 kills friendly monsters and the current code needs to map 1 to -2 and 2 to -1.

I guess someone merged the old specials 182 and 183 into eSpecType::TOWN_NUKE_MONSTS; that's okay if the inherited code makes sense. The problem is that some legacy codes don't make sense; for example, in at least a legacy game you can find a 182 special with ex1a=-1 (or maybe 0 or -2, I do not remember) that does nothing (the coder probably added a 182 special and then forgot about it), so when we convert old specials, we have to clean up the code a bit to keep the scenario playable (e.g. by setting ex1a to -3)

I remember seeing a game that brings up "0str,... does not exist" (but I don't remember which game).

Yeah, this should not happen. The fix isn't to create those dialogs though. Instead, it should not show a dialog at all if both strings are blank or missing.

This can happen if a scenario contains strings that are empty and use them but also in boe.dlgutil.cpp

case eShopItemType::CALL_SPECIAL:
			cStrDlog(base_item.desc, "", base_item.full_name, base_item.graphic_num, PIC_ITEM).show();
			break;

The simplest solution is to add all the potential 0str*.xml files to avoid this problem. After I don't really have a preference, either we display a message without strings or an error message (as long as it doesn't leave the game)

Note: There is a game that displays a dialog that contains only a picture and "Chapter I: The Invasion" (or something similar); it may be the one that made me add these two files (but I am not sure).

[fosnola]

that I've done this, I'd like to request that you rebase this onto my latest master so as to remove all the commits that were already merged. That'll make things much easier when I get around to going through the list again.

I just did a little test, it might take a little while to do this properly: ie. the conflicts they find are easy enough to solve but there are others they don't find, so I end up with:

      else {
           return true;
      }
      else
           return true;

So I'll have to go step by step, check each time what it does, that it still compiles, ... :-~

[CelticMinstrel]

After I don't really have a preference, either we display a message without strings or an error message (as long as it doesn't leave the game)

Note: There is a game that displays a dialog that contains only a picture and "Chapter I: The Invasion" (or something similar); it may be the one that made me add these two files (but I am not sure).

We should probably also check what the original game did if you request a 2-string dialog with no strings… does it show nothing, or a blank 1-string dialog?

[fosnola]

We should probably also check what the original game did if you request a 2-string dialog with no strings… does it show nothing, or a blank 1-string dialog?

Yes, it is to be checked. But if I had to make a guess, I would say that it doesn't display anything if the special is called with spec.m1=spec.m2=-1 but that on the other hand, the dialog appears if it is called with spec.m1=3, spec.m2=-1 and string number 3 is empty (which would force us to add the 0str files...).

[CelticMinstrel]

but that on the other hand, the dialog appears if it is called with spec.m1=3, spec.m2=-1 and string number 3 is empty (which would force us to add the 0str files...)

Can't we just use the 1str files in that case? Not showing anything when both strings are -1 should be handled by the calling code (don't call the dialog at all), so I don't think there's a conflict between the two cases.

[fosnola]

but that on the other hand, the dialog appears if it is called with spec.m1=3, spec.m2=-1 and string number 3 is empty (which would force us to add the 0str files...)

Can't we just use the 1str files in that case?

Probably. In fact, the code counts the number of non-empty strings to determine which dialog box is used, so we can either add the 0strXXX dialog xmls or add a test if (num_strings==0) use("1str...") else use("%dstr..."% num_strings)

[fosnola]

that I've done this, I'd like to request that you rebase this onto my latest master so as to remove all the commits that were already merged. That'll make things much easier when I get around to going through the list again.

I just tried again, I got https://github.com/fosnola/cboe/tree/redo2 ; I tried to check from time to time that everything was still recompiling ( ie. when rebase gave me back the hand to solve a conflict ) and I check at the end what has changed with a recursive diff.

[CelticMinstrel]

Okay… will you be pushing that to this PR? Or do we close this one and open a new one?

[fosnola]

Okay… will you be pushing that to this PR? Or do we close this one and open a new one?

I will open a new PR: ie. I created a new branch on GitHub because I wasn't sure if GitHub would appreciate a push -force (and not get confused) ; now the easiest way is to make a new PR and close this one

Note I just created the new pull request (Redo2), I hope it is correct. If it is, we can close it.

[CelticMinstrel]

I created a new branch on GitHub because I wasn't sure if GitHub would appreciate a push -force (and not get confused)

For the record, GitHub does not get confused by a push -force.

[fosnola]

For the record, GitHub does not get confused by a push -force.

Ok.

Note: as PR redo2 is created, I close this PR.

[CelticMinstrel]

However, there are some commits that I do reject, which I've listed below. 8dcc387 is wrong – that function is supposed to only be called with positive values. If you want to decrease SP you call drain_sp instead.

Actually, restore_sp can be called with amt<0 by boe.specials.cpp:

case eSpecType::AFFECT_SP:
			if(spec.ex1b == 0)
				pc.restore_sp(spec.ex1a);
			else pc.restore_sp(-spec.ex1a);

( so yes, it is probably wrong to reset cur_sp to max_sp when (cur_sp>max_sp) && (amt>0), but this seems a minor problem...)

I've addressed this differently in d7dcf2464426496cb64d34faa77dccabad251ed7.

Note: In boe.combat.cpp

				pc_target->cur_sp *= abil.second.gen.strength;
				pc_target->cur_sp /= 100;

does not look very safe, and must probably replaced by pc_target->cur_sp = short(int(pc_target->cur_sp)*abil.second.gen.strength/100);

I've also addressed this, in 4c6296612db8d5e2f79cf57678685490eedc7bb9.