More randomness for slime chunks #4466

Open
kno10 wants to merge 1 commits from kno10/VoxeLibre:slime-spawning into master
First-time contributor

The old code caused a diagonal pattern. If some x,z was a slime chunk, then so was x+1,z-1.
Use a classic pseudo-random hashing approach instead, by multiplication of chunk numbers
with large primes that should be more random.

  • make slime density (as 1 in N) and maximum light level (default: no limit) configurable
  • Allow using a 3d chunking system where y is also used for hashing

This pull does not modify spawn frequency, only the chunk logic.

Because this replaces the computation of slime chunks, do not expect slimes to spawn in the same places as before. For testing, you can increase the slime chunk density, e.g., to 2 (every other chunk). There is a commented log that will print, e.g., Chunk 1182545475 is a slime chunk: false and Chunk 2924583950 is a slime chunk: true

The old code caused a diagonal pattern. If some x,z was a slime chunk, then so was x+1,z-1. Use a classic pseudo-random hashing approach instead, by multiplication of chunk numbers with large primes that should be more random. - make slime density (as 1 in N) and maximum light level (default: no limit) configurable - Allow using a 3d chunking system where y is also used for hashing This pull does *not* modify spawn frequency, only the chunk logic. Because this replaces the computation of slime chunks, do not expect slimes to spawn in the same places as before. For testing, you can increase the slime chunk density, e.g., to 2 (every other chunk). There is a commented log that will print, e.g., `Chunk 1182545475 is a slime chunk: false` and `Chunk 2924583950 is a slime chunk: true`
kno10 added 1 commit 2024-06-26 15:38:27 +02:00
ee0db05876 More randomness for slime chunks
The old code caused a diagonal pattern. If some x,z was a slime chunk, then so was x+1,z-1.
Use a classic pseudo-random hashing approach instead, by multiplication of chunk numbers
with large primes that should be more random.
Also make slime density and light level configurable.

Allow using a 3d chunking system where y is also used.
the-real-herowl requested changes 2024-06-26 16:47:07 +02:00
the-real-herowl left a comment
Owner

Aside of the typo in a comment, lgtm... as long as slimes don't spawn on the surface more than they do without this patch.

Haven't tested yet.

Aside of the typo in a comment, lgtm... as long as slimes don't spawn on the surface more than they do without this patch. Haven't tested yet.
@ -9,0 +13,4 @@
local slime_max_light = (tonumber(minetest.settings:get_bool("mcl_mobs_slime_max_light", true)) or minetest.LIGHT_MAX) + 1
-- maximum light level for swamp spawning
local swamp_light_max = 7
-- maximum highzt to spawn in slime chunks

Typo: height not highzt?

Typo: `height` not `highzt`?
rudzik8 marked this conversation as resolved
the-real-herowl added the
mobs
code quality
labels 2024-06-26 16:59:03 +02:00
kno10 force-pushed slime-spawning from ee0db05876 to 0edcfce8d4 2024-06-26 22:03:23 +02:00 Compare
Author
First-time contributor

Fixed the typo, thanks.

Fixed the typo, thanks.
Member

Fixed the typo, thanks.

Next time you fix something, please mark the conversation as 'Resolved'.

> Fixed the typo, thanks. Next time you fix something, please mark the conversation as 'Resolved'.
rudzik8 requested changes 2024-06-27 05:26:36 +02:00
rudzik8 left a comment
Member

Here's your review.

I haven't been able to get any slimes to spawn. Perhaps it's because of all the settings being so broken?

Here's your review. I haven't been able to get any slimes to spawn. Perhaps it's because of all the settings being so broken?
@ -9,0 +8,4 @@
-- slime density, where default N=10 is every 10th chunk
local slime_density = math.floor(tonumber(minetest.settings:get("mcl_mobs_slime_density")) or 10)
-- use 3D chunking instead of 2d chunks
local slime_3d_chunks = minetest.settings:get("mcl_mobs_slime_3d_chunks") or false
Member
  1. Getting boolean values through get method is... bizarre.
  2. get_bool can handle default values as its second argument, see http://api.minetest.net/class-reference/#methods_16.
1. Getting boolean values through `get` method is... bizarre. 2. `get_bool` can handle default values as its second argument, see http://api.minetest.net/class-reference/#methods_16.
kno10 marked this conversation as resolved
@ -9,0 +10,4 @@
-- use 3D chunking instead of 2d chunks
local slime_3d_chunks = minetest.settings:get("mcl_mobs_slime_3d_chunks") or false
-- maximum light level, for slimes in caves only, not magma/swamps
local slime_max_light = (tonumber(minetest.settings:get_bool("mcl_mobs_slime_max_light", true)) or minetest.LIGHT_MAX) + 1
Member

So now you're getting an integer value through get_bool, with true as the default value. O_o

So now you're getting an integer value through `get_bool`, with `true` as the default value. O_o
Author
First-time contributor

LOL, put that into the wrong line...

LOL, put that into the wrong line...
kno10 marked this conversation as resolved
@ -88,2 +27,2 @@
assert(calculate_chunk_value(vector.new(16,0,64)) == (3), "calculate_chunk_value failed")
assert(calculate_chunk_value(vector.new(-160,0,-160)) == 0, "calculate_chunk_value failed")
local math_floor = math.floor
local math_max = math.max
Member

Why localize these functions if you're already using them unlocalized a few lines above (when handling settings)?

Why localize these functions if you're already using them unlocalized a few lines above (when handling settings)?
Author
First-time contributor

