Sculk removed off xp_step and triggered by player and mob death [Performance] #3545

ancientmarinerdev · 2023-03-14T22:42:02+01:00

ancientmarinerdev commented

2023-03-14 22:42:02 +01:00

Testing

Kill mob near sculk catalyst - Don't drop xp, sculk expands
Kill mob far from sculk catalyst - Drop xp, sculk expands
Player death near sculk expands sculk
Player death not near sculk doesn't expand sculk

Fixes #3394 ### Testing Kill mob near sculk catalyst - Don't drop xp, sculk expands Kill mob far from sculk catalyst - Drop xp, sculk expands Player death near sculk expands sculk Player death not near sculk doesn't expand sculk

👍 1

ancientmarinerdev added the

#P2: HIGH

mobs

non-mob entities

labels 2023-03-14 22:42:03 +01:00

ancientmarinerdev commented

2023-03-15 02:36:10 +01:00

I dropped 10 zombies for an iron golem. He smashed them and they dropped XP (I know, it's a bug, but I'll exploit for testing purposes :)).

On master, mobs_step and xp take around 8.5% xp each. On the fix branch, mobs take 6% and xp, it's gone.

Main register method was taking 69%, afterwards, 78%.

This is ready for review.

I dropped 10 zombies for an iron golem. He smashed them and they dropped XP (I know, it's a bug, but I'll exploit for testing purposes :)). On master, mobs_step and xp take around 8.5% xp each. On the fix branch, mobs take 6% and xp, it's gone. Main register method was taking 69%, afterwards, 78%. This is ready for review.

after_sculk_fixes.svg

110 KiB

before_sculk_fixes.svg

112 KiB

ancientmarinerdev changed title from ~~WIP: Sculk removed off xp_step and triggered by player and mob death [Performance]~~ to Sculk removed off xp_step and triggered by player and mob death [Performance]

2023-03-15 02:37:02 +01:00

ancientmarinerdev commented

2023-03-15 02:44:59 +01:00

Oh, and just for reference, I profiled 0.82.0 in the same situation. 42.78 for main register method. 14% skulk, 22% kelp! This is 36% improvement next to ocean with 10 zombies worth of xp on the ground!

0_82_0.svg

126 KiB

🎉 1

MrRar requested changes 2023-03-15 18:59:10 +01:00

MrRar left a comment

When the player dies XP is dropped and sculk is created.

In creative mode killing mobs also creates sculk and drops XP.

I don't think it is very important but this implementation is very different from Minecraft in that newly generated sculk does not need to touch existing sculk.

When the player dies XP is dropped and sculk is created. In creative mode killing mobs also creates sculk and drops XP. I don't think it is very important but this implementation is very different from Minecraft in that newly generated sculk does not need to touch existing sculk.

SmokeyDope commented

2023-03-15 20:41:16 +01:00

A little thought I just had while reading over this and the other skulk performance issue. Many minecraft mods exist to improve performance by combining many small xp orbs into one big orb. I know that this isn't a vanilla mechanic, but perhaps its worth a thought?

ancientmarinerdev referenced this pull request

2023-03-16 23:09:08 +01:00

Sculk issues #3552

ancientmarinerdev commented

2023-03-16 23:17:02 +01:00

When the player dies XP is dropped and sculk is created.

In creative mode killing mobs also creates sculk and drops XP.

I don't think it is very important but this implementation is very different from Minecraft in that newly generated sculk does not need to touch existing sculk.

Thanks for testing this and raising these issues.

I have fixed the mobs issue dropping xp in creative.

The other two are a little more tricky and I am not sure of the best course of action. I wanted to get this into the next release which is likely soon (due to pending MT 5.7 release which is in feature freeze), and the performance impact is a big issue for most players.

I have created a new ticket and put it as an elevated priority.

MineClone2/MineClone2#3552

I would like to revisit those at a later date as performance issues for most is a bigger issue than a niche end game block that many won't play with for a while. I'm not even sure if I have seen it in game via mapgen yet so the impact could be very low.

