Revert mob rewrite #1992

cora · 2022-02-13T22:52:14+01:00

cora commented

2022-02-13 22:52:14 +01:00

This PR fixes #2014, #2016, #1931

This PR reverts all files in the mods/ENTITIES/mcl_mobs, mods/ENTITIES/mobs_mc and mods/ENTITIES/mobs_mc_gameconfig hierarchies to the state of commit 32c03dc27e (the commit before the new mobs got merged).

now this commit is a fair bit newer than the mcla mobs i used for the mobs-study so this meant that intially it didn't crash at all (because this is after the armor rewrite) and even later I only had to change a call when mobs throw experience on death.

Of course this needs to be tested more. It's running on oy.edgy1.net:30422 now but it seems very promising so far.

This PR fixes #2014, #2016, #1931 This PR reverts all files in the mods/ENTITIES/mcl_mobs, mods/ENTITIES/mobs_mc and mods/ENTITIES/mobs_mc_gameconfig hierarchies to the state of commit 32c03dc27eb835fb60fdc2e396f6c3d5e5fc010d (the commit before the new mobs got merged). now this commit is a fair bit newer than the mcla mobs i used for the mobs-study so this meant that intially it didn't crash at all (because this is after the armor rewrite) and even later I only had to change a call when mobs throw experience on death. Of course this needs to be tested more. It's running on oy.edgy1.net:30422 now but it seems very promising so far.

👍 2 🚀 1

cora force-pushed revert-to-oldmobs from f47e0bc81e to e66ebfea70

2022-02-13 22:52:48 +01:00

Compare

cora added the

#P2: HIGH

mobs

help wanted

labels 2022-02-13 22:54:52 +01:00

cora added this to the 0.72.0 milestone 2022-02-13 22:54:59 +01:00

cora requested review from Legacy-Contributors 2022-02-13 22:55:26 +01:00

cora referenced this pull request

2022-02-13 23:26:30 +01:00

[DISCUSSION] Release and Mobs #1988

NO11 commented

2022-02-13 23:36:04 +01:00

i just realized how much of @jordan4ibanez 's work this is ruining. But it's probably the right move

NO11 added this to the 0.72 project 2022-02-13 23:36:21 +01:00

kabou commented

2022-02-14 01:27:04 +01:00

i just realized how much of @jordan4ibanez 's work this is ruining. But it's probably the right move

Would it be possible to create a branch that still has the unreverted mobs? That way it could in theory be cherrypicked for useful bits when the mobs have stabilized. And there is always mineclone5, it still has the jordan mobs, right?

> i just realized how much of @jordan4ibanez 's work this is ruining. But it's probably the right move Would it be possible to create a branch that still has the unreverted mobs? That way it could in theory be cherrypicked for useful bits when the mobs have stabilized. And there is always mineclone5, it still has the jordan mobs, right?

kabou commented

2022-02-14 01:30:39 +01:00

Of course this needs to be tested more. It's running on oy.edgy1.net:30422 now but it seems very promising so far.

It was nice while it lasted. The server crashed when I met a villager and right-clicked on it.

There was some weirdness, like a spider flying off high into the air.

Some good stuff too, I met a cat. Cats weren't in the 0.71 mobs were they? I Like cats, can we have them on 0.72?

And then there were these: did they get spawned on purpose in the overworld or was that a fluke? (oh wait I get it, that was the /mobtest in action..)

> Of course this needs to be tested more. It's running on oy.edgy1.net:30422 now but it seems very promising so far. It was nice while it lasted. The server crashed when I met a villager and right-clicked on it. There was some weirdness, like a spider flying off high into the air. Some good stuff too, I met a cat. Cats weren't in the 0.71 mobs were they? I Like cats, can we have them on 0.72? And then there were these: did they get spawned on purpose in the overworld or was that a fluke? (oh wait I get it, that was the /mobtest in action..)

screenshot_20220214_004002.png

566 KiB

kabou commented

2022-02-14 01:44:28 +01:00

Definitely a problem with villagers, I made it crash again. I'll try to behave.

kabou commented

2022-02-14 03:15:34 +01:00

Mobs were generally well-behaved. Perhaps some could use a bit of tweaking, like the creeper explosions. There were a number of fixes made after the jorda4ibanez mob merge, some of which may need to be reapplied.

