Make mobs ride minecarts #3507

anarquimico · 2023-03-04T21:31:35+01:00

anarquimico commented

2023-03-04 21:31:35 +01:00

This PR makes the common minecart able to carry mobs.

First time trying to push from the MCL2 repo... If I did something wrong let me know.

This PR makes the common minecart able to carry mobs. First time trying to push from the MCL2 repo... If I did something wrong let me know. ----- ![](https://imgur.com/hDyW2VO.png)

Ghost force-pushed mobs_in_minecarts from 4436f54835 to 8e5cc1b5cd

2023-03-05 01:03:41 +01:00

Compare

Ghost commented

2023-03-05 02:15:08 +01:00

First-time contributor

From my testing, the mobs love to transfer from minecarts and boats to minecarts as the player places more minecarts. It is also very hard to get annoyed mobs into minecarts if they are focused on the user.

anarquimico commented

2023-03-05 02:41:15 +01:00

From my testing, the mobs love to transfer from minecarts and boats to minecarts as the player places more minecarts. It is also very hard to get annoyed mobs into minecarts if they are focused on the user.

Yeah, this also happens with boats, as more and more boats are placed above, it transfer the mob. Probably the best solution would be to have a dedicated function for riding things in mcl_mobs. But don't know how that would procede...

> From my testing, the mobs love to transfer from minecarts and boats to minecarts as the player places more minecarts. It is also very hard to get annoyed mobs into minecarts if they are focused on the user. Yeah, this also happens with boats, as more and more boats are placed above, it transfer the mob. Probably the best solution would be to have a dedicated function for riding things in mcl_mobs. But don't know how that would procede...

👍 1

ancientmarinerdev requested changes 2023-03-08 23:21:23 +01:00

ancientmarinerdev left a comment

Thanks for fixing this, I really appreciate the work and problem solving skills. While the solution is good functionally, and I have learnt something more about minecarts here, we could improve on it performance wise. I have left a few comments on things to consider and how we can improve it.

Apologies for the delay in my response getting back to you. After my vacation, I took a bit of time to get up to speed. :)

Thanks for fixing this, I really appreciate the work and problem solving skills. While the solution is good functionally, and I have learnt something more about minecarts here, we could improve on it performance wise. I have left a few comments on things to consider and how we can improve it. Apologies for the delay in my response getting back to you. After my vacation, I took a bit of time to get up to speed. :)

mods/ENTITIES/mcl_minecarts/init.lua

					
				@ -213,2 +214,4 @@

						end

						-- Grab mob

						if not self._passenger then

ancientmarinerdev commented

2023-03-08 23:05:19 +01:00

I think we're going to need a timer on this. If you set a timer to 0 on the object, and then add dtime to it, after timer > x, then run this code and reset it 0.

The reason for this, is on_step runs at least 20 times a second. Most of the performance profiling I do, the 2 most expensive operations the usually flag up on the report are usually looking for a node near, or looking for an object near. If you're doing that 20 times a second, even for 1 block, it can be really expensive. If you have many minecarts, even worse.

