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

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

Mon, 25th, 18:00 UTC
...
I recommend the Monday

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

[Simon]

I'm sitting in Mumble. Join anytime.

-- Simon

[WillLem]

Pull request created successfully with Simon's help

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

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.

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.