mrimer
Level: Legendary Smitemaster

Rank Points: 5064
Registered: 02-04-2003
IP: Logged

Briareos wrote:
Ummm... is anyone here able to create victory demos with the latest patch? It sure doesn't work for me, as I already posted in it's own thread...

No, no one can. I've responded to this in that thread.

____________________________
Gandalf? Yes... That's what they used to call me.
Gandalf the Grey. That was my name.
I am Gandalf the White.
And I come back to you now at the turn of the tide.

mrimer
Level: Legendary Smitemaster

Rank Points: 5064
Registered: 02-04-2003
IP: Logged

TFMurphy wrote:
Maybe go through each root and recalc as normal, but check against the old groups before adding them to the tile list? And if they're trying to add a tile they weren't connected to before the recalc, ignore it? bRecalc is only for removing briar, after all.

This is the very idea I had in mind too.

EDIT 2: As always when I'm attempting things I don't quite *fully* understand, I'm kinda leery about this work but... here's an attempt to fix it. So, not my best work, and could probably do with some tidying up.

It seems to be working... I'm not noticing any immediate glitches, the room in The Briary is working fine, no demos in Under the Library are breaking, and even all the rooms in some of the holds I was working on that were built around closing doors on briar seem to be working fine. But... eh, still a little concerned, simply because I'm not sure what all this could affect. Still, this is my attempt, and it isn't seeming to crash and burn, so... hope it helps.

Yes, the fix looks good. I've added it. I cleaned up the code in a couple places after your fix and added some more comments, but didn't make any semantic changes. Thank you for rechecking all these briar rooms -- that's very useful.

____________________________
Gandalf? Yes... That's what they used to call me.
Gandalf the Grey. That was my name.
I am Gandalf the White.
And I come back to you now at the turn of the tide.

mrimer
Level: Legendary Smitemaster

Rank Points: 5064
Registered: 02-04-2003
IP: Logged

schep wrote:
First issue:
1. Step on any double potion.
2. Optionally, move around, but don't place.
3. Undo. This correctly takes you to the turn before stepping on the potion.
4. Step onto the potion again.
5. Try to undo. It won't let you!

A couple days ago, Gerry and I fixed this one by now always allowing a move undo to be performed while placing a double (even if you just undid the turn where the double was just placed). We agree it's a little quirky, but I don't think anyone will mind going back to the last "real" player movement in this special case. So, I won't need to apply your fix here. Sorry about that.

Other issue:
1. Be in a room with a preplaced clone or use a clone potion. Make some moves.
2. Undo.
3. Switch to another clone.
4. Go to step 2.
You now have Unlimited Undo (okay, limited by when you drank the clone potion, maybe).

I wasn't aware of this issue or had forgotten it. Thanks for fixing it!

I'm also playing with changes in DRODLib so that undoing will skip past double placement and clone swapping commands. But one thing has me confused: A Question dialog always gives Undo as an option, but then in normal gameplay, CGameScreen applies its normal rules to that sort of Undo as well: no multiple undo, and no undoing during cutscenes. This is probably why Undo never worked in the Negotiator's finale in TCB. Mike, would you rather let Undo-from-Question be more permissive, or add a check so Undo isn't an option when it isn't going to work?

I know these special cases makes undoing properly in all cases a mess. I'm tentatively fine with someone making a fix that always allows undo when a question is pending and no cutscene is playing, and also to not show the Undo option if undo isn't allowed at the current point in time.

However, there are tricky situations that could still cause player confusion, like the Negotiator's final room in TCB. In that room, a cutscene plays until the moment before a question is asked, and then turns off to ask the question. In this case, we could make it so the player can undo the question, but then the game would back up to the final move of the cutscene, which would just advance the game forward to asking the question again. So undo in this case is meaningless. I'm doubtful there's an airtight way to deal with all this, but anyone is welcome to make an attempt at a more universal fix. We could end up with other undo bugs, though, so we'd have to perform a lot of testing before adding it. Hmm...we could write a fix up now, but I'd put it into the next patch candidate rather than in 3.1, which is getting released very soon.

____________________________
Gandalf? Yes... That's what they used to call me.
Gandalf the Grey. That was my name.
I am Gandalf the White.
And I come back to you now at the turn of the tide.