Following this pattern from other modules, although the minetest "vector" class does not seem to bother.
My guess was that this can help with performance in functions that are frequently called (as it is not necessary to check if someone modified math.floor in the meantime).
For the settings, which supposedly is only executed once and hence not performance critical, I would prefer the standard version.
But I have to rely on your experience whether localizing is beneficial or not.

Following this pattern from other modules, although the minetest "vector" class does not seem to bother. My guess was that this can help with performance in functions that are frequently called (as it is not necessary to check if someone modified math.floor in the meantime). For the settings, which supposedly is only executed once and hence not performance critical, I would prefer the standard version. But I have to rely on your experience whether localizing is beneficial or not.
Member

I did a profile of this recently (local access compared to accessing a global) and accessing a local was about 3 times faster on my hardware than a simple global access (see #4338 (comment)). I haven't profiled it this specific thing, but I would expect accessing a field thru a global would be a bit less than an additional 2x. But even then, we are talking about 3.1 ns for a local access and an estimated 16.3 ns (2 * (9.7 - 3.1) + 3.1) for the non-local access so unless this is in an inner loop it probably doesn't matter that much. Defaulting to the faster method when they behave (or should behave) the same is a good habit to get into though.

Probably easy enough just to move the local variable assignment to near the top of the file and use it everywhere.

I did a profile of this recently (local access compared to accessing a global) and accessing a local was about 3 times faster on my hardware than a simple global access (see https://git.minetest.land/VoxeLibre/VoxeLibre/pulls/4338#issuecomment-80333). I haven't profiled it this specific thing, but I would expect accessing a field thru a global would be a bit less than an additional 2x. But even then, we are talking about 3.1 ns for a local access and an estimated 16.3 ns (2 * (9.7 - 3.1) + 3.1) for the non-local access so unless this is in an inner loop it probably doesn't matter that much. Defaulting to the faster method when they behave (or should behave) the same is a good habit to get into though. Probably easy enough just to move the local variable assignment to near the top of the file and use it everywhere.
rudzik8 marked this conversation as resolved
settingtypes.txt Outdated
@ -224,0 +224,4 @@
# Slime chunk density
mcl_mobs_slime_density (Slime chunk density, default is one in ten, 0 to disable cave slime) int 10 0
# Use 3d chunking instead of 2d chunking
mcl_mobs_slime_3d_chunks (Use 3d chunking instead of 2d chunking) bool false
Member

Reduce all of these settings to from mcl_mobs_slime_... to slime_... (and don't forget to update the .lua file for that). Explanation:

  1. Slimes have nothing to do with mcl_mobs. They are defined in mobs_mc.
  2. Wither-related settings (see L183-199) do the same thing with their namespace and it's good to be consistent.

Also, consider making the visible names of these settings a lot shorter and prepending all of them with "Slime". If the 3D/2D chunking is only relevant for slimes, it should be apparent for the player that it's only relevant for slimes.

image

The names get cut off, and it's even worse when you're using a custom font.

UPD: I think I know what the issue was. You've mixed up text in comment (which is the description for a setting) and in brackets (which is the visible name of a setting). Still, you don't have to repeat the same thing for slime_3d_chunks name and description if there's no more info to add.

Reduce all of these settings to from `mcl_mobs_slime_...` to `slime_...` (and don't forget to update the .lua file for that). Explanation: 1. Slimes have nothing to do with `mcl_mobs`. They are defined in `mobs_mc`. 2. Wither-related settings (see L183-199) do the same thing with their namespace and it's good to be consistent. Also, consider making the visible names of these settings a lot shorter and prepending all of them with "Slime". If the 3D/2D chunking is only relevant for slimes, it should be apparent for the player that it's only relevant for slimes. ![image](/attachments/c4bbb5b2-8a4f-461b-ad35-96ec3056dae5) The names get cut off, and it's even worse when you're using a custom font. UPD: I think I know what the issue was. You've mixed up text in comment (which is the description for a setting) and in brackets (which is the visible name of a setting). Still, you don't have to repeat the same thing for `slime_3d_chunks` name and description if there's no more info to add.
kno10 marked this conversation as resolved
kno10 force-pushed slime-spawning from 0edcfce8d4 to 9c759d4f6b 2024-06-27 12:54:06 +02:00 Compare
kno10 force-pushed slime-spawning from 9c759d4f6b to 2adf3a9904 2024-06-27 13:01:12 +02:00 Compare
Author
First-time contributor

@rudzik8, updated. Please let me know whether I should de-localize these functions.

@rudzik8, updated. Please let me know whether I should de-localize these functions.
Member

@rudzik8, updated. Please let me know whether I should de-localize these functions.

First:

image

Second, after reading through teknomunk's comment, I don't think you should.

> @rudzik8, updated. Please let me know whether I should de-localize these functions. First: ![image](/attachments/3c629fa2-73bc-4f9c-8cee-fc9f5e941961) Second, after reading through teknomunk's comment, I don't think you should.
4.4 KiB
rudzik8 added the
Testing / Retest
label 2024-06-27 16:10:30 +02:00

@rudzik8 you can use worldedit to rip out large areas underground and see if slimes are spawning there.

@rudzik8 you can use worldedit to rip out large areas underground and see if slimes are spawning there.
This pull request doesn't have enough approvals yet. 0 of 1 approvals granted.
You are not authorized to merge this pull request.
Sign in to join this conversation.
No reviewers
No Milestone
No project
No Assignees
4 Participants
Notifications
Due Date
The due date is invalid or out of range. Please use the format 'yyyy-mm-dd'.

No due date set.

Dependencies

No dependencies set.

Reference: VoxeLibre/VoxeLibre#4466
No description provided.