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 loot - use proper monster level for generating items #2763

Merged
merged 1 commit into from
Sep 6, 2021

Conversation

qndel
Copy link
Member

@qndel qndel commented Aug 31, 2021

This is the most important PR ever - this is the reason why loot was the same on all difficulties, hell, it was even worse than that, despite unique monsters having a custom data table with lvls and level getting increased by 4 for item generating, it was always taking the level of base monster straight from the data table - means no bonus from difficulty, no bonus from being unique xD

I did the math for lazarus (30 lvl) on hell (+30 lvl)
that alone brings him to 60, he has 0 in unique data level, so that's a +5 bonus and there's a flat +4 bonus hardcoded on top of that so final level would be 69.
According to

enum icreateinfo_flag {
	// clang-format off
	CF_LEVEL        = (1 << 6) - 1,

Max item level can be 63 (not sure if before or after that hardcoded +4 bonus), so we'd need to cap the level to max range.

#Added capping the level to 63, hardcoded +4 for bosses happens after the level gets loaded so we effectively have max lvl 67 items!

@qndel qndel added enhancement New feature or request vanilla Bugs found also in the original Diablo 1.09b release fix fix for a bug critical A bug that is super serious labels Aug 31, 2021
AJenbo
AJenbo previously approved these changes Aug 31, 2021
Copy link
Member

@AJenbo AJenbo left a comment

Choose a reason for hiding this comment

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

😭

@qndel qndel added this to the 1.3.0 milestone Aug 31, 2021
@qndel
Copy link
Member Author

qndel commented Aug 31, 2021

If this gets merged, it really requires a big fat section in the changelog because I just can't stress enough how important this is 😅

@julealgon
Copy link
Contributor

Would be nice to have more proper documentation for this. I don't understand how something can be a "fix" and an "enhancement" at the same time, for instance. Looks like we are touching on a bunch of issues at the same time.

I've personally never heard of any of this before (I don't play nightmare/hell much if at all).

@qndel
Copy link
Member Author

qndel commented Aug 31, 2021

Would be nice to have more proper documentation for this. I don't understand how something can be a "fix" and an "enhancement" at the same time, for instance. Looks like we are touching on a bunch of issues at the same time.

I've personally never heard of any of this before (I don't play nightmare/hell much if at all).

People have always felt that difficulty didn't matter, so best recommended farming spot was normal lazarus and this piece of code explains why, as difficulty was literally skipped in final loot math.

And I've added as many labels as possible to show important this thing is xD
It's a fix that changes literally half of the game, can't it be an enhancement too? 🤪

@galaxyhaxz
Copy link
Member

Yeah, it's a well known fact that despite having a +15/30 level bonus for nightmare and hell respectively, monsters use their normal level on all 3 for dropping loot. This means all those level 60+ monsters on hell can't drop the good stuff wirt can sell you.

@qndel
Copy link
Member Author

qndel commented Aug 31, 2021

Yes but then everyone knew it, nobody investigated why 😅

@galaxyhaxz
Copy link
Member

Yes but then everyone knew it, nobody investigated why

I did more than 2 years ago. Can't remember if I posted it on github or our chat, but it was the exact same change as your PR: update the level in SpawnItem :D

I think it caused item morphing or something along those lines.

@julealgon
Copy link
Contributor

I think it caused item morphing or something along those lines.

😬

@qndel
Copy link
Member Author

qndel commented Aug 31, 2021

I think it caused item morphing or something along those lines.

😬

Doesn't cause any morphing for me and shouldn't, since it doesn't affect recreating the item but only item generation.

@AJenbo
Copy link
Member

AJenbo commented Aug 31, 2021

I did more than 2 years ago. Can't remember if I posted it on github or our chat, but it was the exact same change as your PR: update the level in SpawnItem :D

Good thing we have tests for that in the CI :P

@AJenbo
Copy link
Member

AJenbo commented Aug 31, 2021

If this gets merged

Why wouldn't it?

@qndel qndel force-pushed the fix_loot_generation branch 2 times, most recently from cb56a9e to 710aeb8 Compare August 31, 2021 19:43
@galaxyhaxz
Copy link
Member

Good thing we have tests for that in the CI :P

Yeah it would have been a godsend to have the power and test cases in DevilutionX 3 years ago. Stuff used to break left and right lol.

Also been talking with Qndel, I believe the item level cap of 63 is what causes some items to morph. When I tried fixing this years ago I tested the top tier monsters (lazarus hell) so capping it to 63 should fix the problem!

@AJenbo
Copy link
Member

AJenbo commented Aug 31, 2021

Yeah it would have been a godsend to have the power and test cases in DevilutionX 3 years ago. Stuff used to break left and right lol.

We still badly need tests for the level generation and some for game play. But it shoudent be fare off at this point