mrimer
Level: Legendary Smitemaster

Rank Points: 5064
Registered: 02-04-2003
IP: Logged

mrimer wrote:

schep wrote:
Other issue:
1. Be in a room with a preplaced clone or use a clone potion. Make some moves.
2. Undo.
3. Switch to another clone.
4. Go to step 2.
You now have Unlimited Undo (okay, limited by when you drank the clone potion, maybe).

I wasn't aware of this issue or had forgotten it. Thanks for fixing it!

Wait a second...without your fix, I'm not seeing the behavior you describe happen anyway. I wonder if you still have one of your own personal changes involving undo in your local copy of the code that isn't in the repository, which causes this clone UU bug. I'm not going to add this fix until I can reproduce this bug on my end.

____________________________
Gandalf? Yes... That's what they used to call me.
Gandalf the Grey. That was my name.
I am Gandalf the White.
And I come back to you now at the turn of the tide.

---

[Last edited by mrimer at 09-16-2007 05:13 PM]

silver
Level: Smitemaster
Rank Points: 915
Registered: 01-18-2005
IP: Logged

mrimer wrote:
stuff about undo

I haven't really looked at how undo is implemented, so this is probably a useless suggestion, but:

assuming there's a "state stack", where your first move is on the bottom, and the head/pointer is on the last thing to happen:

then add to each state a flag "is_a_real_turn?" or something similarly named. make it false for moves of the double, for use of a dialog box, for turns "taken" in cutscenes.

on undo, pop the stack until is_a_real_turn is true.


____________________________

mrimer
Level: Legendary Smitemaster

Rank Points: 5064
Registered: 02-04-2003
IP: Logged

TFMurphy wrote:
* An awful lot of demos are now complaining about being corrupt, despite playing perfectly. Almost all complain, but there are some unique ones that don't. Haven't spotted a pattern yet: even demos in the same room with only a few differences can have one saying it's corrupt and the other not. Completely empty rooms are sometimes complaining, while certain rooms full of roaches aren't. Yeah, I'm having trouble spotting this pattern. EDIT: But I'd hazard a guess at "any room with scripting". EDIT 2: Nope, that still doesn't quite work for every room. It's close though.

Can you point out a room in your .dats where I could reproduce this? I haven't seen it happen yet in my testing of different rooms.

