[3.03] Invisibility bug

[Loren Tyr]

Summary: Removing the RemoveInvisibility call from the two GenericAbility.Apply functions would make official and known to the players the existing (and reasonable, in gameplay/flavour terms) status quo that using non-attacking abilities leaves you invisible, as well as fixing the Backstab+Strike ability+Invisibility bug. 

 

----------

 

When using a (non-attacking) abilty while invisible, the invisibility is not removed properly. In the GenericAbility.Apply functions, the RemoveInvisibility function is called. If the GenericAbility is not passive, it will then clear all ModifiedStat.Invisible effects. However, abilities that grant Invisibility also set NonTargetable and NonEngageable ModifiedStats, which are not removed by this RemoveInvisibility function. For attacking abilities (ie. anything that contains an AttackBase (or derived) objects, so this is most of them) this doesn't matter much, since the three StatusEffects are all marked OneHitUse so are cleaned up after an attack anyway. But after non-attacking abilities (eg. activating a modal, drinking a potion, Rhymer's Summon) this doesn't happen, which means the character becomes visible but is still not targetable or engageable; effectively, the character is visible to the player but still invisible to other characters (including your own, by the way). An exception to this are Teleport abilities (eg. Escape), since these use Invisibilty themselves and explicitly make the character targetable and engageable again because of that.

 

One possibility would be to change the RemoveInvisibility function and have it clear NonEngageable and NonTargetable as well. However, this would remove those even if they are caused by another (non-invisibility) effect, since there is no way to tell whether they originated from an Invisibility-type ability. Moreover, from a gameplay perspective you have to wonder whether fixing it is even desirable. After all, wouldn't it make more sense if activating a modal ability or drinking a potion *would* leave you invisible? And anything with an attack in it makes you visible again already anyway. Rhymer's summon (and other summoning, from figurines) might be a bit more borderline in this regard, but that's only a couple of them.

 

I would therefore suggest removing the RemoveInvisibility call from the GenericAbility.Apply function instead. Firstly, this avoids the issue with fixing it above. Secondly, this clarifies the situation with regards to non-attacking abilities: as is, they effectively leave you invisible anyway, the player just doesn't know it because the character is rendered as visible again. Seems better to just make that official, by keeping the character visually invisible as well (for lack of a better description). Thirdly, this also fixes the bug with Backstab, where if you are Invisible and use a Strike ability you don't get the Backstab bonus, because the Strike ability will call the GenericAbility.Apply function before the actual attack occurs, which means that by the time the attack hits the character is visible again as far as the Backstab ability is concerned. 

 

If the RemoveInvisibility call is removed, I would also suggest changing the line "this.m_ownerStats.InvisibilityState--;" into "this.m_ownerStats.InvisibilityState = 0;" in the TeleportAbility.BecomeVisible function, since otherwise with eg. Escape while Invisible you could end up with the reverse situation (character is rendered as invisible for the player, but is targetable and engageable by other characters; in the present situation this wouldn't happen because activating Escape would call RemoveInvisibility and clear all other Invisible effects first). 

Edited August 20, 2016 by Loren Tyr

[Boeroer]

I so support this: clear that invisible mess up please!

 

Also, the line "this.m_ownerStats.InvisibilityState--;"

is worth a hefty facepalm...

[Loren Tyr]

I so support this: clear that invisible mess up please!

 

Also, the line "this.m_ownerStats.InvisibilityState--;"

is worth a hefty facepalm...

 

It makes sense in context though, to some degree. Generally, the ModifiedStat.Invisible effects increment the invisibility state by one and decrement by one upon application/removal, such that you only become visible again if all invisibility effects are gone. The problem is that the NonTargetable and NonEngageable effects are separate, rather than being keyed directly into the invisibility, and don't use this increment/decrement approach either. In that regard it is strange that the TeleportAbility does decrement InvisibilityState, because it sets the corresponding non-targetable and non-engageable booleans to false a couple of lines below it. It only works properly now because at present the InvisibilityState is set to zero (by decrements of one ) by the RemoveInvisibility function, since the TeleportAbility derives from GenericAbility.

[Boeroer]

Nah, incrementing or decrementing something that is boolean in it's nature (being invisible, being alive, being pregnant...) is worth a facepalm. There's no need to beat around the bush. Any solution that uses more than one state of invisibility when in the game you can either be completely invisible or completely visible is a bad solution.

[Loren Tyr]

Sure, but in this case there could be multiple effects that could grant this invisibility, all of which need to expire before you become visible again. So you'd have to enumerate the number of invisibilities at some point anyway (and given how the code is structured, this is probably the best way to do that). As outlined above, in the TeleportAbility you see that going wrong for the nontargetable and nonengageable properties, which *are* boolean and are set to false there even though there could have been other effects that grant those properties still active (currently, this wouldn't be the case in practice because of the call to RemoveInvisibility when the ability gets activated, but relying on that is quite unsafe). So in the context of the current system, the best way to handle it would probably be to make those integers as well, incrementing and decrementing them alongside the invisibilityState. And more generally, to remove them as seperate StatusEffects granted by Shadowing Beyond etc., but incrementing and decrementing them whenever a ModifiedStat.Invisible gets applied or cleared (and having eg. the TeleportAbility use the ClearEffect function rather than doing the decrementing itself).

 

Which isn't to say that I would design the general structure of the code in this way in the first place, for my liking things get pushed down to the level of disconnected StatusEffects far too much, making it difficult to keep track of where they come from and what they are supposed to apply to (this is also why eg. Consecrated Ground + Knockdown is a thing). I'd keep it much more relational. But that's a different (albeit interesting ) discussion.

[Boeroer]

When I want to have many things that turn the char invisible then I still don't need to increment the invisibility status. It can have a duration that gets overwritten when another ability that would also turn you invisible has a longer duration. I see no point in incrementing the invisible status. If you do something that breaks invisibility it should turn you from invisible to visible and done. It's setting a variable like "is_visible" from false to true and that should do it.

 

I get it that there might be other effects attached to the ability that turns you invisible which can also be attached to other abilities - and which should not be turned off when invisibility vanishes. Your last paragraph is a good answer to that. Or to use decorators to implement afflictions/status effects instead of incremented status variables.