@galaxyhaxz
Copy link
Member

We still badly need tests for the level generation and some for game play

Yeah DRLG tests would be very nice, especially since there are still maps with unreachable areas.

Also I tested thoroughly and can confirm that items dropped from monsters over level 63 will morph on new games. With capping it to 63, everything works perfectly. So, hooray, now you can finally find that godly plate of the whale.

Too bad the only level 60+ monsters in the game are soul burners, blood knights, and advocates. You also have any unique monster on level 15 as well as Lazarus and his two succubi.

@AJenbo
Copy link
Member

AJenbo commented Sep 1, 2021

I just can't stress enough how important this is sweat_smile

Yet the title says nothing about what this does, maybe you fixed a debug command 🤷

@qndel qndel changed the title fix loot fix loot - use proper monster level for generating items Sep 1, 2021
@AJenbo AJenbo merged commit 197d3ed into diasurgical:master Sep 6, 2021
@agris-codes
Copy link

agris-codes commented Oct 2, 2021

What’s the item quality floor for drops from these high mlvl creatures, is it 1? mlvl/4? I ask because, while this is a huge improvement to motivating people to play the harder difficulties and let the itemization system really shine, I’m reminded of the fact that some of the best base level items don’t have affix combos that take them above ilvl 25 or. This combined with vendor ilvl selection results in some weirdness where the player can actually level out of ever finding the “best” non-unique shields, for example.

The point I’m trying to motivate with my aside about vendors is that, if you’re fixing this issue can it be done such there isn’t the filtering of best-of-type magic items that don’t have very high ilvl permutations? Obsidian tower shield of the ages is one of those iirc.

edit: I may mean qlvl rather than ilvl, it’s been a few months since I was reading about this.

@galaxyhaxz
Copy link
Member

@agris-codes diasurgical/devilution#64 (comment)

Posted more in-depth info a week ago. Long story short, things like Obsidian can't spawn from monsters in late Hell and staffs also stop spawning with suffixes. It's because affixes are capped to a minimum of monster level / 2 which was intended to cut out crap affixes later on, but ends up botching good ones too.

@agris-codes
Copy link

@agris-codes diasurgical/devilution#64 (comment)

Posted more in-depth info a week ago. Long story short, things like Obsidian can't spawn from monsters in late Hell and staffs also stop spawning with suffixes. It's because affixes are capped to a minimum of monster level / 2 which was intended to cut out crap affixes later on, but ends up botching good ones too.

Since you all are fixing parts of this, perhaps the qlvl floor could be reduced to monster_level / 3? That means the level 64 monsters can still drop something with a qlvl of 21, and iirc it is certain shields you can level out of finding, with a qlvl of ~24. I can check Jarulf’s guide to post some specific examples if y’all want.

@qndel
Copy link
Member Author

qndel commented Oct 9, 2021

@agris-codes diasurgical/devilution#64 (comment)
Posted more in-depth info a week ago. Long story short, things like Obsidian can't spawn from monsters in late Hell and staffs also stop spawning with suffixes. It's because affixes are capped to a minimum of monster level / 2 which was intended to cut out crap affixes later on, but ends up botching good ones too.

Since you all are fixing parts of this, perhaps the qlvl floor could be reduced to monster_level / 3? That means the level 64 monsters can still drop something with a qlvl of 21, and iirc it is certain shields you can level out of finding, with a qlvl of ~24. I can check Jarulf’s guide to post some specific examples if y’all want.

I'm actually suggesting reverting this fix for 1.3.0 as it makes uniques ,that dropped only on nightmare/hell because of the bug that this PR fixes, undroppable at all, so should be combined with the fix that makes all uniques droppable, so either they both get into 1.3.0 or both wait for 1.4.0 . Changing the qlvl floor would make all vanilla items morph when transferred to devilutionx, it's not something to be taken lightly.

@AJenbo
Copy link
Member

AJenbo commented Oct 9, 2021

Alright, in that case let's revert this one for now, we are to close to release to experiment.

@qndel
Copy link
Member Author

qndel commented Oct 9, 2021

Reverted in #3064 and moved to #2930

@NiteKat
Copy link
Contributor

NiteKat commented Oct 9, 2021

@agris-codes diasurgical/devilution#64 (comment)

Posted more in-depth info a week ago. Long story short, things like Obsidian can't spawn from monsters in late Hell and staffs also stop spawning with suffixes. It's because affixes are capped to a minimum of monster level / 2 which was intended to cut out crap affixes later on, but ends up botching good ones too.

Is it really so bad if some affixes cannot be found in late Hell? I will admit Obsidian seems a bit rough to lose, but then if you want Obsidian you should start hunting other areas? Though NO suffixes on staves at all is kinda bad. I like the recommendation of changing to monsterLevel / 3. You're already tweaking the balance of the game by fixing this issue, you might as well tweak it further. OR you could change the levels for affixes? I'll be sad if this doesn't make 1.3.0, but it would make sense to hold off for now.