> When the player dies XP is dropped and sculk is created. > > In creative mode killing mobs also creates sculk and drops XP. > > I don't think it is very important but this implementation is very different from Minecraft in that newly generated sculk does not need to touch existing sculk. Thanks for testing this and raising these issues. I have fixed the mobs issue dropping xp in creative. The other two are a little more tricky and I am not sure of the best course of action. I wanted to get this into the next release which is likely soon (due to pending MT 5.7 release which is in feature freeze), and the performance impact is a big issue for most players. I have created a new ticket and put it as an elevated priority. https://git.minetest.land/MineClone2/MineClone2/issues/3552 I would like to revisit those at a later date as performance issues for most is a bigger issue than a niche end game block that many won't play with for a while. I'm not even sure if I have seen it in game via mapgen yet so the impact could be very low.

MrRar approved these changes 2023-03-17 19:24:34 +01:00

MrRar left a comment

It's fine expect for the player dropping XP and producing skulk. Another problem is that the amount of sculk the player produces is fixed. I think it should be based on the players actual experience count. If you did that and then had mcl_experience depend on mcl_sculk, mcl_sculks on_dieplayer will get run first so you can remove all the XP from the player if sculk is produced. When mcl_experience on_dieplayer runs, player will have no XP and no orbs will be dropped.

Or you can have mcl_experience trigger the creation of sculk directly which is probably a better option.

It's fine expect for the player dropping XP and producing skulk. Another problem is that the amount of sculk the player produces is fixed. I think it should be based on the players actual experience count. If you did that and then had mcl_experience depend on mcl_sculk, mcl_sculks on_dieplayer will get run first so you can remove all the XP from the player if sculk is produced. When mcl_experience on_dieplayer runs, player will have no XP and no orbs will be dropped. Or you can have mcl_experience trigger the creation of sculk directly which is probably a better option.

ancientmarinerdev referenced this pull request

2023-03-17 21:33:41 +01:00

Sculk issues #3552

ancientmarinerdev commented

2023-03-17 21:35:55 +01:00

It's fine expect for the player dropping XP and producing skulk. Another problem is that the amount of sculk the player produces is fixed. I think it should be based on the players actual experience count. If you did that and then had mcl_experience depend on mcl_sculk, mcl_sculks on_dieplayer will get run first so you can remove all the XP from the player if sculk is produced. When mcl_experience on_dieplayer runs, player will have no XP and no orbs will be dropped.

Or you can have mcl_experience trigger the creation of sculk directly which is probably a better option.

Fair points. I have added these into the ticket. I think you've got a strong grasp on player related stuff so that'll definitely help with the issue resolution.

Thanks for reviewing :). It's always appreciated.

> It's fine expect for the player dropping XP and producing skulk. Another problem is that the amount of sculk the player produces is fixed. I think it should be based on the players actual experience count. If you did that and then had mcl_experience depend on mcl_sculk, mcl_sculks on_dieplayer will get run first so you can remove all the XP from the player if sculk is produced. When mcl_experience on_dieplayer runs, player will have no XP and no orbs will be dropped. > > Or you can have mcl_experience trigger the creation of sculk directly which is probably a better option. > Fair points. I have added these into the ticket. I think you've got a strong grasp on player related stuff so that'll definitely help with the issue resolution. Thanks for reviewing :). It's always appreciated.

👍 1

ancientmarinerdev force-pushed sculk_performance_fix from 518dd07781 to ef633ce617

2023-03-17 21:37:03 +01:00

Compare

ancientmarinerdev added this to the 0.83.0 - Safe and Sound milestone 2023-03-17 21:41:58 +01:00

ancientmarinerdev referenced this issue from a commit

2023-03-17 21:42:24 +01:00

Merge pull request 'Sculk removed off xp_step and triggered by player and mob death [Performance]' (#3545) from sculk_performance_fix into master

ancientmarinerdev merged commit d437f45f4a into master

2023-03-17 21:42:25 +01:00

ancientmarinerdev deleted branch sculk_performance_fix

2023-03-17 21:42:39 +01:00