* Regarding Master Walls: Assertion Errors in demo are gone, but the minidemo and corrupt demo checking (well, I think it does, but it's difficult to tell with the current problem with corrupt reporting) still assumes Master Walls are up, while watching them assumes Master Walls are down.

Also, running into an obstacle on a lowered Master Wall doesn't fire CID_HitObstacle (among other things), and if you hold down the key, the visual effect of the player's last move before bumping sometimes flickers.

I think the former is a symptom of the latter bug. When I fixed the latter issue, demo replay that steps through open master walls works just fine for me in the miniroom replay window and also in the demo description.

____________________________
Gandalf? Yes... That's what they used to call me.
Gandalf the Grey. That was my name.
I am Gandalf the White.
And I come back to you now at the turn of the tide.

Briareos
Level: Smitemaster

Rank Points: 3516
Registered: 08-07-2005
IP: Logged

mrimer wrote:

TFMurphy wrote:
* An awful lot of demos are now complaining about being corrupt, despite playing perfectly. Almost all complain, but there are some unique ones that don't. Haven't spotted a pattern yet: even demos in the same room with only a few differences can have one saying it's corrupt and the other not. Completely empty rooms are sometimes complaining, while certain rooms full of roaches aren't. Yeah, I'm having trouble spotting this pattern. EDIT: But I'd hazard a guess at "any room with scripting". EDIT 2: Nope, that still doesn't quite work for every room. It's close though.

Can you point out a room in your .dats where I could reproduce this? I haven't seen it happen yet in my testing of different rooms.

That looks suspicuously like what I'm having here.

TFMurphy, if I may ask - does DROD still record new victory demos for you?

np: Nicole Willis - Bliss Of Life (Soul Makeover)

____________________________
"I'm not anti-anything, I'm anti-everything, it fits better." - Sole
R.I.P. Robert Feldhoff (1962-2009)

---

[Last edited by Briareos at 09-16-2007 06:06 PM]

TFMurphy
Level: Smitemaster
Rank Points: 3118
Registered: 06-11-2007
IP: Logged

mrimer wrote:
Can you point out a room in your .dats where I could reproduce this? I haven't seen it happen yet in my testing of different rooms.

Well, in The City Beneath alone...

The Infohut: The Entrance and 1W report as corrupt.
The High Path: No corrupt demos.
The City: The Entrance, 2N, 3N, 5N, 1N2W, 1N3W, 2N3W, 2N2W, 3N3W, 3N2W, 3N1W, 2N1W, 1N2E, 2N2E, 2N3E, 1N3E, 3N2E, 3N3E, 4N2E, 6N, 2N5W, 1N4W, 4N5W, 5N5W, 6N5W, 5N4W, 5N3W, 5N1W, 4N3W and 6N2W all report as corrupt.
The Naming Office: 1E reports as corrupt.
Twenty-Fifth Level: No corrupt demos.
Theater Hall: The Entrance reports as corrupt.

...you get the idea.

This occurs even with reimporting my 3.0.0 dats (which you have). And yes, Briareos, victory demos fail to record as well. My favourite demo recording test room - The Magic Show: Infinite Enemies: 1S - is now failing to record even without using Undo or pressing 'R' like the previous issue prevented.

mrimer
Level: Legendary Smitemaster

Rank Points: 5064
Registered: 02-04-2003
IP: Logged

TFMurphy wrote:

mrimer wrote:
Can you point out a room in your .dats where I could reproduce this? I haven't seen it happen yet in my testing of different rooms.

Well, in The City Beneath alone...

The Infohut: The Entrance and 1W report as corrupt.
The High Path: No corrupt demos.
The City: The Entrance, 2N, 3N, 5N, 1N2W, 1N3W, 2N3W, 2N2W, 3N3W, 3N2W, 3N1W, 2N1W, 1N2E, 2N2E, 2N3E, 1N3E, 3N2E, 3N3E, 4N2E, 6N, 2N5W, 1N4W, 4N5W, 5N5W, 6N5W, 5N4W, 5N3W, 5N1W, 4N3W and 6N2W all report as corrupt.
The Naming Office: 1E reports as corrupt.
Twenty-Fifth Level: No corrupt demos.
Theater Hall: The Entrance reports as corrupt.

Hmmm....I've replayed through The Infohut and don't get any corrupted descriptions for any of the demos I've recorded either with your player files or with my own. I wonder what's happening on your end that's not on mine. This seems to be the fixal serious issue left to work out, so I think I'll post our fixes so far as build 48. I'll also upload the latest code, so you and schep can debug what's causing the corruption message to appear. This stuff should be up in 10-20 minutes or so.

____________________________
Gandalf? Yes... That's what they used to call me.
Gandalf the Grey. That was my name.
I am Gandalf the White.
And I come back to you now at the turn of the tide.

schep
Level: Smitemaster

Rank Points: 865
Registered: 03-01-2005
IP: Logged

mrimer wrote:
Wait a second...without your fix, I'm not seeing the behavior you describe happen anyway. I wonder if you still have one of your own personal changes involving undo in your local copy of the code that isn't in the repository, which causes this clone UU bug. I'm not going to add this fix until I can reproduce this bug on my end.

Whoops, yeah. That issue only came up after my DRODLib Undo changes. Sorry for the false alarm. The behavior in unmodified builds is that undo right after a CMD_CLONE undoes only the most recent CMD_CLONE, which is rather useless, but can't be exploited like that.

---

[Last edited by schep at 09-16-2007 06:44 PM]

Briareos
Level: Smitemaster

Rank Points: 3516
Registered: 08-07-2005
IP: Logged

mrimer wrote:
Hmmm....I've replayed through The Infohut and don't get any corrupted descriptions for any of the demos I've recorded either with your player files or with my own. I wonder what's happening on your end that's not on mine. This seems to be the fixal serious issue left to work out, so I think I'll post our fixes so far as build 48. I'll also upload the latest code, so you and schep can debug what's causing the corruption message to appear. This stuff should be up in 10-20 minutes or so.

FWIW, the corrupted demo messages seem to have gone away for me with build 48. Must've been something else you changed...

np: Nicole Willis - Dinosauri (Soul Makeover)

____________________________
"I'm not anti-anything, I'm anti-everything, it fits better." - Sole
R.I.P. Robert Feldhoff (1962-2009)

Briareos
Level: Smitemaster

Rank Points: 3516
Registered: 08-07-2005
IP: Logged

FWIW, I was just watching Rabscuttle's top demo for Twenty-Three Rooms, First Level: 1N2E.

Stepping through the demo until turn 7 (where he turns to carve the first bend into the tar), then hitting "4" and "6" to move a step back and forward will make Beethro bump into the tar east of him as if hitting "6" on the keypad had made him move east instead of turning clockwise to cut the tar.

Hijinx ensue afterwards, of course.

np: Nicole Willis - All The Time (Roberto Rodrigues Remix) (Soul Makeover)

____________________________
"I'm not anti-anything, I'm anti-everything, it fits better." - Sole
R.I.P. Robert Feldhoff (1962-2009)

TFMurphy
Level: Smitemaster
Rank Points: 3118
Registered: 06-11-2007
IP: Logged

mrimer wrote:
Hmmm....I've replayed through The Infohut and don't get any corrupted descriptions for any of the demos I've recorded either with your player files or with my own. I wonder what's happening on your end that's not on mine. This seems to be the fixal serious issue left to work out, so I think I'll post our fixes so far as build 48. I'll also upload the latest code, so you and schep can debug what's causing the corruption message to appear. This stuff should be up in 10-20 minutes or so.

That would be because your latest tinkering fixed it ^_^ I'm betting on the bug Briareos brought up, since a lot of the rooms that didn't come up as corrupt were left via stairs or scripting. (Not all room-exit demos came up as corrupt though)

I'll take a longer look at things later - I'm going to be kinda busy tonight, so won't have the time I'd like to spend on it until later, but we'll see what's left to be found.

schep
Level: Smitemaster

Rank Points: 865
Registered: 03-01-2005
IP: Logged

mrimer wrote:
I'll also upload the latest code, so [TFMurphy] and schep can debug what's causing the corruption message to appear. This stuff should be up in 10-20 minutes or so.

The link changed, but now gives me browser error 404. Oh well, sounds like now there aren't many bugs left.

mrimer
Level: Legendary Smitemaster

Rank Points: 5064
Registered: 02-04-2003
IP: Logged

schep wrote:

mrimer wrote:
I'll also upload the latest code, so [TFMurphy] and schep can debug what's causing the corruption message to appear. This stuff should be up in 10-20 minutes or so.

The link changed, but now gives me browser error 404. Oh well, sounds like now there aren't many bugs left.

I took the time to commit everything to our repository first. It's up now.

Also, I'm glad to hear the demo corruption message seems to be gone now.

____________________________
Gandalf? Yes... That's what they used to call me.
Gandalf the Grey. That was my name.
I am Gandalf the White.
And I come back to you now at the turn of the tide.

mrimer
Level: Legendary Smitemaster

Rank Points: 5064
Registered: 02-04-2003
IP: Logged

Briareos wrote:
FWIW, I was just watching Rabscuttle's top demo for Twenty-Three Rooms, First Level: 1N2E.

Stepping through the demo until turn 7 (where he turns to carve the first bend into the tar), then hitting "4" and "6" to move a step back and forward will make Beethro bump into the tar east of him as if hitting "6" on the keypad had made him move east instead of turning clockwise to cut the tar.

Hmm...I'm wondering whether the "6 moves east" phenomenon is actually just a coincidental red herring. What happens when you use the left and right arrows on the four-arrow pad instead of 4 and 6 to navigate turns?

Tangent: what issues are left to fix up now? Is this post the only one, or did I miss anything that still needs fixing?

____________________________
Gandalf? Yes... That's what they used to call me.
Gandalf the Grey. That was my name.
I am Gandalf the White.
And I come back to you now at the turn of the tide.

Briareos
Level: Smitemaster

Rank Points: 3516
Registered: 08-07-2005
IP: Logged

mrimer wrote:
Hmm...I'm wondering whether the "6 moves east" phenomenon is actually just a coincidental red herring. What happens when you use the left and right arrows on the four-arrow pad instead of 4 and 6 to navigate turns?

Mmmh, herring...

Actually, you're right - the same thing happens when using the arrow keys; so it's not that, but a bug nonetheless - I think this only happens after a double has been placed, but it sure makes going fore and back through a demo a pain in the rear exit...

No biggie, though.

____________________________
"I'm not anti-anything, I'm anti-everything, it fits better." - Sole
R.I.P. Robert Feldhoff (1962-2009)

TFMurphy
Level: Smitemaster
Rank Points: 3118
Registered: 06-11-2007
IP: Logged

mrimer wrote:
I think the former is a symptom of the latter bug. When I fixed the latter issue, demo replay that steps through open master walls works just fine for me in the miniroom replay window and also in the demo description.

Running through some Build 48 test now, but this still isn't quite fixed. The issue is that when the hold is *not* Mastered on your profile (easy enough to just add a new Secret Room to the hold in the editor), minidemos and description will treat Master Walls as if they're up, while watching the demo treats Master Walls as if they're down. It's not a big issue, but it is annoying that the minidemo is understanding it differently from actually watching the demo.

Essentially, to reproduce this from scratch, you'd do:
* Create a new hold.
* In its only room, create two staircases.
* Block one staircase off with a Master Wall.
* Conquer the hold using the normal staircase.
* Now replay the room and use the Master Wall staircase to complete the room.
* Finally, go back into the editor and add a new inaccessible secret room.

The demo you made that went through the Master Wall should now report as being corrupt, even though it can be watched fine (since it considers the Master Walls to be down). [fixed in build 39 -- mrimer]

The other Master Wall problems all look fixed now, from what I can see.

---

[Last edited by mrimer at 09-19-2007 01:47 AM]

schep
Level: Smitemaster

Rank Points: 865
Registered: 03-01-2005
IP: Logged

I think I understand the demo replay issue, but my attempts at fixing it haven't worked yet.

With the current front end, CCurrentGame::wTurnNo increments once for each command added to the Commands container. So it's fine and dandy that CDemoScreen::OnKeyDown() case SDLK_LEFT calls pCurrentGame->SetTurn(pCurrentGame->wTurnNo - 1). But when looking at an older demo in which double placement was recorded as a bunch of moves followed by CMD_WAIT rather than CMD_DOUBLE(coordinates), this isn't true: Each double placement move was put in Commands, but only the final CMD_WAIT incremented wTurnNo. Changing the behavior of wTurnNo isn't an option, or we're back into 'demo is corrupt' problems.

EDIT:

I don't think this one's going to be easy. Several functions in DRODLib, including CCurrentGame::SetTurn(), seem to assume that this->wTurnNo and this->Commands.Size() are interchangeable. So it looks like the only options are:

1. Go through DRODLib and make sure everything is clear about whether variables refer to the turn number or a number of commands. This seems like going backwards, now that these are always the same except in old demos.

2. "Convert" old demos to use CMD_DOUBLE like a new one would. I think the only way to do this is to process each command internally (much like the demo verify step). But when to do that?
a. When the "Watch" action is selected from the F6 screen, since that's the only scenario where "step backwards" is even possible.
b. Any time a demo is loaded to a "current game", since it's better in general to keep things the way DRODLib expects.
c. Any time a demo is imported from external file or CaravelNet, but not from the database, AND do one of those one-time database-upgrade things to all saved games in people's databases.

With 2a or 2b, it might be possible to combine the conversion process with the CDbDemo::Test() call, avoiding a double behind-the-scenes runthrough.

I could probably come up with a patch doing 2a or 2b, but wouldn't want to mess with the database-upgrade part of 2c on my own.

---

[Last edited by schep at 09-16-2007 11:49 PM]

mrimer
Level: Legendary Smitemaster

Rank Points: 5064
Registered: 02-04-2003
IP: Logged

TFMurphy wrote:

mrimer wrote:
I think the former is a symptom of the latter bug. When I fixed the latter issue, demo replay that steps through open master walls works just fine for me in the miniroom replay window and also in the demo description.

Running through some Build 48 test now, but this still isn't quite fixed. The issue is that when the hold is *not* Mastered on your profile (easy enough to just add a new Secret Room to the hold in the editor), minidemos and description will treat Master Walls as if they're up, while watching the demo treats Master Walls as if they're down. It's not a big issue, but it is annoying that the minidemo is understanding it differently from actually watching the demo.

Essentially, to reproduce this from scratch, you'd do:
* Create a new hold.
* In its only room, create two staircases.
* Block one staircase off with a Master Wall.
* Conquer the hold using the normal staircase.
* Now replay the room and use the Master Wall staircase to complete the room.
* Finally, go back into the editor and add a new inaccessible secret room.

The demo you made that went through the Master Wall should now report as being corrupt, even though it can be watched fine (since it considers the Master Walls to be down).

Hmm...in the scenario you describe above, I'd say it is correct that the demo being described would be labeled as corrupt or from a different version of the game once a new secret room is added. It seems to me that the thing that should be fixed here then is that demo replay always assumes master walls are down. Instead, it should apply the same criterion as everywhere else and not provide a special case hack to make the master walls appear down. (Hmm...why did we put that hack in there anyway?)

____________________________
Gandalf? Yes... That's what they used to call me.
Gandalf the Grey. That was my name.
I am Gandalf the White.
And I come back to you now at the turn of the tide.

mrimer
Level: Legendary Smitemaster

Rank Points: 5064
Registered: 02-04-2003
IP: Logged

schep wrote:
I don't think this one's going to be easy. Several functions in DRODLib, including CCurrentGame::SetTurn(), seem to assume that this->wTurnNo and this->Commands.Size() are interchangeable. So it looks like the only options are:

...

2. "Convert" old demos to use CMD_DOUBLE like a new one would. I think the only way to do this is to process each command internally (much like the demo verify step). But when to do that?
a. When the "Watch" action is selected from the F6 screen, since that's the only scenario where "step backwards" is even possible.
b. Any time a demo is loaded to a "current game", since it's better in general to keep things the way DRODLib expects.
c. Any time a demo is imported from external file or CaravelNet, but not from the database, AND do one of those one-time database-upgrade things to all saved games in people's databases.

With 2a or 2b, it might be possible to combine the conversion process with the CDbDemo::Test() call, avoiding a double behind-the-scenes runthrough.

I could probably come up with a patch doing 2a or 2b, but wouldn't want to mess with the database-upgrade part of 2c on my own.

Good thinking, and thanks for providing brainstorming notes. Yes, options 2a and 2b appear the least invasive. I'd tend to prefer 2a since, as you point out, this is most likely a one-time fix for the specific backward-forward control the user has only on the Demo Screen. I don't think conversion code needs to be plugged directly into DRODLib because it would be wasteful to compute everywhere else. If 2a is okay with you, go right ahead with a patch!

(Edit: my initial thoughts on how to do that, which you may take or leave as you'd like, since you're the one implementing the patch: when watching a demo on the Demo Screen, whenever the game state reaches a point where the player is placing a double, then if there is a next command and it is not CMD_DOUBLE, then silently play through the next commands until CMD_WAIT (or end of demo) is found, and then collapse the cursor placement turns into a single CMD_DOUBLE command, or truncate the command list at this point if CMD_WAIT is not encountered before the end of demo. The turn iterator probably needs to be refreshed at this point since we're meddling with the command sequence. This might be exactly what you have in mind. In that case, it's nice to be on the same page.)

____________________________
Gandalf? Yes... That's what they used to call me.
Gandalf the Grey. That was my name.
I am Gandalf the White.
And I come back to you now at the turn of the tide.

---

[Last edited by mrimer at 09-18-2007 05:33 PM]

schep
Level: Smitemaster

Rank Points: 865
Registered: 03-01-2005
IP: Logged

mrimer wrote:
Hmm...in the scenario you describe above, I'd say it is correct that the demo being described would be labeled as corrupt or from a different version of the game once a new secret room is added. It seems to me that the thing that should be fixed here then is that demo replay always assumes master walls are down. Instead, it should apply the same criterion as everywhere else and not provide a special case hack to make the master walls appear down. (Hmm...why did we put that hack in there anyway?)

Because "the same criterion as everywhere else" depends on whether the current player has mastered the hold. But you can view a demo which wasn't created by the current player. In fact, the player which created the demo might not be in the database at all. I think you'd get the same results if instead of adding a new secret room you just switch to a player who hasn't mastered the hold, and then watch the demo. Which is why I've been arguing that for demo validation and playback (except the CaravelNet Spider), master walls are always down.

I'd think we should forcibly set CCurrentGame::bHoldMastered to true at an appropriate time after loading a demo. If the Spider works the way I imagine, it could easily flip that to false, maybe with an extra bool function parameter or two added to DRODLib.

mrimer
Level: Legendary Smitemaster

Rank Points: 5064
Registered: 02-04-2003
IP: Logged

schep wrote:

mrimer wrote:
Hmm...in the scenario you describe above, I'd say it is correct that the demo being described would be labeled as corrupt or from a different version of the game once a new secret room is added. It seems to me that the thing that should be fixed here then is that demo replay always assumes master walls are down. Instead, it should apply the same criterion as everywhere else and not provide a special case hack to make the master walls appear down. (Hmm...why did we put that hack in there anyway?)

Because "the same criterion as everywhere else" depends on whether the current player has mastered the hold. But you can view a demo which wasn't created by the current player. In fact, the player which created the demo might not be in the database at all. I think you'd get the same results if instead of adding a new secret room you just switch to a player who hasn't mastered the hold, and then watch the demo. Which is why I've been arguing that for demo validation and playback (except the CaravelNet Spider), master walls are always down.

I'd think we should forcibly set CCurrentGame::bHoldMastered to true at an appropriate time after loading a demo. If the Spider works the way I imagine, it could easily flip that to false, maybe with an extra bool function parameter or two added to DRODLib.

Ah, gotcha. I remember now. So...if I understand correctly, the most failsafe general-purpose solution is: when viewing mini-demos and also calling ::Test to get the demo description, you're suggesting to always assume the hold is mastered, but the spider should always have it set to false. Is that right?

____________________________
Gandalf? Yes... That's what they used to call me.
Gandalf the Grey. That was my name.
I am Gandalf the White.
And I come back to you now at the turn of the tide.

schep
Level: Smitemaster

Rank Points: 865
Registered: 03-01-2005
IP: Logged

mrimer wrote:
So...if I understand correctly, the most failsafe general-purpose solution is: when viewing mini-demos and also calling ::Test to get the demo description, you're suggesting to always assume the hold is mastered, but the spider should always have it set to false. Is that right?

Yes, that's what I think would work.

schep
Level: Smitemaster

Rank Points: 865
Registered: 03-01-2005
IP: Logged

mrimer wrote:

schep wrote:
...

2. "Convert" old demos to use CMD_DOUBLE like a new one would. I think the only way to do this is to process each command internally (much like the demo verify step). But when to do that?
a. When the "Watch" action is selected from the F6 screen, since that's the only scenario where "step backwards" is even possible.
b. Any time a demo is loaded to a "current game", since it's better in general to keep things the way DRODLib expects.
c. Any time a demo is imported from external file or CaravelNet, but not from the database, AND do one of those one-time database-upgrade things to all saved games in people's databases.

...

Good thinking, and thanks for providing brainstorming notes. Yes, options 2a and 2b appear the least invasive. I'd tend to prefer 2a since, as you point out, this is most likely a one-time fix for the specific backward-forward control the user has only on the Demo Screen. I don't think conversion code needs to be plugged directly into DRODLib because it would be wasteful to compute everywhere else. If 2a is okay with you, go right ahead with a patch!

Ah phooey. I went to check a suspicion, and it turns out that this issue also affects saved games. To reproduce:

In JtRH, create a hold with a mimic potion and a checkpoint. Play it, making sure to place the mimic potion and then step on the checkpoint. Import the hold and then player into TCB (3.0.0 or Beta 3.1.0.48, same results, I think). Continue playing for a little while from either the "current" saved game or the checkpoint, then attempt to Undo. You'll see buggy behaviors and assertion errors.

So it sounds like option (1) is out of favor. Modifying 2a to try to catch places where a saved game is loaded and restored would be pretty nasty. That leaves us with:

2b-prime: Do a fixup every time a CDbSavedGame is converted to a CCurrentGame. (eww?)
2c-prime: Do a fixup every time a CDbSavedGame is imported from external XML, plus implement a database upgrade.

And here's a new idea. Let's call it (3), before the numbering gets too complicated. Do a fixup on the fly whenever old style mimic placement is detected. If the command list is not frozen, CCurrentGame::ProcessCommand() already handles the fixup. Each caller of CCurrentGame::ProcessCommand() which freezes the commands should check before each command for the case where wPlacingDoubleType!=0 and the command about to be processed is not CMD_DOUBLE. When that happens, it should temporarily unfreeze the commands, call the fixup function, refreeze the commands, and reset its iterators and whatever other reasons it had for wanting the commands frozen. I think there are about four of these contexts to modify. (Is CCurrentGame::FindLastCheckpointSave() dead code? If so, fix it like the others, or remove it?)

Now that I've thought through it well enough to type it out, I think (3) is the easiest way to go to make sure old DbSavedGames for both demos and restores work right. I'm going to start trying to make it actually work, unless I hear otherwise.

---

[Last edited by schep at 09-18-2007 11:55 PM : :P]

mrimer
Level: Legendary Smitemaster

Rank Points: 5064
Registered: 02-04-2003
IP: Logged

schep wrote:
And here's a new idea. Let's call it (3), before the numbering gets too complicated. Do a fixup on the fly whenever old style mimic placement is detected. If the command list is not frozen, CCurrentGame::ProcessCommand() already handles the fixup. Each caller of CCurrentGame::ProcessCommand() which freezes the commands should check before each command for the case where wPlacingDoubleType!=0 and the command about to be processed is not CMD_DOUBLE. When that happens, it should temporarily unfreeze the commands, call the fixup function, refreeze the commands, and reset its iterators and whatever other reasons it had for wanting the commands frozen. I think there are about four of these contexts to modify. (Is CCurrentGame::FindLastCheckpointSave() dead code? If so, fix it like the others, or remove it?)

Yes, it does look like that is dead code. Let's remove it.

Now that I've thought through it well enough to type it out, I think (3) is the easiest way to go to make sure old DbSavedGames for both demos and restores work right. I'm going to start trying to make it actually work, unless I hear otherwise.

Hey, that doesn't sound too bad! Yes, please go ahead. I'll start working on the master wall fix now. [fixed for build 49 -- mrimer]

____________________________
Gandalf? Yes... That's what they used to call me.
Gandalf the Grey. That was my name.
I am Gandalf the White.
And I come back to you now at the turn of the tide.

---

[Last edited by mrimer at 09-19-2007 05:31 PM]

Syntax
Level: Smitemaster
Rank Points: 1218
Registered: 05-12-2005
IP: Logged

mrimer wrote:
[fixed for build 39 -- mrimer]

That's what I call backward compatibility [d'oh -- mrimer]