@galaxyhaxz
Copy link
Member

You're already tweaking the balance of the game by fixing this issue, you might as well tweak it further

There is a workaround to fix the no-affix issue while keeping the original design. You could add code to the affix function to check if an affix was picked. if no affixes were selected, change the minimum level to 1 and start the function over. This would ensure staffs that pick a suffix always get one instead of spawning white.

The no-obsidian issue is a different beast. I'm speculating how to fix this in my mod and basically you have these options:

  • Add a maximum level each affix can spawn rather than using monster level.
  • Only enforce minimum affix level on cursed affixes, so the worst ones stop spawning later on.
  • Remove the minimum level altogether so Hell Baal drops rares with level 2 firebolt charges and +1 strength like in Diablo 2.

Tricky, tis it not?

@NiteKat
Copy link
Contributor

NiteKat commented Oct 9, 2021

Remove the minimum level altogether so Hell Baal drops rares with level 2 firebolt charges and +1 strength like in Diablo 2.

Hell Baal in D1? What are you doing with your mod? 😛

@galaxyhaxz
Copy link
Member

Hell Baal in D1? What are you doing with your mod?

Hahaha! No it's a joke in reference to d2. You'd expect Hell Baal to drop good stuff but most magic items/rares he drops wind up having really low level crap affixes. D2 has weighted rarity that makes the best affixes harder to find, on top of no minimum level.

@agris-codes
Copy link

agris-codes commented Oct 10, 2021

@agris-codes diasurgical/devilution#64 (comment)
Posted more in-depth info a week ago. Long story short, things like Obsidian can't spawn from monsters in late Hell and staffs also stop spawning with suffixes. It's because affixes are capped to a minimum of monster level / 2 which was intended to cut out crap affixes later on, but ends up botching good ones too.

Since you all are fixing parts of this, perhaps the qlvl floor could be reduced to monster_level / 3? That means the level 64 monsters can still drop something with a qlvl of 21, and iirc it is certain shields you can level out of finding, with a qlvl of ~24. I can check Jarulf’s guide to post some specific examples if y’all want.

Changing the qlvl floor would make all vanilla items morph when transferred to devilutionx, it's not something to be taken lightly.

Are you sure about this? I do not know anywhere near the same depth or breadth of the game’s underpinnings like y’all do, but this seems wrong. Why would the criteria for an item dropping change its data structure and reference such that morphing occurs?

@galaxyhaxz
Copy link
Member

Why would the criteria for an item dropping change its data structure and reference such that morphing occurs?

Because items are packed and then recreated. Only some very basic info is stored like item seed and flags. Whenever the recreation routine occurs, everything must take the same code path or the item will be different.

@agris-codes
Copy link

agris-codes commented Oct 10, 2021

Damn. That is both efficient, and maddening.

Would such a change only introduce the possibility of items morphing when going from a DX save to vanilla, or could it occur on vanilla to DX as well? Are saves going across platforms perfectly pristine right now, with no hiccups such as this? I ask because it helps evaluate the impact of suggesting this change. If there are already issues with saves across platforms, it is less of a "big deal".

If bidirectional transfer of saves currently works perfectly, then it is indeed a big deal to break that.

@qndel
Copy link
Member Author

qndel commented Oct 10, 2021

Damn. That is both efficient, and maddening.

Would such a change only introduce the possibility of items morphing when going from a DX save to vanilla, or could it occur on vanilla to DX as well? Are saves going across platforms perfectly pristine right now, with no hiccups such as this? I ask because it helps evaluate the impact of suggesting this change. If there are already issues with saves across platforms, it is less of a "big deal".

If bidirectional transfer of saves currently works perfectly, then it is indeed a big deal to break that.

How about chatting on discord? Issues/PRs aren't meant for that ...

@julealgon
Copy link
Contributor

Issues/PRs aren't meant for that ...

What are issues for, then?

@AJenbo
Copy link
Member

AJenbo commented Oct 10, 2021

They should be limited to the specific subject so as to allow for an easy overview with out meta topics taking up a lot of space. Things like that are better done on discord or by creating a new issue for the new subject.

@qndel
Copy link
Member Author

qndel commented Dec 1, 2022

Reverted in #3064 and moved to #2930 - reposting for visibility because covered by a ton of comments ...

@yuripourre
Copy link
Collaborator

For one second I thought the 2930 was merged.

@diasurgical diasurgical locked as off-topic and limited conversation to collaborators Dec 1, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
critical A bug that is super serious enhancement New feature or request fix fix for a bug vanilla Bugs found also in the original Diablo 1.09b release
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants