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

Core/Spells: Fixed work of sobering spells and other improvements for drunk system. #29745

Open
wants to merge 2 commits into
base: 3.3.5
Choose a base branch
from

Conversation

r4d1sh
Copy link
Contributor

@r4d1sh r4d1sh commented Feb 24, 2024

Changes proposed:

The first commit related to SPELL_EFFECT_INEBRIATE (100) effect. The problem is that there are not only inebriation but also sobering spells (with basepoint < 0). The current implementation uses an unsigned type, ignoring sobering spells. This leads to a uint8 underflow, resulting in the spell's behavior being completely unexpected: it can both inebriate and sober you up.

Example:

  1. .gobject add temp 185914
  2. Use it!
  3. If you were completely sober before, you will now get maximum inebriation. Although coffee was supposed to sober you up by 20%. This happened because of uint8 underflow.

There is another problem: there are spells with an inebriate effect with basepoint values so large or small that it inevitably leads to uint8 overflow/underflow. For example, 29690 or 46874. As intended, 29690 should make you extremely drunk, but in reality, the behavior can be unpredictable each time. If someone keeps casting this spell on you again and again, you will likely notice that sometimes you get drunk and sometimes you sober up.

The first commit completely fixes this issues.

The second commit is related to the aura effect SPELL_AURA_MOD_FAKE_INEBRIATE (304). The main problem is that the handler of this effect ignores the value of player's actual inebriation. In any case, there was a lot of redundant (duplicate) code here. Now the logic for updating invisibility detect has been moved to a separate method, and all known issues have been fixed.

Tests performed:

Builded and tested in-game.

@r4d1sh r4d1sh changed the title Inebriate Core/Spells: Fixed work of sobering spells and other improvements for drunk system. Feb 24, 2024
@Ovahlord
Copy link
Contributor

Ovahlord commented Feb 24, 2024

the sobering part has been fixed in master branch by me already. so cherrypick that commit.
45333fc

@r4d1sh
Copy link
Contributor Author

r4d1sh commented Feb 24, 2024

the sobering part has been fixed in master branch by me already. so cherrypick that commit. 45333fc

Oh, I don't think the overflow issue is completely resolved here. Unless we hope that no one will ever create a spell with basepoint = (2147483547; 2147483647]

image

@Ovahlord
Copy link
Contributor

this has not been a problem throughout all expansions so far so I doubt that this will ever happen.

@r4d1sh
Copy link
Contributor Author

r4d1sh commented Feb 24, 2024

this has not been a problem throughout all expansions so far so I doubt that this will ever happen.

Yes, but why do we need doubts if there is ready-made code that solves the problem completely ¯_(ツ)_/¯
However, I am only suggesting and not insisting on anything.

@Ovahlord
Copy link
Contributor

just change the int32 to int64 and your problems are gone.

@r4d1sh
Copy link
Contributor Author

r4d1sh commented Feb 24, 2024

just change the int32 to int64 and your problems are gone.

Yes, but why should I do this if ready-made code has already been written that completely solves the problem. Not mine btw xd

@CraftedRO
Copy link
Contributor

Just tested this pr but doesn't seem to work with the given example: https://youtu.be/4N-zMfxoigQ , maybe I'm missing smth

@r4d1sh
Copy link
Contributor Author

r4d1sh commented May 26, 2024

Just tested this pr but doesn't seem to work with the given example: https://youtu.be/4N-zMfxoigQ , maybe I'm missing smth

You drink a sobering coffee while being completely sober, and as expected, nothing happens

@CraftedRO
Copy link
Contributor

ah I got it, I have to drink smth before, well now it works: https://youtu.be/gdjO3SZGrRs

@CraftedRO
Copy link
Contributor

maybe we'll see this man's work merged this decade, just saying 😏

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.

None yet

4 participants