Replay Insert Mode ("blue R") silently overwrites: Decision?

Started by Simon, July 25, 2024, 10:13:01 PM

Previous topic - Next topic

0 Members and 1 Guest are viewing this topic.

Simon

Hi,

Replay Insert Mode ("blue R") silently overwrites

namida, you closed the original topic, but you've never declared this fixed nor wontfix.

You consider this data loss. I agree.

You wrote that don't want elaborate fixes or physics changes; fine with me. WillLem offered help, you haven't replied; also fine with me, I assume you'll find something reasonable yourself.

Considering that you care about the data loss, you should at least chose one of the un-elaborate resolutions:
  • Refuse all same-frame assignments.
  • Refuse only to different lemming. Allow overwriting same lemming.
  • Allow noisy overwriting, i.e., tell the player that he lost his old data.
  • Allow silent overwriting and let people run into unnoticed data loss.
-- Simon

namida

QuoteIt pretty much comes down to either "keep as is" or "preserve the original assignment instead", possibly with some added visual and/or audio feedback when the collision occurs.

This was the last thing I said on the matter. Would people prefer the "preserve the original assignment" behavior? Does the advantage of this outweigh the added hassle of overwriting such assignments (ie: you must either remove them with the replay editor, or cut the replay off at an earlier point than the assignment - the latter in particular likely undesirable if Replay Insert Mode is being used in the first place).
My projects
2D Lemmings: NeoLemmix (engine) | Lemmings Plus Series (level packs) | Doomsday Lemmings (level pack)
3D Lemmings: Loap (engine) | L3DEdit (level / graphics editor) | L3DUtils (replay / etc utility) | Lemmings Plus 3D (level pack)
Non-Lemmings: Commander Keen: Galaxy Reimagined (a Commander Keen fangame)

Simon

How is hassle added in this case? Do you have a workflow that relies on the silent overwriting?

I imagine that nobody, when he enters replay insert mode, ever wants to implicitly overwrite to some different far-away lemming. The mere possibility of this accident feels nasty and murky, and was the reason for making the first topic. Your answer makes me now believe that you want the implicit overwrite for some workflow.

Anyway, to answer your question directly: For me, cutting the assignment some other way is vastly preferable over the silent overwrite to different lemmings. I don't mind explicitly cutting assignments in a different way.

-- Simon

namida

Ah, I was mostly thinking about the same lemming. Yeah, needing to overwrite specifically on the same frame for a different lemming is a rare enough case.

"If in replay insert mode, and a skill assignment is attempted that would result in overwriting an assignment to a different lemming, prevent that assignment. Retain existing behavior in the case of assignments to the same lemming, as well as when not in insert mode."

Does this sound good? Should a single physics update still be performed when the assignment is prevented for this reason (as would happen if it were successful)? (I should probably double check what happens with an attempt to make an invalid assignment (eg. assigning a builder to a blocker), and keep consistency with that.)
My projects
2D Lemmings: NeoLemmix (engine) | Lemmings Plus Series (level packs) | Doomsday Lemmings (level pack)
3D Lemmings: Loap (engine) | L3DEdit (level / graphics editor) | L3DUtils (replay / etc utility) | Lemmings Plus 3D (level pack)
Non-Lemmings: Commander Keen: Galaxy Reimagined (a Commander Keen fangame)

Simon

Quote from: namida on July 25, 2024, 11:22:13 PM
"If in replay insert mode, and a skill assignment is attempted that would result in overwriting an assignment to a different lemming, prevent that assignment.

Sounds good!

QuoteRetain existing behavior in the case of assignments to the same lemming,

The existing behavior is: Silently overwrite the next-frame-same-lemming assignment. Yes, keep this. I would be surprised if somebody wants to keep the old assignment here.

In addition, I recommend to remove all future assignments to this same lemming. They'll all be out of sync. They'll even be of sync when one of the two assignments (the overwriting one or the overwritten one) was a permanent ability. The rare exception that doesn't desync is overwriting a permanent ability with a different permanent ability, and this sounds too rare to cater.

This will then be closest to Lix, and I like it like this in Lix, but I may well be biased. Prod more people for opinions.

Quoteas well as when not in insert mode."

The existing behavior is: Cut all future, then append the assignment. Yes, that remains the expected behavior in normal mode, i.e., the mode that isn't insert mode.

QuoteShould a single physics update still be performed when the assignment is prevented for this reason (as would happen if it were successful)?

Interesting. I never had to solve this before, but NL needs a design for this.