It should run that code the minimum amount to function, so will require some testing. x could be 0.5 (so twice a second, which would be 10 times less costly. I would also consider every 1 second, or ever 0.75 for example, just see what works, and what doesn't at fastest minecart setting. This is trial and error or course. We need the most infrequent that still functions.

I think we're going to need a timer on this. If you set a timer to 0 on the object, and then add dtime to it, after timer > x, then run this code and reset it 0. The reason for this, is on_step runs at least 20 times a second. Most of the performance profiling I do, the 2 most expensive operations the usually flag up on the report are usually looking for a node near, or looking for an object near. If you're doing that 20 times a second, even for 1 block, it can be really expensive. If you have many minecarts, even worse. It should run that code the minimum amount to function, so will require some testing. x could be 0.5 (so twice a second, which would be 10 times less costly. I would also consider every 1 second, or ever 0.75 for example, just see what works, and what doesn't at fastest minecart setting. This is trial and error or course. We need the most infrequent that still functions.

anarquimico commented

2023-03-19 23:21:47 +01:00

Tried to add a timer, but didn't worked out. At least I didn't managed to make it work... But, nonetheless I used the math.random(1,20) > 15 strategy. I hope this makes performance better.

mods/ENTITIES/mcl_minecarts/init.lua

					
				@ -215,0 +231,4 @@

							end

						-- Make room in the minecart after the mob dies

						elseif self._passenger then

							local mobinside = minetest.get_objects_inside_radius(self.object:get_pos(), 1)

ancientmarinerdev commented

2023-03-08 23:11:52 +01:00

I'm wondering if rather than running this code 20 times a second, we call a function from the death handling.

If you look in physics.lua in mcl_mobs for:

function mob_class:check_for_death(cause, cmi_cause)

It calls:

local function death_handle(self)

In there feels the most appropriate place to check for the minecart and detach. That way the code and looking for the minecart happens once only on death, and not multiple times a second. I would have the minecart logic in a function in minecarts, and the death_handle calls calls that function with mob as parameter. This function would check for a minecart nearby, hopefully find the most appropriate and update it.

I don't want to discourage you as your problem solving and code is good, will likely work and maybe before profiling, I might have done a similar thing. I do think, however that looking for the most performant solution will really help this game scale for servers and help things run good and lag free :).

I'm wondering if rather than running this code 20 times a second, we call a function from the death handling. If you look in physics.lua in mcl_mobs for: function mob_class:check_for_death(cause, cmi_cause) It calls: local function death_handle(self) In there feels the most appropriate place to check for the minecart and detach. That way the code and looking for the minecart happens once only on death, and not multiple times a second. I would have the minecart logic in a function in minecarts, and the death_handle calls calls that function with mob as parameter. This function would check for a minecart nearby, hopefully find the most appropriate and update it. I don't want to discourage you as your problem solving and code is good, will likely work and maybe before profiling, I might have done a similar thing. I do think, however that looking for the most performant solution will really help this game scale for servers and help things run good and lag free :).

👍 1

MrRar commented

2023-03-08 23:40:12 +01:00

Or you can set self._passenger to the ObjectRef/luaentity and then just do an ObjectRef validity test on self._passenger. Setting self._passenger to the name of the mob doesn't seem to useful.

Also you need to consider that the radius of 1.3 will pick up some entities that aren't the riding mob. If you know the exact position of an entity you can use a radius of 0 to get the entity. But it is moot in this case.

Or you can set self._passenger to the ObjectRef/luaentity and then just do an ObjectRef validity test on self._passenger. Setting self._passenger to the name of the mob doesn't seem to useful. Also you need to consider that the radius of 1.3 will pick up some entities that aren't the riding mob. If you know the exact position of an entity you can use a radius of 0 to get the entity. But it is moot in this case.

👍 1

anarquimico commented

2023-03-19 23:23:30 +01:00

Changed the self._passenger for the entity instead of the entity.name. By this I could run the check_for_death. A much better approach, thanks for the suggestions!

Changed the self.\_passenger for the entity instead of the entity.name. By this I could run the check_for_death. A much better approach, thanks for the suggestions!

ancientmarinerdev commented

2023-03-20 03:06:12 +01:00

Hi. I probably didn't communicate well what I mean. I meant that this code is called 20 times a second. By putting the logic in the death_handle function, it will only be called on death. So the death_handle function will call a function that checks if a minecart was near that it was in, and remove passenger if it was in it. That way, it is only called once, and that is when the mob dies.

anarquimico commented

2023-03-20 12:48:34 +01:00

Oh now I got it! As I mentioned before, probably the best way to handle mobs in minecarts/boats would be to have a dedicated function in mcl_mobs, but since the boat riding code was on the boat init.lua I though on adding this on the minecart as well.

I personally don't know if it's a good thing to search for an attached minecart everytime a mob dies, as probably there will be more mobs dying than mobs riding minecarts.

Oh now I got it! As I mentioned before, probably the best way to handle mobs in minecarts/boats would be to have a dedicated function in mcl_mobs, but since the boat riding code was on the boat init.lua I though on adding this on the minecart as well. I personally don't know if it's a good thing to search for an attached minecart everytime a mob dies, as probably there will be more mobs dying than mobs riding minecarts.

ancientmarinerdev commented

2023-03-20 14:22:36 +01:00

I think you're right in having minecart logic in minecart.

We do have a dependency and it doesn't seem to be documented at present. I am not sure if minecarts depend on mobs or mobs depend on minecart and this will need some thinking. This does work however.

I did make the death check less frequent. It doesn't need to be checked 20 times a second, so I have made it every second.

Another issue I spotted, was if you have a passenger, and you shut down, they vanish. I have now ensured on load it does load the mob.

I think you're right in having minecart logic in minecart. We do have a dependency and it doesn't seem to be documented at present. I am not sure if minecarts depend on mobs or mobs depend on minecart and this will need some thinking. This does work however. I did make the death check less frequent. It doesn't need to be checked 20 times a second, so I have made it every second. Another issue I spotted, was if you have a passenger, and you shut down, they vanish. I have now ensured on load it does load the mob.

MrRar commented

2023-03-08 23:44:42 +01:00

How do you get a mob out of a cart?

FossFanatic commented

2023-03-09 12:31:05 +01:00

How do you get a mob out of a cart?

"either by choice, by breaking the minecart, or by passing over an activator rail"

https://minecraft.fandom.com/wiki/Minecart?so=search#Dismounting

>How do you get a mob out of a cart? "either by choice, by breaking the minecart, or by passing over an activator rail" https://minecraft.fandom.com/wiki/Minecart?so=search#Dismounting

anarquimico commented

2023-03-10 01:28:15 +01:00

Apologies for the delay in my response getting back to you.

No need for apologies, @ancientmarinerdev we all have a life outside of this project :)

