Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Better handling CheckModsAllowFailure #20513

Closed
wants to merge 11 commits into from
Closed

Conversation

cdwcgt
Copy link
Contributor

@cdwcgt cdwcgt commented Sep 27, 2022

close #20511

It was thought that this would affect mod like autoplay but IncompatibleMods will aviod it

@Flan-Fan
Copy link

A thing that I forgot to mention in the original post is that I somehow managed to get a 100 while not failing while attempting to record the problem. Here's a YouTube video of that

@cdwcgt
Copy link
Contributor Author

cdwcgt commented Sep 27, 2022

A thing that I forgot to mention in the original post is that I somehow managed to get a 100 while not failing while attempting to record the problem. Here's a YouTube video of that

Note that by default EZ has three chances to fail

One used when first miss, second is 100

then you failed in the third

@bdach
Copy link
Collaborator

bdach commented Sep 27, 2022

This change makes the following statement in xmldoc false.

/// <returns>Whether the fail should be allowed to proceed. Return false to block.</returns>

Also I'm not convinced that this is the right change to make. If the interface's goal is to prevent failure, it arguably shouldn't get overridden by other mods when multiple fail-deciding mods are active.

The actual problem here is that there are multiple mods trying to affect failure in the first place and the interface isn't properly designed for that. I worry that trying to fix this is going to reintroduce the problem of adding mod combination specific logic.

Maybe there's a chance this could be fixed at a higher level by reformulating how ModWithExtraLives works (i.e. lift the extra lives logic out of IApplicableFailOverride into the health processor), but I'm not sure that's the right direction either.

@cdwcgt
Copy link
Contributor Author

cdwcgt commented Sep 27, 2022

I also think there are some issues with this change, but it seems all are prevented by incompatibilities between mods

Failing while triggering the condition of sd or pf(or all failcondition) is definitely the highest priority (in currently compatible mod combinations

or I think we can do more judge like 'force fail'

For a problem of this pr is NF will faill if Ez open and run out extra life

@cdwcgt
Copy link
Contributor Author

cdwcgt commented Sep 28, 2022

I thought about another possibility of adding the "allowFail" method (or attributes) to the interface.
It is designed to define whether this game session can fail.

If one of the mods is defined it as false, then this session will not allow to fail(for ModBlockFail)

Then the other reservations are as it is.

@pull-request-size pull-request-size bot added size/M and removed size/S labels Sep 28, 2022
@cdwcgt cdwcgt changed the title Perform fail if one of IApplicableFailOverride Mod return true Better handling CheckModsAllowFailure Sep 28, 2022
@cdwcgt
Copy link
Contributor Author

cdwcgt commented Sep 28, 2022

I decide to create a enum to deal with three situations

  1. All combinations with NF: NF It should be unquestionably unfailable, so it return BlockFail and will handle first. all IApplicableFailOverride should incompatible with its to avoid problem
  2. All combinations with SD PF: They should ignore other mod status to trigger failures if eligible, they are incompatible with ModBlockFail.
  3. last case for other mod with extra life: should not block ForceFail when they fail to block because they are eligible.

so if one of mod return BlockFail, fail will not perform and will not check other condition, and then if one of mod return ForceFail, it will handle fail although other mods want to avoid it.
at last all mod are AllowFail, fail will perform

Comment on lines 25 to 28
BlockFail,
ForceFail,
AvoidFail,
AllowFail
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Without XMLDocs I can't tell what AvoidFail does.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

xmldoc has been added

oversight
In the past, it was not successfully tested because of the override of  CheckModsAllowFailure()
@pull-request-size pull-request-size bot added size/L and removed size/M labels Sep 29, 2022
@peppy peppy added the area:mods label Oct 4, 2022
@peppy peppy marked this pull request as draft June 16, 2023 06:09
@cdwcgt cdwcgt closed this Mar 7, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Perfect/Sudden Death doesn't work when used with Easy
5 participants