Improve head swivel code #4622

kno10 · 2024-08-31T19:58:38+02:00

kno10 commented

2024-08-31 19:58:38 +02:00

Use the minetest 5.9.0 API that uses radians not degree.
Simplify computations to make this more efficient, in particular by querying and updating the bone position less frequently.

Resolves minetest warning Deprecated call to set_bone_position, use set_bone_override instead in this location, but other uses remain.

I chose to not modify mcl_util.set_bone_position because it redundantly compares to the previous rotation once more.

Use the minetest 5.9.0 API that uses radians not degree. Simplify computations to make this more efficient, in particular by querying and updating the bone position less frequently. **Resolves minetest warning** `Deprecated call to set_bone_position, use set_bone_override instead` in this location, but other uses remain. I chose to not modify `mcl_util.set_bone_position` because it redundantly compares to the previous rotation once more.

kno10 added 1 commit 2024-08-31 19:58:39 +02:00

8f427042a3 Improve head swivel code.

kno10 force-pushed head-swivel from 8f427042a3 to 5267fdcf9d

2024-08-31 22:22:50 +02:00

Compare

kno10 changed title from ~~Improve head swivel code.~~ to Improve head swivel code

2024-09-01 02:06:59 +02:00

kno10 commented

2024-09-01 10:56:09 +02:00

According to this comment, this has a (positive) performance impact to reduce the calls to get_bone:
https://codeberg.org/mineclonia/mineclonia/pulls/1992#issuecomment-2274316

According to this comment, this has a (positive) performance impact to reduce the calls to get_bone: https://codeberg.org/mineclonia/mineclonia/pulls/1992#issuecomment-2274316

kno10 added the

code quality

performance

labels 2024-09-01 12:42:27 +02:00

kno10 added this to the 0.88.0 – Back on Track milestone 2024-09-01 12:42:31 +02:00

kno10 force-pushed head-swivel from 5267fdcf9d to e212280753

2024-09-03 22:35:31 +02:00

Compare

ryvnf commented

2024-09-04 20:34:07 +02:00

Use the minetest 5.9.0 API that uses radians not degree.

Currently Mineclonia currently officially support Minetest version 5.6.0. We should probably update that to 5.7.0 about now since 5.9.0 was released. I do however think it is too early to use features from 5.9.0. Can you remove them from the PR? You can put them into another PR but it will probably be a year or so until it we can merge it.

Many servers still run <5.9.0 and people install the game on older clients using ContentDB.

Edit: Posted on the wrong issue tracker :)

> Use the minetest 5.9.0 API that uses radians not degree. Currently Mineclonia currently officially support Minetest version 5.6.0. We should probably update that to 5.7.0 about now since 5.9.0 was released. I do however think it is too early to use features from 5.9.0. Can you remove them from the PR? You can put them into another PR but it will probably be a year or so until it we can merge it. Many servers still run <5.9.0 and people install the game on older clients using ContentDB. _Edit: Posted on the wrong issue tracker :)_

kno10 commented

2024-09-04 22:41:44 +02:00

For previous version if self.object.get_bone_override then -- minetest >= 5.9 will detect them and use the old method. So this is supposed to be portable (but I did not test): self.object:set_bone_position(self.head_swivel, newp, vector.apply(newr, math.deg))

For previous version `if self.object.get_bone_override then -- minetest >= 5.9` will detect them and use the old method. So this is supposed to be portable (but I did not test): `self.object:set_bone_position(self.head_swivel, newp, vector.apply(newr, math.deg))`

kno10 force-pushed head-swivel from e212280753 to 3537814a16

2024-09-05 15:30:47 +02:00

Compare

kno10 force-pushed head-swivel from 3537814a16 to ef1facbeda

2024-09-05 15:37:02 +02:00

Compare

kno10 commented

2024-09-05 16:20:52 +02:00

Pushed a bug fix for the <5.9 branch.

kno10 added the

bug