Villagers were a bit of an issue, interacting caused a server crash. They also spawn outside of villages.

I did not see the mobs trampolining on water, but I did not take the time to investigate thoroughly.

What surprised me most pleasantly was that mobs clip eachother again. Mobs colliding and even standing on top of one another was a nuisance before, but you fixed it here. Good work!

Thanks for putting up the testing server.

Mobs were generally well-behaved. Perhaps some could use a bit of tweaking, like the creeper explosions. There were a number of fixes made after the jorda4ibanez mob merge, some of which may need to be reapplied. Villagers were a bit of an issue, interacting caused a server crash. They also spawn outside of villages. I did not see the mobs trampolining on water, but I did not take the time to investigate thoroughly. What surprised me most pleasantly was that mobs clip eachother again. Mobs colliding and even standing on top of one another was a nuisance before, but you fixed it here. Good work! Thanks for putting up the testing server.

cora commented

2022-02-14 03:32:42 +01:00

Mobs were generally well-behaved. Perhaps some could use a bit of tweaking, like the creeper explosions. There were a number of fixes made after the jorda4ibanez mob merge, some of which may need to be reapplied.

well i'm not sure how applicable they're going to be but yeah some could still apply.

Villagers were a bit of an issue, interacting caused a server crash. They also spawn outside of villages.

they don't naturally spawn outside of villages now i suppose – they spawn on grass path. I think the current mobs' villagers spawn on grass.

I did not see the mobs trampolining on water, but I did not take the time to investigate thoroughly.

What surprised me most pleasantly was that mobs clip eachother again. Mobs colliding and even standing on top of one another was a nuisance before, but you fixed it here. Good work!

Uh i didnt fix anything except the little function call in the 2nd commit. Otherwise it's literally the state from said commit.

> Mobs were generally well-behaved. Perhaps some could use a bit of tweaking, like the creeper explosions. There were a number of fixes made after the jorda4ibanez mob merge, some of which may need to be reapplied. well i'm not sure how applicable they're going to be but yeah some could still apply. > Villagers were a bit of an issue, interacting caused a server crash. They also spawn outside of villages. they don't naturally spawn outside of villages now i suppose – they spawn on grass path. I think the current mobs' villagers spawn on grass. > I did not see the mobs trampolining on water, but I did not take the time to investigate thoroughly. > > What surprised me most pleasantly was that mobs clip eachother again. Mobs colliding and even standing on top of one another was a nuisance before, but you fixed it here. Good work! Uh i didnt fix anything except the little function call in the 2nd commit. Otherwise it's literally the state from said commit.

cora commented

2022-02-14 03:33:07 +01:00

But yeah villager crash needs to be tested

cora commented

2022-02-14 03:36:00 +01:00

It was nice while it lasted. The server crashed when I met a villager and right-clicked on it.

nice i'll have a look.

Some good stuff too, I met a cat. Cats weren't in the 0.71 mobs were they? I Like cats, can we have them on 0.72?

Cats were always there they just have very rare spawn conditions (jungle wood with air on top).

And then there were these: did they get spawned on purpose in the overworld or was that a fluke? (oh wait I get it, that was the /mobtest in action..)

yeah i did the mobtest thing a few times to maybe catch some random crashes when someone logs in.

> It was nice while it lasted. The server crashed when I met a villager and right-clicked on it. nice i'll have a look. > Some good stuff too, I met a cat. Cats weren't in the 0.71 mobs were they? I Like cats, can we have them on 0.72? Cats were always there they just have very rare spawn conditions (jungle wood with air on top). > And then there were these: did they get spawned on purpose in the overworld or was that a fluke? (oh wait I get it, that was the /mobtest in action..) yeah i did the mobtest thing a few times to maybe catch some random crashes when someone logs in.

erle commented

2022-02-14 14:09:58 +01:00

Cats were always there they just have very rare spawn conditions (jungle wood with air on top).

I think the problem was something like:

Ocelots (which become cats if tamed) can spawn on jungle wood and jungle leaves.
Mobs can only spawn on opaque, liquid, or grass path nodes – i.e. not on leaves.

> Cats were always there they just have very rare spawn conditions (jungle wood with air on top). I think the problem was something like: 1. Ocelots (which become cats if tamed) can spawn on jungle wood and jungle leaves. 2. Mobs can only spawn on opaque, liquid, or grass path nodes – i.e. not on leaves.

