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

fix berserk continue the fight to almost infinite if value negative(1.15/16 version) #4086

Open
wants to merge 7 commits into
base: master
from

Conversation

@newfrenchy83
Copy link
Contributor

commented May 20, 2019

if berserk value<0 in both special or abilitie then the fight continue until death of combattant, and the number limit of turn may be infinite. for fix that, the value must be be null or positive only.

fix berserk continue the fight to infinite if value negative
if berserk value<0 in both special or abilitie then the fight continue until death of combattant, and the number limit of turn may be infinite. for fix that, the value must be be null or positive only.
@@ -143,9 +143,9 @@ battle_context_unit_stats::battle_context_unit_stats(const unit& u,
petrifies = weapon->bool_ability("petrifies");
poisons = !opp.get_state("unpoisonable") && weapon->bool_ability("poison") && !opp.get_state(unit::STATE_POISONED);
backstab_pos = is_attacker && backstab_check(u_loc, opp_loc, units, resources::gameboard->teams());
rounds = weapon->get_specials("berserk").highest("value", 1).first;
rounds = std::max(0, weapon->get_specials("berserk").highest("value", 1).first);

This comment has been minimized.

Copy link
@jostephd

jostephd May 20, 2019

Member

Do you need to update the other battle_context_unit_stats constructor?

This comment has been minimized.

Copy link
@jostephd

jostephd Jun 17, 2019

Member

@newfrenchy83 Just reminding you that this still needs to happen, both here and in #4122.

This comment has been minimized.

Copy link
@jostephd

jostephd Jun 17, 2019

Member

@newfrenchy83 Please answer my question.

This comment has been minimized.

Copy link
@newfrenchy83

newfrenchy83 Jun 17, 2019

Author Contributor

i already changed attack.cpp for replace sts::max by clamp in both hre and in #4122

This comment has been minimized.

Copy link
@jostephd

jostephd Jun 17, 2019

Member

Yes, but the constructor has two overloads and you only updated one of them.

Show resolved Hide resolved src/actions/attack.cpp Outdated
@CelticMinstrel

This comment has been minimized.

Copy link
Member

commented May 20, 2019

I think we should not allow an infinite berserk loop, as in some rare cases it could result in a soft-locked game (at least you'd be able to escape to the menu, but that's about it). For example, what if the both units somehow have a 0% CTH, or they both happen to be immune to each other's weapon's damage type?

I can't tell from the description though whether this is removing or adding the possibility of an infinite loop, though based on josteph's comments I guess it's removing it? In that case I support the general idea.

@newfrenchy83

This comment has been minimized.

Copy link
Contributor Author

commented May 20, 2019

I think alos what an infinite loop is an bad diea, and i be happy what @CelticMinstrel will be to same opinion.
and what will be interest of an infinite loop for berserk then what high value is sufficient for don't locked game? for me infinite loop is a bug to and no an adding by develloper.

@jostephd

This comment has been minimized.

Copy link
Member

commented May 22, 2019

and what will be interest of an infinite loop for berserk then what high value is sufficient for don't locked game? for me infinite loop is a bug to and no an adding by develloper.

Let's say, a value such that a battle between a Berserker and an Elvish Fighter, both with 0% cth, completes in 120 seconds in the default settings (combats are animated, x1 acceleration). Note that in this case, both negative and large positive values should be handled the same way.

@gfgtdf

This comment has been minimized.

Copy link
Contributor

commented May 23, 2019

I don't think 'Using -1 to represent "no limit" ' is something we should support, it's neither intuitive unless you have knowledge about the quirks of signed-unsigned conversion nor do I see any use case for real unlimited berserk. I think it's safe to assume that a negative number there is in most cases the result of a bug in the wml code (like using a wrong formula in the berserk ability) that should be logged.

@jostephd

This comment has been minimized.

Copy link
Member

commented May 23, 2019

it's neither intuitive unless you have knowledge about the quirks of signed-unsigned conversion nor

The signed/unsigned conversion is a red herring. The question is whether using -1 to mean infinity would be a good WML API. It's pretty common to use -1 to represent "no limit" even in context such as ini files and command-line arguments where any string value could be used. If we opt for -1 to mean "no limit", we should change the implementation to not depend on signed-to-unsigned conversions.

I think it's safe to assume that a negative number there is in most cases the result of a bug in the wml code (like using a wrong formula in the berserk ability) that should be logged

How about if we accept -1 to mean infinity but consider any other negative number an error?

Tally so far:

-1 should mean infinity: @jostephd, @soliton- (on #3918). That's also the current behavior.

-1 shouldn't mean infinity: @CelticMinstrel, @stevecotton (in a reaction to CelMin), @newfrenchy83, @gfgtdf.

@newfrenchy83

This comment has been minimized.

Copy link
Contributor Author

commented May 28, 2019

@jostephd this is because rounds variable is declared unsigned int what negatives values create these effect. An similar bugs has affect chance_to_hit and fixed in d8869b8#diff-54411f8c204f7ceac25ecc0882ee5623

@newfrenchy83 newfrenchy83 changed the title fix berserk continue the fight to infinite if value negative WIP fix berserk continue the fight to infinite if value negative May 28, 2019

@soliton-

This comment has been minimized.

Copy link
Member

commented May 28, 2019

Such a conversion would produce a big number but certainly not infinity.

As jostephd mentioned -1 is a common convention not a result of some implementation detail.

Whether we want to allow infinity is of course another question and there is certainly precedence for wesnoth trying to prevent infinite loops in WML for example...

@newfrenchy83

This comment has been minimized.

Copy link
Contributor Author

commented May 28, 2019

so in this case, -1 does not mean 'infinite' but is converted into a large number, if it was really an infinite value like convention then I would understand the hesitations of those thinking the 'infinite' loop is deliberate, but here it would be a bug, not a deliberate act, and yes the infinite loops are a danger to the gameplay in wesnoth that needs to be repaired

@newfrenchy83 newfrenchy83 changed the title WIP fix berserk continue the fight to infinite if value negative fix berserk continue the fight to almost infinite if value negative May 28, 2019

@jostephd

This comment has been minimized.

Copy link
Member

commented May 29, 2019

so in this case, -1 does not mean 'infinite' but is converted into a large number, if it was really an infinite value like convention then I would understand the hesitations of those thinking the 'infinite' loop is deliberate,

@newfrenchy83 Yes, the loop is not infinite in the mathematical sense, but it is an infinite loop, as far as the player is concerned. The number of iterations is so large that the player is not going to wait for the loop to terminate.

here it would be a bug, not a deliberate act

Yes, the behavior is caused by a bug (specifically, a signed-to-unsigned cast), but that doesn't mean the behavior itself is undesirable. As I already said on one of the other issues discussing this, we could change the implementation to deliberately interpret -1 as "no limit on number of rounds", removing the signed-to-unsigned cast without changing the WML API.

yes the infinite loops are a danger to the gameplay in wesnoth that needs to be repaired

In this case, an infinite loop can only happen if somebody wrote WML that prescribes it. In general, it's not an error for an interpreter to enter an infinite loop if it's interpreting a program that prescribes an infinite loop. For example, sh -c 'while true; do :; done' enters an infinite loop and that's not a bug in sh.

@CelticMinstrel

This comment has been minimized.

Copy link
Member

commented Jun 8, 2019

I still maintain that infinite berserk is undesirable, and probably the best option is to print an error and set it to 1 or 2. (A value of 0 should also trigger the error, actually. In fact, maybe even a value of 1?)

In this case, an infinite loop can only happen if somebody wrote WML that prescribes it.

There's actually a max loop count to guard against this scenario, though I'm not sure if that's used in every type of loops or only some types...

Yeah, it's not an error for the interpreter to enter an infinite loop if it's interpreting code that prescribes one. But it doesn't have to obey that code and actually loop forever, and I think when we're talking about a scripting language for a game, it may be better not to.

@jostephd

This comment has been minimized.

Copy link
Member

commented Jun 8, 2019

I still maintain that infinite berserk is undesirable, and probably the best option is to print an error and set it to 1 or 2. (A value of 0 should also trigger the error, actually. In fact, maybe even a value of 1?)

I don't see why a value of 1 should be an error. If someone wrote, say, a pickable item that increases terrain defense by 1% and decreases the number of berserk rounds by one, I think it should be perfectly fine to let a single unit take 29 copies of that item. Actually, I think a unit should even be allowed to take 30 copies of that item. That would basically remove the unit's berserk attacks, which might sound stupid, but could be a good strategy in some cases, for example if the unit has a strong ranged attack and the player wants to trade off melee retaliation damage for terrain defense. I don't want to limit what UMC authors or players can do. However, I do agree with you that it's poor UX for a game to enter an infinite loop.

So... maybe if a berserk attack goes on for more than of iterations, we pop a dialog and ask the player if they want to abort or continue the current battle/scenario?

@CelticMinstrel

This comment has been minimized.

Copy link
Member

commented Jun 15, 2019

So basically you're suggesting that an attack with a berserk special for 0 rounds should be equivalent to an attack with a disable special? I think I could get behind that, sure. I still don't think we should allow negative values to mean infinite rounds, though. That said, even if we didn't, someone could set the rounds to something ludicrously high like 10,000,000 to get the same effect, so your idea of popping up a dialog after a certain number of rounds (say, every 50 or 100 rounds) sounds like a good one.

@newfrenchy83

This comment has been minimized.

Copy link
Contributor Author

commented Jun 15, 2019

why not impose an superior limits of rounds (100-150) then?

@stevecotton

This comment has been minimized.

Copy link
Contributor

commented Jun 16, 2019

IMHO, berserk with 1 round would be to disable the special, and with 0 rounds would disable the weapon.

I wouldn't worry so much about deliberately setting a high value, as there are surely other ways to make deliberate infinite loops in a malicious add-on, and we're have to handle that as malice. I'm more interested in the accidental case, such as (berserk limiter add-on + another add-on + items in a campaign) causing a negative value, particularly if it means the player has to either hack the save file or restart the campaign.

@jostephd

This comment has been minimized.

Copy link
Member

commented Jun 16, 2019

I'm more interested in the accidental case, such as (berserk limiter add-on + another add-on + items in a campaign) causing a negative value, particularly if it means the player has to either hack the save file or restart the campaign.

That sounds like a good reason to treat negative values either as zero or as errors.

clamp round value
for what loop don't be too greater, it is preferable to value can't greater limit value(100 or other).

@newfrenchy83 newfrenchy83 changed the title fix berserk continue the fight to almost infinite if value negative fix berserk continue the fight to almost infinite if value negative(1.15/16 version) Jun 17, 2019

newfrenchy83 added some commits Jun 17, 2019

newfrenchy83 added some commits Jul 25, 2019

@Wedge009

This comment has been minimized.

Copy link
Member

commented Aug 14, 2019

I haven't read through all of the discussion but it seems to me this PR is just ensuring berserk attack count remains positive, which (generally speaking) seems sensible to me. Is there a strong reason to not merge this? Or is the discussion regarding accidental & malicious infinite berserk attacks attempting to resolve the question on whether or not to force a positive berserk attack count?

@jostephd

This comment has been minimized.

Copy link
Member

commented Aug 14, 2019

@Wedge009 This PR makes it so if WML specifies a negative number of rounds, the number of rounds will be silently changed to 1. I have two concerns with this: I think we should allow zero rounds, if possible (see #4086 (comment)) and I'm not sure that silently converting a negative value to a nonnegative one is better than simply throwing an error.

The part about showing a dialog if the number of rounds is very large could probably be split to another issue, it's not strictly about handling of negative values, it's also relevant to [berserk] value=999999.

@Wedge009

This comment has been minimized.

Copy link
Member

commented Aug 14, 2019

Fair enough. Limiting makes sense - I thought we already limited to 30 rounds (or is that just the configuration setting for the Ulfserker line?).

@jostephd

This comment has been minimized.

Copy link
Member

commented Aug 14, 2019

I thought we already limited to 30 rounds (or is that just the configuration setting for the Ulfserker line?).

It's part of the configuration for the Ulfserker line:

#define WEAPON_SPECIAL_BERSERK
# Canned definition of the Berserk ability to be included in a
# [specials] clause.
[berserk]
id=berserk
name= _ "berserk"
description= _ "Whether used offensively or defensively, this attack presses the engagement until one of the combatants is slain, or 30 rounds of attacks have occurred."
value=30
[/berserk]
#enddef

If there's a limit, it's certainly higher than 30, or the bug this discussion started with ("setting the number of rounds to -1 sets it to the 2**32") wouldn't be possible.

@Wedge009

This comment has been minimized.

Copy link
Member

commented Aug 14, 2019

This reminds me of that scenario in TRoW where Haldric first makes landfall on the great continent and can potentially face endless waves of enemies until we added a turn limit on the scenario end condition of just 'first death of Haldric's team'. I find it hard to think of a good reason for (practically) unlimited berserk attacks vs having some sort of reasonable maximum limit. As it currently stands, then, it seems that upper limits are only there if they are specified in the unit.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
7 participants
You can’t perform that action at this time.