First hunch without sleeping over it: Don't advance. On successful assignments, NL advances once. Now, NL refused the assignment, and the easiest feedback is to refuse to advance, too. The feedback won't indicate what exactly is wrong, but you be the judge on how much UI work you want to invest. More is better but will go against your grain.

I'll sleep over it.

Quote(I should probably double check what happens with an attempt to make an invalid assignment (eg. assigning a builder to a blocker), and keep consistency with that.)

Yes, look at what you do on disallowed assignment attempts, and make the two consistent. First hunch without sleeping: Neither should advance.

-- Simon

WillLem

FWIW, I added "prevent overwriting existing assignments when in Replay Insert mode" in SLX (along with the Assign Fail sound to provide auditory feedback) quite early on in development, and it's vastly preferable behaviour.

This commit prevents overwriting; a function is added to check for any replay assignment on the current frame (excluding RR changes) and deployed when attempting a new assignment.

@namida The Assign Fail sound, if you want it, is spread across multiple commits because it was fairly tricky to implement (partly due to getting the hang of the complex assignment procedures, partly due to the existence of highlit lemmings). It's been thoroughly tested and a number of bugs have been fixed, so I'd say it's a stable feature as it currently stands. It definitely improves the gameplay, and is worth the effort. If you want me to do a PR for this I'd be happy to do it, but it might take some time to gather everything together so I won't go ahead with that unless you specifically express interest in adding this feature.

namida

Sure, that sounds good.

Does it have an exception when the new assignment is to the same lemming?
My projects
2D Lemmings: NeoLemmix (engine) | Lemmings Plus Series (level packs) | Doomsday Lemmings (level pack)
3D Lemmings: Loap (engine) | L3DEdit (level / graphics editor) | L3DUtils (replay / etc utility) | Lemmings Plus 3D (level pack)
Non-Lemmings: Commander Keen: Galaxy Reimagined (a Commander Keen fangame)

WillLem

Quote from: namida on July 27, 2024, 06:51:48 AM
Sure, that sounds good.

Cool, I'll get a PR sorted soon and send it over.

Quote from: namida on July 27, 2024, 06:51:48 AM
Does it have an exception when the new assignment is to the same lemming?

Currently, no: the original assignment is always preferred. I could look at updating this - do we think the lem should always accept the new assignment?

For: 1) The player may wish to change the original assignment on the same frame.

Against: 1) The player may wish to preserve the original assignment and apply the new one; if this is the case, the new assignment must take place on a different frame anyway, 2) The player may not realise that there is already an assignment to the current lemming on the current frame and accidentally overwrite (which is exactly what this feature is aiming to avoid), 3) If the player does want to change the original assignment, they can delete it using the Replay Editor if necessary.

I probably lean more towards preserving the original assignment, even for the same lemming.

namida

If they overwrite an assignment to the same lemming, it's going to be noticable pretty quickly (and quite possibly intentional). Overwriting an assignment to a different lemming is far more subtle.
My projects
2D Lemmings: NeoLemmix (engine) | Lemmings Plus Series (level packs) | Doomsday Lemmings (level pack)
3D Lemmings: Loap (engine) | L3DEdit (level / graphics editor) | L3DUtils (replay / etc utility) | Lemmings Plus 3D (level pack)
Non-Lemmings: Commander Keen: Galaxy Reimagined (a Commander Keen fangame)

WillLem

Honestly unsure which way to go on this. I'll send the PR with "preserve all existing assignments" behaviour first, maybe we can test in an RC and see what people think. If consensus is that it's worth changing it to "always overwrite same lem", then I'll figure it out at that point. It should be easy enough to do, but I'd honestly rather get a bit more feedback before going ahead with it (or not).

namida

You can always submit the PR with what you've got as a starting point, and I can adjust from there.
My projects
2D Lemmings: NeoLemmix (engine) | Lemmings Plus Series (level packs) | Doomsday Lemmings (level pack)
3D Lemmings: Loap (engine) | L3DEdit (level / graphics editor) | L3DUtils (replay / etc utility) | Lemmings Plus 3D (level pack)
Non-Lemmings: Commander Keen: Galaxy Reimagined (a Commander Keen fangame)

Simon

What's the status of this?