label 2024-09-05 16:21:06 +02:00

the-real-herowl requested review from the-real-herowl 2024-09-09 13:29:13 +02:00

the-real-herowl reviewed 2024-09-09 21:46:52 +02:00

mods/ENTITIES/mcl_mobs/effects.lua Outdated

					
				@ -331,3 +324,1 @@

							if chance < 1 then chance = 1 end

							local look_at_player_chance = math.random(chance)

							local look_at_player_chance = math.random(math.max(1,150/self.curiosity))

the-real-herowl commented

2024-09-09 21:39:10 +02:00

Is this correct usage of math.random, considering #4621?

Is this correct usage of `math.random`, considering #4621?

kno10 commented

2024-09-10 09:01:19 +02:00

I believe the integer rounding is intended here, as the value is compared using look_at_player_chance == 1 below. Sheep have curiosity 6, so this will be math.random(25).
It could probably be improved/simplified to something like

local look_at_player = math.random() * 150 <= self.curiosity

I believe the integer rounding is intended here, as the value is compared using `look_at_player_chance == 1` below. Sheep have curiosity 6, so this will be `math.random(25)`. It could probably be improved/simplified to something like ``` local look_at_player = math.random() * 150 <= self.curiosity ```

kno10 marked this conversation as resolved

the-real-herowl added the

Testing / Retest

label 2024-09-09 21:47:01 +02:00

the-real-herowl commented

2024-09-09 21:55:32 +02:00

I tested this under 5.8, seems to work fine. Not sure what could break here.

However, some of our mobs just don't have head swivel support in their models. I remember having weird issues when doing rovers and stalkers, and eventually skipped this part. Those not working are obviously not regressions and shouldn't stall this PR.

I tested this under 5.8, seems to work fine. Not sure what could break here. However, some of our mobs just don't have head swivel support in their models. I remember having weird issues when doing rovers and stalkers, and eventually skipped this part. Those not working are obviously not regressions and shouldn't stall this PR.

kno10 force-pushed head-swivel from ef1facbeda to c1dc9d401c

2024-09-10 09:53:40 +02:00

Compare

kno10 commented

2024-09-10 09:57:47 +02:00

updated, but did not test curiosity yet.

the-real-herowl reviewed 2024-09-16 12:58:13 +02:00

the-real-herowl left a comment

Curiosity meaning may be skewed, if it's configured properly for the various mobs, it may be out of scope for this PR. If it isn't... well, could as well reverse it.

Otherwise code LGTM.

Curiosity meaning may be skewed, if it's configured properly for the various mobs, it may be out of scope for this PR. If it isn't... well, could as well reverse it. Otherwise code LGTM.

mods/ENTITIES/mcl_mobs/effects.lua Outdated

					
				@ -307,3 +304,2 @@

					-- was 10000 - div by 12 for avg entities as outside loop

					local stop_look_at_player = stop_look_at_player_chance == 1

					local stop_look_at_player = math.random() * 833 <= self.curiosity

the-real-herowl commented

2024-09-16 12:22:56 +02:00

Or should it be >=? As it is, the higher "curiosity", the lower the chance to look at the player...?

Or should it be `>=`? As it is, the higher "curiosity", the lower the chance to look at the player...?

kno10 commented

2024-09-16 13:11:15 +02:00

I believe the motivation is that a more curious entity also loses interest faster.

the-real-herowl marked this conversation as resolved

mods/ENTITIES/mcl_mobs/effects.lua Outdated

					
				@ -348,0 +322,4 @@

									-- but frequency of check isn't good as it is costly. Making others too infrequent requires testing

									-- was 5000 but called in loop based on entities. so div by 12 as estimate avg of entities found,

									-- then div by 20 as less freq lookup

									if math.random() * 150 <= self.curiosity then

the-real-herowl commented

2024-09-16 12:24:49 +02:00

Same thing here?

kno10 commented

2024-09-16 13:12:24 +02:00