Regarding the alterations, it's not discouraging! I'll work on them. But now I've been a bit busy and can only dive in the codes on weekends. So I'll mark this PR as WIP.

How do you get a mob out of a cart?

@MrRar, as FossFanatic mentioned, in MC the mob leaves the minecarts if the minecarts is broken or by passing on an activator rail. I thought that for now breaking the minecart would be enough and leave the activator rail as a "to do" thing. But I'll only let it as this if I couldn't get it to work properly. I'll give it a try first.

> Apologies for the delay in my response getting back to you. No need for apologies, @ancientmarinerdev we all have a life outside of this project :) Regarding the alterations, it's not discouraging! I'll work on them. But now I've been a bit busy and can only dive in the codes on weekends. So I'll mark this PR as WIP. > How do you get a mob out of a cart? @MrRar, as FossFanatic mentioned, in MC the mob leaves the minecarts if the minecarts is broken or by passing on an activator rail. I thought that for now breaking the minecart would be enough and leave the activator rail as a "to do" thing. But I'll only let it as this if I couldn't get it to work properly. I'll give it a try first.

👍 2 ❤️ 1

anarquimico changed title from ~~Make mobs ride minecarts~~ to WIP: Make mobs ride minecarts

2023-03-10 01:29:15 +01:00

anarquimico commented

2023-03-19 23:25:56 +01:00

Sorry for the delay on this. I've modified the code and think that now everything is working a bit more optimally. Gonna remove the WIP prefix as now is just testing.

anarquimico changed title from ~~WIP: Make mobs ride minecarts~~ to Make mobs ride minecarts

2023-03-19 23:26:01 +01:00

ancientmarinerdev approved these changes 2023-03-20 14:33:50 +01:00

ancientmarinerdev left a comment

Approving this. Thanks for working on this. I did add a few commits. One to ensure we use new style vectors, a second to ensure the minecart loaded with mob in it. Oh and make the death check less frequent. I hope you understand why I made those changes and if you have any questions. Feel free to ask.

👍 1

ancientmarinerdev force-pushed mobs_in_minecarts from f961910ed2 to ecf72db684

2023-03-20 14:37:34 +01:00

Compare

ancientmarinerdev added the

mobs

non-mob entities

labels 2023-03-20 14:38:42 +01:00

ancientmarinerdev added this to the 0.83.0 - Safe and Sound milestone 2023-03-20 14:38:46 +01:00

ancientmarinerdev referenced this issue from a commit

2023-03-20 14:42:39 +01:00

Merge pull request 'Make mobs ride minecarts' (#3507) from mobs_in_minecarts into master