(If nobody posts, I'll assume that namida would still like to implement this for NL 12.14.)

-- Simon

WillLem

I've now tried numerous times to create the PR, but BitBucket just doesn't want to play ball. I select the newly-created branch as the source, but the most recent 8 commits are automatically bundled along with the intended commit. No idea why this is happening, and BitBucket's interface is no help at all.

I have everything ready to go. I can either send the file changes across manually or, if someone is happy to show me step-by-step how to create a pull request without getting loads of errors from BitBucket, I'll by all means do it that way.

Simon

Wild guesses because you didn't post the error messages: Re-fetch from namida's repo to see his current master. Rebase onto namida's master. Choose a yet-nonexistant branch name. You can delete branches in your public repo that you don't need anymore:
git push yourRemote --delete remoteBranch

We can look at it tomorrow, 15:00 UTC.

-- Simon

WillLem

Quote from: Simon on November 23, 2024, 07:40:44 PMyou didn't post the error messages

There aren't any, it just adds commits with no explanation and no ability to drop commits you don't want to include.

Quote from: Simon on November 23, 2024, 07:40:44 PM: Re-fetch from namida's repo to see his current master.

Yes, good shout. I did that already; I always fetch before making any changes to NL, just to make sure I have the latest working copy.

Quote from: Simon on November 23, 2024, 07:40:44 PMRebase onto namida's master.

Not sure how to do this...

Quote from: Simon on November 23, 2024, 07:40:44 PMWe can look at it tomorrow, 15:00 UTC.

OK, thanks. Plans for this weekend are up in the air at the moment; if I can't make it, I'll send you a message on here.

WillLem

@Simon - I won't be available between 3pm and 8pm today (Sunday) unfortunately. I'm hoping I might be able to join the Lix session for a bit. Maybe we can have a look at the PR sometime during the week?

Simon

During the week, I can offer:

Mon, 25th, 18:00 UTC, and
Wed, 27th, 18:00 UTC.

I recommend the Monday. If we aren't done after Monday night (PR and installing DMD), we can then schedule the Wednesday.

-- Simon

WillLem

Quote from: Simon on November 24, 2024, 02:21:35 AMMon, 25th, 18:00 UTC
...
I recommend the Monday

Thanks. I've put it in the calendar, see you then :)

Simon


WillLem


Simon

The PR is #5: Don't overwrite Replay Insert same-frame assignments + Assign Fail sound

namida has already rejected this PR, and suggests to earmark it for the Community Edition fork of NL.

This PR prevents all same-frame overwriting, even to the same lemming. I recommend that same-frame same-lemming assignments silently overwrite the previous assignment. Probably, all future of this same lemming should be erased in this case.

-- Simon

WillLem

Quote from: Simon on November 25, 2024, 08:58:50 PMI recommend that same-frame same-lemming assignments silently overwrite the previous assignment. Probably, all future of this same lemming should be erased in this case.

I'll take a look at it. I do still think that whether or not to overwrite same-lemming-same-frame assignments is up for debate, though, and would probably want a bit more discussion around it before making a decision either way.

After some quick investigation, here are the existing methods that are interesting/relevant:

  • TLemmingGame.AssignNewSkill - deals explicitly with assigning the skill to the lemming
  • TLemmingGame.ProcessSkillAssignment - deals with passing assignments to AssignNewSkill according to real-time mouse clicks (including to highlit lemmings)
  • TLemmingGame.ReplaySkillAssignment - deals with passing assignments to AssignNewSkill according to existing replay events

At present, the check for [Replay Insert Mode + Existing Event] is in ProcessSkillAssignment because user input is involved, but the scope of this function doesn't include checking for existing replay assignments to a particular lemming.

There are 2 possible approaches here:

1) Expand the scope of ProcessSkillAssignment to look for same-lem-same-frame assignments. This might need to make use of ReplaySkillAssignment logic (some of which might need to be broken out into new functions/methods) in order to look specifically for "is there an assignment to this lemming?", and would add another lemming list scan for every assignment made.

2) Move the [Replay Insert Mode + Existing Event] check to AssignNewSkill, so we decide at the very last minute whether or not to assign. This would reduce the chance of duplicate code and additional scans, but might be the more difficult and bug-prone approach. I'd also have to re-think the Assign Fail sound call, which has already required a lot of manoeuvering. This is the less preferred approach, then.

There might even be another way to do it, but that's all I can think of for now. I'll come back to it later.

For now, is there any more support for overwriting same-lem-same-frame in Replay Insert? What are the reasons for preferring this over using the Replay Editor?



EDIT: Please note that although this feature has been rejected for NL 12.14, it might be included in the proposed "Community Edition" version of NL, so it's worth putting your opinion forward now.