cora force-pushed revert-to-oldmobs from 5ba818aa8c to 4cda7ab400

2022-02-14 14:29:07 +01:00

Compare

cora commented

2022-02-15 12:38:27 +01:00

So funny thing: one of the things i fixed, the rightclicking of villagers is actually a crash in master rn – doesnt make sense to make an issue though i suppose if we're moving forward with this.

👍 1

cora force-pushed revert-to-oldmobs from 4cda7ab400 to 990d51b890

2022-02-18 02:32:30 +01:00

Compare

cora referenced this pull request

2022-02-21 04:07:54 +01:00

Right-click on Villager crash #2014

kabou commented

2022-02-21 04:53:45 +01:00

@cora your "fix crash on opening villager formspec" patch fixes the crashes with villagers.

Please merge at least this commit with master.

@cora your ["fix crash on opening villager formspec"](https://git.minetest.land/MineClone2/MineClone2/commit/e1573d1c6207aadb74b91f1e1ec73ce886a8a9c7) patch fixes the crashes with villagers. Please merge at least this commit with master.

kabou commented

2022-02-21 05:00:56 +01:00

For completeness sake, and not to suggest that these are the most pressing matters, I would like to make some incidental observations of current (master branch) mob behaviors that should change.

While testing the abovementioned patch, I tested rightclicking three villagers that had spawned near me. There was no village. When I went to look for a village (only found a "village" that comprised of one house) and went back, all three villagers had depawned.

villagers should spawn in villages, near beds.
villagers should not despawn ever, I think.
When I found a "village" in an arctic biome, it showed pigs on ice and also an icecow. I don't think that these mobs should spawn there.

For completeness sake, and not to suggest that these are the most pressing matters, I would like to make some incidental observations of current (master branch) mob behaviors that should change. While testing the abovementioned patch, I tested rightclicking three villagers that had spawned near me. There was no village. When I went to look for a village (only found a "village" that comprised of one house) and went back, all three villagers had depawned. * villagers should spawn in villages, near beds. * villagers should not despawn ever, I think. * When I found a "village" in an arctic biome, it showed pigs on ice and also an icecow. I don't think that these mobs should spawn there.

screenshot_20220221_044657.png

562 KiB

kabou referenced this pull request

2022-02-21 05:02:35 +01:00

Right-click on Villager crash #2014

cora commented

2022-02-21 05:08:16 +01:00

Please merge at least this commit with master.

Seeing the massive changes of this PR i'm not sure that would even work. I don't think it makes sense to fix this in master now if we're going to merge this soon.

> Please merge at least this commit with master. Seeing the massive changes of this PR i'm not sure that would even work. I don't think it makes sense to fix this in master now if we're going to merge this soon.

kabou commented

2022-02-21 05:10:40 +01:00

We'll have to bite the bullit one way or another..

kabou commented

2022-02-21 10:05:31 +01:00

@cora the villager fix is not a mob fix, it is a fix for the re-enchanting"fix" committed 1 week ago that set tool_capabilities to nil, which is causing the crash downstream in mcl_enchanting code.

All of this takes place in mcl_enchanting, so not related to the mob revert in any way. Your patch is good, but should not even be part of this mob related PR.

I will create a separate PR for this fix to the fix.

@cora the villager fix is not a mob fix, it is a fix for the re-enchanting["fix"](https://git.minetest.land/MineClone2/MineClone2/commit/d898b02c8bfe192f1647533094a4dfcc85b756d4) committed [1 week ago](https://git.minetest.land/MineClone2/MineClone2/pulls/1989) that set tool_capabilities to nil, which is causing the crash downstream in mcl_enchanting code. All of this takes place in mcl_enchanting, so not related to the mob revert in any way. Your patch is good, but should not even be part of this mob related PR. I will create a separate PR for this fix to the fix.

cora commented

2022-02-21 12:07:04 +01:00

@cora the villager fix is not a mob fix, it is a fix for the re-enchanting"fix" committed 1 week ago that set tool_capabilities to nil, which is causing the crash downstream in mcl_enchanting code.

there are 2 villager rightclick fixes here. but yes you are correct the 2nd one has to do with enchanting

c2cbf1198c

990d51b890

but again it makes very little sense to patch this in master now.

> @cora the villager fix is not a mob fix, it is a fix for the re-enchanting["fix"](https://git.minetest.land/MineClone2/MineClone2/commit/d898b02c8bfe192f1647533094a4dfcc85b756d4) committed [1 week ago](https://git.minetest.land/MineClone2/MineClone2/pulls/1989) that set tool_capabilities to nil, which is causing the crash downstream in mcl_enchanting code. there are 2 villager rightclick fixes here. but yes you are correct the 2nd one has to do with enchanting https://git.minetest.land/MineClone2/MineClone2/commit/c2cbf1198c29b8917b07dff7c497c29990ed9b77 https://git.minetest.land/MineClone2/MineClone2/commit/990d51b890d31ddc500468063d7b66029c5e1053 but again it makes very little sense to patch this in master now.

kay27 commented

2022-02-21 12:31:48 +01:00

(only found a "village" that comprised of one house)

There is an error in villages which stops building placement on any error, that's why it was very common to find single-building village on their initial commit, then I buried this bug deeper, but now it's fixed in MineClone2/MineClone2#1976

IDK. It's a bit hard for me to synchronyze mapgen stuff between MCL2 and MCL5. I did my best for this PR, but after that I already fixed several minor mistakes in MCL5. If it was merged, many things would be more obvious. Now I can only suggest ignoring villages like this. Villager spawn currently implemented in a different way in all versions of villages. It will be changed when you'll got final decision about mob API.

> (only found a "village" that comprised of one house) There is an error in villages which stops building placement on any error, that's why it was very common to find single-building village on their initial commit, then I buried this bug deeper, but now it's fixed in https://git.minetest.land/MineClone2/MineClone2/pulls/1976 IDK. It's a bit hard for me to synchronyze mapgen stuff between MCL2 and MCL5. I did my best for this PR, but after that I already fixed several minor mistakes in MCL5. If it was merged, many things would be more obvious. Now I can only suggest ignoring villages like this. Villager spawn currently implemented in a different way in all versions of villages. It will be changed when you'll got final decision about mob API.

kabou commented

2022-02-21 12:56:26 +01:00

[cora]

but again it makes very little sense to patch this in master now.

I don't understand the reasoning here. What do you see as the downside to applying these fixes?

[kay27]

There is an error in villages which stops building placement on any error, that's why it was very common to find single-building village on their initial commit, then I buried this bug deeper, but now it's fixed in #1976

This is a bug, but not one that causes crashes. It would be great to get this fixed in due time.

[cora] > but again it makes very little sense to patch this in master now. I don't understand the reasoning here. What do you see as the downside to applying these fixes? [kay27] > There is an error in villages which stops building placement on any error, that's why it was very common to find single-building village on their initial commit, then I buried this bug deeper, but now it's fixed in #1976 This is a bug, but not one that causes crashes. It would be great to get this fixed in due time.

cora force-pushed revert-to-oldmobs from 990d51b890 to 34b07bd02a

2022-02-21 13:36:49 +01:00

Compare

cora commented

2022-02-21 14:32:42 +01:00

(only found a "village" that comprised of one house)

There is an error in villages which stops building placement on any error, that's why it was very common to find single-building village on their initial commit, then I buried this bug deeper, but now it's fixed in MineClone2/MineClone2#1976

This has literally never happened on oysterity (mineclonia) before (afaik) - p sure people would have told me if they had found such a thing.

IDK. It's a bit hard for me to synchronyze mapgen stuff between MCL2 and MCL5. I did my best for this PR, but after that I already fixed several minor mistakes in MCL5. If it was merged, many things would be more obvious. Now I can only suggest ignoring villages like this. Villager spawn currently implemented in a different way in all versions of villages. It will be changed when you'll got final decision about mob API.

I get that but to be fair your mapgen stuff did some rather wild things before and your prs aren't like easy to review sorry to say.

I don't understand the reasoning here. What do you see as the downside to applying these fixes?

Well for starters I'm the only one who has ever tested it as far as I know.

But generally this is just a very large change and I'd like to do what I can to make sure it doesn't make things worse.

> > (only found a "village" that comprised of one house) > > There is an error in villages which stops building placement on any error, that's why it was very common to find single-building village on their initial commit, then I buried this bug deeper, but now it's fixed in https://git.minetest.land/MineClone2/MineClone2/pulls/1976 This has literally never happened on oysterity (mineclonia) before (afaik) - p sure people would have told me if they had found such a thing. > > IDK. It's a bit hard for me to synchronyze mapgen stuff between MCL2 and MCL5. I did my best for this PR, but after that I already fixed several minor mistakes in MCL5. If it was merged, many things would be more obvious. Now I can only suggest ignoring villages like this. Villager spawn currently implemented in a different way in all versions of villages. It will be changed when you'll got final decision about mob API. I get that but to be fair your mapgen stuff did some rather wild things before and your prs aren't like easy to review sorry to say. > I don't understand the reasoning here. What do you see as the downside to applying these fixes? Well for starters I'm the only one who has ever tested it as far as I know. But generally this is just a very large change and I'd like to do what I can to make sure it doesn't make things worse.

👍 1 ❤️ 1

erle commented

2022-02-21 14:44:24 +01:00

This has literally never happened on oysterity (mineclonia) before (afaik) - p sure people would have told me if they had found such a thing.

I have found villages in Mineclonia that consisted of two or three houses. But they were all at a coastline … so I guess villagegen is not creating artificial islands?

> This has literally never happened on oysterity (mineclonia) before (afaik) - p sure people would have told me if they had found such a thing. I have found villages in Mineclonia that consisted of two or three houses. But they were all at a coastline … so I guess villagegen is not creating artificial islands?

kabou commented

2022-02-21 15:56:04 +01:00

But generally this is just a very large change and I'd like to do what I can to make sure it doesn't make things worse.

To be clear, I was specifically referring to the small villager crash patches. Those should be safe to merge into master, shoudln't they?

I understand your considered hesitance about the larger mob changes, although - as already said - we're going to have to bite the bullet at some point in time anyway.

BTW, I did test your mob patch as you currently feature on your test server and I found it good enough. The only real regressions that I encountered were.. villager related crashes!

> But generally this is just a very large change and I'd like to do what I can to make sure it doesn't make things worse. To be clear, I was specifically referring to the small villager crash patches. Those should be safe to merge into master, shoudln't they? I understand your considered hesitance about the larger mob changes, although - as already said - we're going to have to bite the bullet at some point in time anyway. BTW, I did test your mob patch as you currently feature on your test server and I found it good enough. The only real regressions that I encountered were.. villager related crashes!

cora force-pushed revert-to-oldmobs from 34b07bd02a to 82653e84d7

2022-02-22 23:30:12 +01:00

Compare

cora force-pushed revert-to-oldmobs from 82653e84d7 to a581c403f3

2022-02-24 03:02:02 +01:00

Compare

cora commented

2022-02-24 16:04:06 +01:00

The server hasn't been crashing since the enchanting thing. So far as I could see i tested all special functions of mobs including going through a few villager trades (even though it apparently all gets generated when you fist click it as the enchanting crash showed).

I'll remove the wip. would be good if someone else still tested it a little bit though.

The server hasn't been crashing since the enchanting thing. So far as I could see i tested all special functions of mobs including going through a few villager trades (even though it apparently all gets generated when you fist click it as the enchanting crash showed). I'll remove the wip. would be good if someone else still tested it a little bit though.

cora changed title from ~~WIP: Revert mob rewrite~~ to Revert mob rewrite

2022-02-24 16:04:13 +01:00

cora force-pushed revert-to-oldmobs from a581c403f3 to 83aa9cd4a0

2022-02-24 16:04:47 +01:00

Compare

kneekoo commented

2022-02-24 17:26:01 +01:00

I'll remove the wip. would be good if someone else still tested it a little bit though.

I tested it since #1988 and reported the few things I found. I'd have to do extensive testing with all mobs, to be fair, but so far I encountered (and they acted normally):

cows, mooshrooms, sheep, rabbits, pigs, wolves, horses... not sure about others
creepers, zombies, skeletons, endermen

I'll get back and try to do some more stuff, but I'll have to go far from spawn. The place I set up near spawn has been looted, so it was a bad place to gather materials. A non-anarachy server would certainly be more productive for testing. :P

> I'll remove the wip. would be good if someone else still tested it a little bit though. I tested it since #1988 and reported the few things I found. I'd have to do extensive testing with all mobs, to be fair, but so far I encountered (and they acted normally): - cows, mooshrooms, sheep, rabbits, pigs, wolves, horses... not sure about others - creepers, zombies, skeletons, endermen I'll get back and try to do some more stuff, but I'll have to go far from spawn. The place I set up near spawn has been looted, so it was a bad place to gather materials. A non-anarachy server would certainly be more productive for testing. :P

erle commented

2022-02-24 17:55:24 +01:00

The server hasn't been crashing since the enchanting thing. So far as I could see i tested all special functions of mobs including going through a few villager trades (even though it apparently all gets generated when you fist click it as the enchanting crash showed).

I'll remove the wip. would be good if someone else still tested it a little bit though.

Can we make a list with things that at least one person has tested? Like:

Putting a saddle on a pig works.
Riding a sadlded pig works.
Steering a saddled pig with a carrot on a stick works.

And so on …

> The server hasn't been crashing since the enchanting thing. So far as I could see i tested all special functions of mobs including going through a few villager trades (even though it apparently all gets generated when you fist click it as the enchanting crash showed). > > I'll remove the wip. would be good if someone else still tested it a little bit though. Can we make a list with things that at least one person has tested? Like: * Putting a saddle on a pig works. * Riding a sadlded pig works. * Steering a saddled pig with a carrot on a stick works. And so on …

cora commented

2022-02-24 20:21:18 +01:00

Putting a saddle on a pig works.

Riding a sadlded pig works.

Steering a saddled pig with a carrot on a stick works.

these work. even on horses at least that were new mobs before.

> * Putting a saddle on a pig works. > * Riding a sadlded pig works. > * Steering a saddled pig with a carrot on a stick works. these work. even on horses at least that were new mobs before.

kneekoo commented

2022-02-24 21:05:02 +01:00

@cora By the way, the server crashed a few hours ago, while I was logged in (but away from PC). Hopefully the log has something useful about it.

cora commented

2022-02-24 21:21:39 +01:00

yes nice. it's another throw_experience apparently.

Fixed this one an one more occurence in the codebase (in enderdragon)

yes nice. it's another throw_experience apparently. Fixed this one an one more occurence in the codebase (in enderdragon)

cora force-pushed revert-to-oldmobs from 85a93f9b81 to 34a578ee5d

2022-02-24 22:38:38 +01:00

Compare

cora commented

2022-02-25 00:11:39 +01:00

I will do another round of going through all mobs 1 by 1 killing them with enchanted swords, normal arrows and potion arrows, do the villager trades a few times, try riding again.

Can anyone think of something else?

Otherwise I'll merge this tomorrow. (If all tests work out ofc)

I will do another round of going through all mobs 1 by 1 killing them with enchanted swords, normal arrows and potion arrows, do the villager trades a few times, try riding again. Can anyone think of something else? Otherwise I'll merge this tomorrow. (If all tests work out ofc)

cora force-pushed revert-to-oldmobs from 34a578ee5d to 773b79e9c5

2022-02-25 00:12:53 +01:00

Compare

cora force-pushed revert-to-oldmobs from 773b79e9c5 to 3feca330c9

2022-02-25 01:40:35 +01:00

Compare

kabou commented

2022-02-25 02:20:36 +01:00

No major problems found in an existing world.

The annoying zombies on meth are gone, good. Slime aggression may need a bit of tuning, they're kind of passive, unlike their nether brethren the magma cubes.

Villager trades work. Tamed and bred mobs do not despawn.

Mobs clip other mobs - good. Mobs clip player, perhaps not good.

Looks good overall so far.

No major problems found in an existing world. The annoying zombies on meth are gone, good. Slime aggression may need a bit of tuning, they're kind of passive, unlike their nether brethren the magma cubes. Villager trades work. Tamed and bred mobs do not despawn. Mobs clip other mobs - good. Mobs clip player, perhaps not good. Looks good overall so far.

cora commented

2022-02-25 02:22:45 +01:00

yeah i've done another killing and riding round on the server after rebase. everything worked out there as well.

cora commented

2022-02-25 14:54:38 +01:00

alright i'll say this is good enough. Killed all of them again (btw. is it on purpose that you can't shoot potioned arrows withe the corssbow (that's no typo, that's what it's called)