PDA

View Full Version : Random Behaviour from 'int onSaveMapPic()'



ZoriaRPG
10-01-2018, 02:01 PM
Here's a good one:

The function int onSaveMapPic() seems to behave erratically, as follows:

Spamming the F5 key, which calls this, during scrolling will copy the dest screen map to the scrollbuf of the screen you are leaving.
Likewise, it can cause the game to become paused, as if the user pressed F3.

Apparently, it can also trigger screen secrets and do other things.

ANy ideas on what the cause might be?

This function is sort of horrible, to begin with.

Here's just the single function (https://pastebin.com/JSPkadHR), for reference.

Normally with something simple such as drawing the dest screen onto the scrollbuf, I'd suspect a simple error, but with all of these random patterns, it acts as if it's reading and writing to undefined memory.

I feel as if this has something to do with temp_buf, but I have only begun to analyse this issue.

At the least, I might require doing this from the menus, so that the game is paused; or look at the code that generates the spacebar map and see if it doesn't align somewhere. IDR what the function name is that generates the spacebar map, so, that'll be fun to find again. rMBtn() should be the key to that, but a quick search didn't show any calls to rMBtn() or similar Mbtn stuff that actually calls a the function for it.

Looking closer, I think it is an issue with using temp_buf, because that's also used by void draw_screen(mapscr* this_screen, bool showlink).

At least, that'd be the problem with spamming it during scrolling, as it's trying to use that buffer at the same time as other functions. I'm not sure about triggering secrets, or pause states though. That truly sounds like something is uninitialized somewhere, or pointer errors.

Saffith
10-01-2018, 08:21 PM
It's because of the call to loadscr(). It loads the screen to draw into tmpscr[1], which is where the screen being scrolled from is stored.
That's presumably also why F5 is disabled in caves and item rooms - there, tmpscr[1] is the screen you return to.

ZoriaRPG
10-01-2018, 08:35 PM
It's because of the call to loadscr(). It loads the screen to draw into tmpscr[1], which is where the screen being scrolled from is stored.
That's presumably also why F5 is disabled in caves and item rooms - there, tmpscr[1] is the screen you return to.

Oh, dear. Very well, I'll have a look at that and make some safer swap space in general for it. IDK why it was designed to share these resources at all. In fact, it seems as if it could just have locals, that exist until the function exits, but perhaps there's a reason that wasn't done, that I don't know about.

( I hadn't tracked back where else loadsxr() was used that might conflict. I did track back the palette setting functions, to ensure that it wasn't those, and then I was hit by this server nightmare. )

I can't imagine that it writing to temp_buf, as it does, is safe, either, FWIW.

TY Saffith.

P.S. Do you think that either of these could be linked to it randomly pausing or triggering things? I'll look at loadscr() tomorrow, but I can't imagine that doing anything that'd pause the game, unless somehow for a frame it invalidates some ptr and doesn't know what to do with itself, but that'd usually just crash.

If after safety-netting this, it still does it, I guess it's time for more robust debugging. Part of me wants to fix this to read cset2 values at the same time, because nothing does that properly. Nothing at all.

Saffith
10-01-2018, 09:02 PM
Hard to say, but I wouldn't be surprised. loadscr does a lot of stuff, and there's no telling what might result from changing out data while other parts of the program are using it.
The easiest thing to do would be to increase the size of tmpscr by one and load to tmpscr[2] instead.

ZoriaRPG
10-04-2018, 01:31 PM
Hard to say, but I wouldn't be surprised. loadscr does a lot of stuff, and there's no telling what might result from changing out data while other parts of the program are using it.
The easiest thing to do would be to increase the size of tmpscr by one and load to tmpscr[2] instead.

I ended up rewriting it based on ViewMap(), which seems to be good, although only time will tell if it still has some issues that I missed. Adding the third index and using that failed to properly update the screen on the map, and continually blitted one screen to every space that Link visited, but I did try it. In the end, I went with my what was my first thought...

I believe that loadscr2() is safer, too, as it was designed to display maps and not do a number of other random things, as so...

https://github.com/ArmageddonGames/ZeldaClassic/commit/26d7d6ac0534afce40eda0ced8ea8f711ab407cb