---

[Last edited by mrimer at 09-19-2007 05:31 PM]

Monkey
Level: Master Delver

Rank Points: 190
Registered: 03-21-2006
IP: Logged

Uh... I see build48, not build49...

____________________________
lurking

schep
Level: Smitemaster

Rank Points: 865
Registered: 03-01-2005
IP: Logged

File: double_cmds.patch (15.8 KB)
Downloaded 40 times.
License: Public Domain

This patch seems good with my tests so far, but something this big needs a while with other testers involved.

I ended up modifying the ProcessCommand()ing in three places, not counting the deleted dead code:

All other calls of CCurrentGame::ProcessCommand() I determined don't need to do the fixup. Feel free to double check me here. Details:
(Yeah, no spoilers or secrets here, just semi-relevant long paragraphs.)

By the way, since Mike applies most of these patches "manually", I added the -p flag to diff, which makes function names or other context markers appear on the '@@' lines. Might be a bit helpful. Hope it doesn't mess up anybody's patch program.

---

[Last edited by schep at 09-20-2007 02:49 AM : whitespace]

mrimer
Level: Legendary Smitemaster

Rank Points: 5064
Registered: 02-04-2003
IP: Logged

schep wrote:
This patch seems good with my tests so far, but something this big needs a while with other testers involved.

Man, those are some slick fixes. They look good! I'll put build 49 out now so people can start testing out this behavior on imported 2.0 demos and saved games with double placement right away.

____________________________
Gandalf? Yes... That's what they used to call me.
Gandalf the Grey. That was my name.
I am Gandalf the White.
And I come back to you now at the turn of the tide.