Seems fine to me: A more curious entity should start looking at the player more often.

the-real-herowl marked this conversation as resolved

the-real-herowl commented

2024-09-16 13:29:13 +02:00

Code is fine then, needs testing only.

kno10 referenced this pull request

2024-09-26 17:36:36 +02:00

Improve mob smartness (cliffs, paths) part 1 #4479

kno10 added a new dependency 2024-09-26 17:37:24 +02:00

#4479 Improve mob smartness (cliffs, paths) part 1

kno10 referenced this pull request

2024-10-01 12:30:29 +02:00

Improve mob smartness (cliffs, paths) part 1 #4479

kno10 force-pushed head-swivel from c1dc9d401c to 2e5b0bb3c7

2024-10-27 19:21:05 +01:00

Compare

kno10 commented

2024-10-28 18:04:25 +01:00

I am somewhat troubled that the entire eye_height logic in the existing code is somewhat odd. If you look at the mobs, they often have very weird eye_height values:

axolotl.lua:	head_eye_height = -0.5,
blaze.lua:	head_eye_height = 3.5,
chicken.lua:	head_eye_height = 1.5,
cow+mooshroom.lua:	head_eye_height = 1.1,
llama.lua:	head_eye_height = 3,
ocelot.lua:	head_eye_height = 0.4,
piglin.lua:	head_eye_height = 1.4,
pig.lua:	head_eye_height = 0.8,
polar_bear.lua:	head_eye_height = 1,
rabbit.lua:	head_eye_height = 0.5,
sheep.lua:	head_eye_height = 1.1,
stalker.lua:	head_eye_height = 1.8;
villager_evoker.lua:	head_eye_height = 2.2,
villager_illusioner.lua:	head_eye_height = 2.2,
villager.lua:	head_eye_height = 2.2,
villager_vindicator.lua:	head_eye_height = 2.2,
wolf.lua:	head_eye_height = 1.1,
zombie.lua:	head_eye_height = 2.2,

I believe there might be a sign error somewhere in these computations, and these values were manually adjusted to "look good" rather than by fixing the math. Just consider axolotl and chicken: -0.5 vs. 1.5? why?

I am somewhat troubled that the entire `eye_height` logic in the existing code is somewhat odd. If you look at the mobs, they often have very weird eye_height values: ``` axolotl.lua: head_eye_height = -0.5, blaze.lua: head_eye_height = 3.5, chicken.lua: head_eye_height = 1.5, cow+mooshroom.lua: head_eye_height = 1.1, llama.lua: head_eye_height = 3, ocelot.lua: head_eye_height = 0.4, piglin.lua: head_eye_height = 1.4, pig.lua: head_eye_height = 0.8, polar_bear.lua: head_eye_height = 1, rabbit.lua: head_eye_height = 0.5, sheep.lua: head_eye_height = 1.1, stalker.lua: head_eye_height = 1.8; villager_evoker.lua: head_eye_height = 2.2, villager_illusioner.lua: head_eye_height = 2.2, villager.lua: head_eye_height = 2.2, villager_vindicator.lua: head_eye_height = 2.2, wolf.lua: head_eye_height = 1.1, zombie.lua: head_eye_height = 2.2, ``` I believe there might be a sign error somewhere in these computations, and these values were manually adjusted to "look good" rather than by fixing the math. Just consider axolotl and chicken: -0.5 vs. 1.5? why?

the-real-herowl commented

2024-10-28 18:16:11 +01:00

Well, the rest don't look that off though.

kno10 commented

2024-10-28 19:53:21 +01:00

@the-real-herowl llama at 3, blaze with 3.5? even villagers clearly are not 2.2 blocks tall.

Bloodaxe commented

2024-11-08 00:55:38 +01:00

First-time contributor

Looks good to me from my testing. The only thing I'd comment is their heads seem to switch back-and-forth from looking at you to looking forward a bit too rapidly.